This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Fix GCC build issues and restore "Additional APIs used by the MSVC compiler for loop collapse (rectangular and non-rectangular loops)"
ClosedPublic

Authored by vadikp-intel on Apr 22 2023, 11:07 PM.

Diff Detail

Event Timeline

vadikp-intel created this revision.Apr 22 2023, 11:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 22 2023, 11:07 PM
vadikp-intel requested review of this revision.Apr 22 2023, 11:07 PM
vadikp-intel added a reviewer: natgla.
vadikp-intel added a project: Restricted Project.
natgla accepted this revision.Apr 23 2023, 9:56 AM
This revision is now accepted and ready to land.Apr 23 2023, 9:56 AM

Folks, the same two bots that failed on the previous check-in that this one attempts to fix continue to fail, but with a different problem down the build line. Both are doing GCC builds that are now complaining about unresolved C++ runtime references that can apparently be fixed by adding -lstdc++ to the cmake linker set up. Not sure why this not being picked up by default in GCC configs - others (clang, MSVC, etc.) look OK.

also seeing bot fails here: https://lab.llvm.org/buildbot/#/builders/193/builds/30195
please revert and fix

And this patch introduces a lot of new warnings. Please fix them.

/home/ac.shilei.tian/Documents/vscode/llvm-project/openmp/runtime/src/kmp_collapse.cpp:303:44: warning: unused typedef 'UT' [-Wunused-local-typedef]
  typedef typename traits_t<T>::unsigned_t UT;
                                           ^
/home/ac.shilei.tian/Documents/vscode/llvm-project/openmp/runtime/src/kmp_collapse.cpp:304:42: warning: unused typedef 'ST' [-Wunused-local-typedef]
  typedef typename traits_t<T>::signed_t ST;
                                         ^
/home/ac.shilei.tian/Documents/vscode/llvm-project/openmp/runtime/src/kmp_collapse.cpp:379:42: warning: unused typedef 'ST' [-Wunused-local-typedef]
  typedef typename traits_t<T>::signed_t ST;
                                         ^
/home/ac.shilei.tian/Documents/vscode/llvm-project/openmp/runtime/src/kmp_collapse.cpp:378:44: warning: unused typedef 'UT' [-Wunused-local-typedef]
  typedef typename traits_t<T>::unsigned_t UT;
                                           ^
/home/ac.shilei.tian/Documents/vscode/llvm-project/openmp/runtime/src/kmp_collapse.cpp:510:42: warning: unused typedef 'ST' [-Wunused-local-typedef]
  typedef typename traits_t<T>::signed_t ST;
                                         ^
/home/ac.shilei.tian/Documents/vscode/llvm-project/openmp/runtime/src/kmp_collapse.cpp:574:42: warning: unused typedef 'ST' [-Wunused-local-typedef]
  typedef typename traits_t<T>::signed_t ST;
                                         ^
/home/ac.shilei.tian/Documents/vscode/llvm-project/openmp/runtime/src/kmp_collapse.cpp:984:42: warning: unused typedef 'ST' [-Wunused-local-typedef]
  typedef typename traits_t<T>::signed_t ST;
                                         ^
/home/ac.shilei.tian/Documents/vscode/llvm-project/openmp/runtime/src/kmp_collapse.cpp:988:11: warning: unused typedef 'big_span_t' [-Wunused-local-typedef]
          big_span_t;
          ^
/home/ac.shilei.tian/Documents/vscode/llvm-project/openmp/runtime/src/kmp_collapse.cpp:983:44: warning: unused typedef 'UT' [-Wunused-local-typedef]
  typedef typename traits_t<T>::unsigned_t UT;
                                           ^
/home/ac.shilei.tian/Documents/vscode/llvm-project/openmp/runtime/src/kmp_collapse.cpp:1213:44: warning: unused typedef 'UT' [-Wunused-local-typedef]
  typedef typename traits_t<T>::unsigned_t UT;
                                           ^
/home/ac.shilei.tian/Documents/vscode/llvm-project/openmp/runtime/src/kmp_collapse.cpp:1214:42: warning: unused typedef 'ST' [-Wunused-local-typedef]
  typedef typename traits_t<T>::signed_t ST;
                                         ^
11 warnings generated.

Folks, the same two bots that failed on the previous check-in that this one attempts to fix continue to fail, but with a different problem down the build line. Both are doing GCC builds that are now complaining about unresolved C++ runtime references that can apparently be fixed by adding -lstdc++ to the cmake linker set up. Not sure why this not being picked up by default in GCC configs - others (clang, MSVC, etc.) look OK.

I get the same errors, I'm going to go ahead and revert this. Someone will need to check to make sure that we're setting the correct language for the linking job.

tianshilei1992 added inline comments.Apr 24 2023, 2:27 PM
openmp/runtime/src/kmp_collapse.h
17

vector cannot be used here because it requires the standard C++ library. libomp is a C library *written in C++ syntax with header only STL*.

tianshilei1992 reopened this revision.Apr 24 2023, 2:27 PM
This revision is now accepted and ready to land.Apr 24 2023, 2:27 PM
tianshilei1992 requested changes to this revision.Apr 24 2023, 2:27 PM
This revision now requires changes to proceed.Apr 24 2023, 2:27 PM
natgla added inline comments.Apr 24 2023, 2:43 PM
openmp/runtime/src/kmp_collapse.h
17

yes, realized that after the failures. It's strange the pre-commit build allows C++ runtime usage.

Just an FYI to everyone: There are no real pre-merge checks for the openmp project anymore. The "build and test" step is just a setup, then report. No actual building or testing takes place.

https://buildkite.com/llvm-project/premerge-checks/builds/148337#0187acb9-a985-4e1a-aa53-1dcce046b641

openmp is in the excluded list.

...
INFO    Get list of projects affected by the change.
INFO    Projects directly modified by this patch:
  openmp
INFO    projects: {'openmp'}
INFO    added set() projects as they are affected
INFO    all affected projects(*) {'openmp'}
INFO    added set() projects as they are affected
INFO    all excluded projects(*) {'libcxxabi', 'libc', 'lldb', 'bolt', 'openmp', 'compiler-rt', 'cross-project-tests', 'check-cxxabi', 'libcxx'}
INFO    effective projects list set()
INFO    windows_projects: []
INFO    Files modified by this patch:
...

Implementation changes to remove the dependency on the C++ standard library

vadikp-intel marked 2 inline comments as done.May 11 2023, 3:14 PM
vadikp-intel added a comment.EditedMay 11 2023, 3:21 PM

Could someone in the know comment whether the openmp removal was unrelated to the previous GCC build failures, or it was in which case what action may be required to restore it now that this is being fixed? Thanks

tianshilei1992 accepted this revision.May 12 2023, 10:15 AM

I'll unblock it for now though I didn't test it because I lost my development environment.

This revision is now accepted and ready to land.May 12 2023, 10:15 AM

Thanks, this has been tested with GCC 12 on Windows.

tianshilei1992 added inline comments.
openmp/runtime/src/kmp_collapse.cpp
132

I'm not sure if we can use std::abs here w/o pulling in <cmath>. However, libomp doesn't depend on libm anyway, so we might probably just want to "reinvent the wheels" for std::abs here.

jhuber6 added inline comments.May 16 2023, 9:55 AM
openmp/runtime/src/kmp_collapse.cpp
132

If this was C++23 we could rely on them being constexpr. I think most implementations will provide this without cmath, but it's certainly possible to call into libm so it's safest to avoid the reference.