-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Upgrade 3rdparty/openmp to release_90 version #17012
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im fine with this after we can resolve that there’s no bug in mxnet which is causing the assert which led to this PR.
Also would be interesting to note if the assert is still there in this version and if not, why was it removed (ie git blame PR)?
Please see the last line of the PR description above. Given that the assert is still there and it is not failing anymore I think we can establish that there is no bug in MXNet. Thus I can't follow your reasoning that there's a bug in MXNet. Then please rescind your veto or give some evidence that there is a bug. Currently I consider this to be a veto without technical justification. Thanks! |
536d7ec
to
627fdd4
Compare
Force push to retrigger CI. Failed with Github API error for 5/11 jobs (other 6 passed):
CC @marcoabreu |
Chris rescinded his veto: https://lists.apache.org/thread.html/fbb9c65977afcea064245f7d72bc15a1638fa6d5dcd42c50dba3c701%40%3Cdev.mxnet.apache.org%3E Thus going ahead and merging this. |
i think referring to asking for more information before approving as a
“veto” is a bit over-dramatic
…On Mon, Dec 9, 2019 at 12:19 AM Leonard Lausen ***@***.***> wrote:
Chris rescinded his veto:
https://lists.apache.org/thread.html/fbb9c65977afcea064245f7d72bc15a1638fa6d5dcd42c50dba3c701%40%3Cdev.mxnet.apache.org%3E
Thus going ahead and merging this.
—
You are receiving this hunk because your review was requested.
Reply to this email directly, view it on GitHub
<#17012>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACVWZ7JGFBQT5DY3MC7FIDTQXX5PNANCNFSM4JXZLT2A>
.
|
Sorry, that wasn't my intention. I just wanted to reference your approval on the mailinglist. |
OpenMP offloading was introduced some time during the past two years and is enabled by default. With upgrading 3rdparty/openmp in apache#17012 it was made part of the MXNet CMake build. But we don't use OpenMP offloading and the Cuda target in the llvm OpenMP Offloading build is broken in our setting.
OpenMP offloading was introduced some time during the past two years and is enabled by default. With upgrading 3rdparty/openmp in apache#17012 it was made part of the MXNet CMake build. But we don't use OpenMP offloading and the Cuda target in the llvm OpenMP Offloading build is broken in our setting.
OpenMP offloading was introduced some time during the past two years and is enabled by default. With upgrading 3rdparty/openmp in apache#17012 it was made part of the MXNet CMake build. But we don't use OpenMP offloading and the Cuda target in the llvm OpenMP Offloading build is broken in our setting.
* Disable OpenMP offloading support for 3rdparty/openmp OpenMP offloading was introduced some time during the past two years and is enabled by default. With upgrading 3rdparty/openmp in #17012 it was made part of the MXNet CMake build. But we don't use OpenMP offloading and the Cuda target in the llvm OpenMP Offloading build is broken in our setting. * Update CMake on CI
* Disable OpenMP offloading support for 3rdparty/openmp OpenMP offloading was introduced some time during the past two years and is enabled by default. With upgrading 3rdparty/openmp in apache#17012 it was made part of the MXNet CMake build. But we don't use OpenMP offloading and the Cuda target in the llvm OpenMP Offloading build is broken in our setting. * Update CMake on CI
* Upgrade 3rdparty/openmp to release_90 version (#17012) Fixes #10856 * Fix omp assert issue (#17039) * Disable OpenMP offloading support for 3rdparty/openmp (#17098) * Disable OpenMP offloading support for 3rdparty/openmp OpenMP offloading was introduced some time during the past two years and is enabled by default. With upgrading 3rdparty/openmp in #17012 it was made part of the MXNet CMake build. But we don't use OpenMP offloading and the Cuda target in the llvm OpenMP Offloading build is broken in our setting. * Update CMake on CI * Fix reshape interoperability test (#17155) * fix reshape interoperability test * fix for scipy import Co-authored-by: Chris Olivier <[email protected]> Co-authored-by: Hao Jin <[email protected]>
Description
Fixes #10856
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Changes
Comments
See #10856 for a discussion on the symptom. It seems it's due to a bug in llvm openmp fixed in the meantime.
The previously failing assertion is still part of the
release_90
version. It is at https://github.com/llvm-mirror/openmp/blob/release_90/runtime/src/kmp_runtime.cpp#L6616But the assertion does not fail anymore (based on my test with
cmake -DUSE_CUDA=0 -DUSE_MKLDNN=0 -DCMAKE_EXPORT_COMPILE_COMMANDS=1 -DBUILD_CYTHON_MODULES=1 -DUSE_OPENCV=1 -DCMAKE_BUILD_TYPE=Debug ..
, which leads to assertion failure without this PR: #10856 (comment))You can compare the lines in blame mode and conclude they are the same
https://github.com/llvm-mirror/openmp/blame/37c72127e90360a020f351f18d9cccfc30e5145a/runtime/src/kmp_runtime.cpp#L6481
https://github.com/llvm-mirror/openmp/blame/release_90/runtime/src/kmp_runtime.cpp#L6616