Page MenuHomePhabricator

An attempt to abandon omptarget out-of-tree builds.
ClosedPublic

Authored by vzakhari on Apr 28 2021, 6:46 PM.

Details

Summary

I want to start using LLVM component libraries in libomptarget to stop duplicating implementations already available in LLVM (e.g. LLVMObject, LLVMSupport, etc.). Without relying on LLVM in all libomptarget builds one has to provide fallback implementation for each used LLVM feature.

This is an attempt to stop supporting out-of-llvm-tree builds of libomptarget. I understand that I may need to revert this, if this affects downstream projects in a bad way.

Diff Detail

Event Timeline

vzakhari created this revision.Apr 28 2021, 6:46 PM
vzakhari requested review of this revision.Apr 28 2021, 6:46 PM

Is this going to block all direct builds of openmp or just these that don't have an LLVM source tree handy? Also, do you really need *sources* and not just compiled libLLVM?

Is this going to block all direct builds of openmp or just these that don't have an LLVM source tree handy?

This is aimed at libomptarget, libopenmp can still build without llvm, provided it excludes the target part.

Also, do you really need *sources* and not just compiled libLLVM?

I think, for this part, binaries would do. There are various hazards around getting compiled libraries that match the current compiler settings, but they're solvable. Note though that libomptarget already requires the llvm source (for shared magic numbers, mostly)

openmp/CMakeLists.txt
63–64

Do we still have a definition of LIBOMPTARGET_LLVM_INCLUDE_DIRS with this block deleted? Looks unset on one side of the branch from L7 to me, but maybe hitting the fatal error further down catches that path.

Is this going to block all direct builds of openmp or just these that don't have an LLVM source tree handy?

This is aimed at libomptarget, libopenmp can still build without llvm, provided it excludes the target part.

This doesn't really answer my question.

For me, the main use-case for stand-alone builds is runtime code development. In that case, I use a ("nightly") build of LLVM and use that to compile and test stand-alone runtime builds.
Switching branches regularly results in recompiling a lot of code. With stand-alone runtime builds, this is restricted to OpenMP runtime code.

If you really only depend on LLVM libraries and header files, they should get installed. find_package(LLVM) should provide you all necessary information to include and link those libraries.

Is this going to block all direct builds of openmp or just these that don't have an LLVM source tree handy?

This is aimed at libomptarget, libopenmp can still build without llvm, provided it excludes the target part.

This doesn't really answer my question.

Perhaps you could rephrase the question? This patch is intended to leave openmp building exactly like it used to. The subcomponent libomptarget would no longer build without access to the llvm libraries, but that subcomponent is optional.

For me, the main use-case for stand-alone builds is runtime code development. In that case, I use a ("nightly") build of LLVM and use that to compile and test stand-alone runtime builds.
Switching branches regularly results in recompiling a lot of code. With stand-alone runtime builds, this is restricted to OpenMP runtime code.

If you really only depend on LLVM libraries and header files, they should get installed. find_package(LLVM) should provide you all necessary information to include and link those libraries.

That is interesting. The header files used by libomptarget may not be on the installed list, but perhaps they should be. For efficient development purposes, using headers from the source tree with binaries from somewhere on disk would be great, provided that slight mashup works. I've been building llvm from scratch repeatedly, changing to a find_package workflow would do good things for iteration time.

Is this going to block all direct builds of openmp or just these that don't have an LLVM source tree handy?

This is aimed at libomptarget, libopenmp can still build without llvm, provided it excludes the target part.

This doesn't really answer my question.

Perhaps you could rephrase the question? This patch is intended to leave openmp building exactly like it used to. The subcomponent libomptarget would no longer build without access to the llvm libraries, but that subcomponent is optional.

For me, the main use-case for stand-alone builds is runtime code development. In that case, I use a ("nightly") build of LLVM and use that to compile and test stand-alone runtime builds.
Switching branches regularly results in recompiling a lot of code. With stand-alone runtime builds, this is restricted to OpenMP runtime code.

If you really only depend on LLVM libraries and header files, they should get installed. find_package(LLVM) should provide you all necessary information to include and link those libraries.

That is interesting. The header files used by libomptarget may not be on the installed list, but perhaps they should be. For efficient development purposes, using headers from the source tree with binaries from somewhere on disk would be great, provided that slight mashup works. I've been building llvm from scratch repeatedly, changing to a find_package workflow would do good things for iteration time.

Right, we were also talking about find_package(LLVM) here: D99553#2687851
I do not have much experience with this approach, but I will try to make it work.

@mgorny, if we switch to find_package(LLVM), one will need a built/installed LLVM to build libomptarget in addition to libopenmp. To link LLVM component libraries into libomptarget I would like to use LLVM macros provided in LLVM CMake files. Aligning the libomptarget build options with LLVM build options (e.g. EH, RTTI related options) will be a pain otherwise.

vzakhari updated this revision to Diff 342036.Apr 30 2021, 2:15 PM
tianshilei1992 added inline comments.
openmp/libomptarget/deviceRTLs/amdgcn/CMakeLists.txt
13

This check could be avoided if the include of libomptarget is guarded by the check of LLVM. That said, all CMake code in libomptarget can assume that LLVM is already there.

vzakhari added inline comments.May 3 2021, 10:10 AM
openmp/libomptarget/deviceRTLs/amdgcn/CMakeLists.txt
13

There is a check in openmp/libomptarget/CMakeLists.txt that will fail, if LLVM is not there:

# LLVM source tree is required at build time for libomptarget
if (NOT LIBOMPTARGET_LLVM_INCLUDE_DIRS)
  message(FATAL_ERROR "Missing definition for LIBOMPTARGET_LLVM_INCLUDE_DIRS")
endif()

I will remove this check here.

vzakhari updated this revision to Diff 342438.May 3 2021, 10:12 AM

@ggeorgakoudis, please check if the changes in openmp/libomptarget/src/CMakeLists.txt look acceptable to you.

For me, the main use-case for stand-alone builds is runtime code development. In that case, I use a ("nightly") build of LLVM and use that to compile and test stand-alone runtime builds.
Switching branches regularly results in recompiling a lot of code. With stand-alone runtime builds, this is restricted to OpenMP runtime code.

I don't really get why you would not rebase the runtime you are developing instead but as long as this doesn't block the patch it does not matter much.

vzakhari updated this revision to Diff 342574.May 3 2021, 3:51 PM

The latest change-set builds in-tree and with a pre-installed LLVM.

ggeorgakoudis added inline comments.May 3 2021, 5:09 PM
openmp/libomptarget/src/CMakeLists.txt
36–37

Is this still needed now that omptarget does not build with add_llvm_library?

vzakhari updated this revision to Diff 342612.May 3 2021, 5:59 PM
vzakhari marked an inline comment as done.

For me, the main use-case for stand-alone builds is runtime code development. In that case, I use a ("nightly") build of LLVM and use that to compile and test stand-alone runtime builds.
Switching branches regularly results in recompiling a lot of code. With stand-alone runtime builds, this is restricted to OpenMP runtime code.

I don't really get why you would not rebase the runtime you are developing instead but as long as this doesn't block the patch it does not matter much.

The branch is not necessarily my branch. Even checking out the branch and temporary rebasing would touch enough files to trigger a rebuild of most of LLVM.

I just realized, that my other case for building OpenMP standalone is to get a debugging build. I usually only need the runtime libraries built with debug info. I could probably change the build type in the runtime build directory, but I prefer to build and install both release and debug runtime.
For debugging, I then only need to load my omp-debug module to pick the debug runtime during execution.

openmp/README.rst
249–250

or LLVM_ENABLE_RUNTIMES

vzakhari updated this revision to Diff 343049.May 5 2021, 8:02 AM
vzakhari marked an inline comment as done.
JonChesterfield accepted this revision.May 6 2021, 6:36 PM

It looks to me like this covers the bases. We can build with an LLVM located through find_package, or against the source tree. The header files that are included from llvm are in the install tree, so I think all is good.

As far as I can tell the initial concerns are all well handled by find_package. We can always revert this if more information comes to light after it lands.

This revision is now accepted and ready to land.May 6 2021, 6:36 PM

Thank you, Jon! I will push it tomorrow morning.

This revision was automatically updated to reflect the committed changes.
weiwang added a subscriber: weiwang.EditedMay 10 2021, 5:19 PM

We are getting build errors internally with this change. They are all related to libomptarget. Our internal build script uses gcc to build llvm.

One example:

[108/122] Generating target_impl.gfx700.bc
FAILED: projects/openmp/libomptarget/deviceRTLs/amdgcn/target_impl.gfx700.bc
cd /data/users/wangwei/tp2/llvm-build/platform009/build_nopic/projects/openmp/libomptarget/deviceRTLs/amdgcn && /data/users/wangwei/tp2/llvm-build/platform009/build_nopic/bin/clang-13 -xc++ -c -std=c++14 -ffreestanding -target amdgcn-amd-amdhsa -emit-llvm -Xclang -aux-triple -Xclang x86_64-unknown-linux-gnu -fopenmp -fopenmp-cuda-mode -Xclang -fopenmp-is-device -D__AMDGCN__ -Xclang -target-cpu -Xclang gfx700 -fvisibility=default -Wno-unused-value -nogpulib -O2 -I/home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/amdgcn/src -I/home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/common/include -I/home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs /home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/amdgcn/src/target_impl.hip -o target_impl.gfx700.bc
/home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/amdgcn/src/target_impl.hip:185:25: error: use of undeclared identifier '__UINT64_C'
  lo = (uint32_t)(val & UINT64_C(0x00000000FFFFFFFF));
                        ^
/home/wangwei/fbsource/fbcode/third-party2/gcc/9.x/centos7-native/3bed279/lib/gcc/x86_64-redhat-linux-gnu/9.x/include/stdint-gcc.h:254:21: note: expanded from macro 'UINT64_C'
#define UINT64_C(c) __UINT64_C(c)
                    ^
/home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/amdgcn/src/target_impl.hip:186:26: error: use of undeclared identifier '__UINT64_C'
  hi = (uint32_t)((val & UINT64_C(0xFFFFFFFF00000000)) >> 32);
                         ^
/home/wangwei/fbsource/fbcode/third-party2/gcc/9.x/centos7-native/3bed279/lib/gcc/x86_64-redhat-linux-gnu/9.x/include/stdint-gcc.h:254:21: note: expanded from macro 'UINT64_C'
#define UINT64_C(c) __UINT64_C(c)
                    ^
2 errors generated.

We are getting build errors internally with this change. They are all related to libomptarget. Our internal build script uses gcc to build llvm.

One example:

[108/122] Generating target_impl.gfx700.bc
FAILED: projects/openmp/libomptarget/deviceRTLs/amdgcn/target_impl.gfx700.bc
cd /data/users/wangwei/tp2/llvm-build/platform009/build_nopic/projects/openmp/libomptarget/deviceRTLs/amdgcn && /data/users/wangwei/tp2/llvm-build/platform009/build_nopic/bin/clang-13 -xc++ -c -std=c++14 -ffreestanding -target amdgcn-amd-amdhsa -emit-llvm -Xclang -aux-triple -Xclang x86_64-unknown-linux-gnu -fopenmp -fopenmp-cuda-mode -Xclang -fopenmp-is-device -D__AMDGCN__ -Xclang -target-cpu -Xclang gfx700 -fvisibility=default -Wno-unused-value -nogpulib -O2 -I/home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/amdgcn/src -I/home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/common/include -I/home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs /home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/amdgcn/src/target_impl.hip -o target_impl.gfx700.bc
/home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/amdgcn/src/target_impl.hip:185:25: error: use of undeclared identifier '__UINT64_C'
  lo = (uint32_t)(val & UINT64_C(0x00000000FFFFFFFF));
                        ^
/home/wangwei/fbsource/fbcode/third-party2/gcc/9.x/centos7-native/3bed279/lib/gcc/x86_64-redhat-linux-gnu/9.x/include/stdint-gcc.h:254:21: note: expanded from macro 'UINT64_C'
#define UINT64_C(c) __UINT64_C(c)
                    ^
/home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/amdgcn/src/target_impl.hip:186:26: error: use of undeclared identifier '__UINT64_C'
  hi = (uint32_t)((val & UINT64_C(0xFFFFFFFF00000000)) >> 32);
                         ^
/home/wangwei/fbsource/fbcode/third-party2/gcc/9.x/centos7-native/3bed279/lib/gcc/x86_64-redhat-linux-gnu/9.x/include/stdint-gcc.h:254:21: note: expanded from macro 'UINT64_C'
#define UINT64_C(c) __UINT64_C(c)
                    ^
2 errors generated.

Hello @weiwang, thank you for reporting this. Can you please provide some details how to reproduce it?

One strange thing is that the pre-built /data/users/wangwei/tp2/llvm-build/platform009/build_nopic/bin/clang-13 includes stdint-gcc.h, where it takes the definition for UINT64_C. In my builds clang takes the definition from its own lib/clang/13.0.0/include/stdint.h.

If you do not need libomptarget for your package, you may pass -DOPENMP_ENABLE_LIBOMPTARGET=OFF to cmake.

We are getting build errors internally with this change. They are all related to libomptarget. Our internal build script uses gcc to build llvm.

One example:

[108/122] Generating target_impl.gfx700.bc
FAILED: projects/openmp/libomptarget/deviceRTLs/amdgcn/target_impl.gfx700.bc
cd /data/users/wangwei/tp2/llvm-build/platform009/build_nopic/projects/openmp/libomptarget/deviceRTLs/amdgcn && /data/users/wangwei/tp2/llvm-build/platform009/build_nopic/bin/clang-13 -xc++ -c -std=c++14 -ffreestanding -target amdgcn-amd-amdhsa -emit-llvm -Xclang -aux-triple -Xclang x86_64-unknown-linux-gnu -fopenmp -fopenmp-cuda-mode -Xclang -fopenmp-is-device -D__AMDGCN__ -Xclang -target-cpu -Xclang gfx700 -fvisibility=default -Wno-unused-value -nogpulib -O2 -I/home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/amdgcn/src -I/home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/common/include -I/home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs /home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/amdgcn/src/target_impl.hip -o target_impl.gfx700.bc
/home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/amdgcn/src/target_impl.hip:185:25: error: use of undeclared identifier '__UINT64_C'
  lo = (uint32_t)(val & UINT64_C(0x00000000FFFFFFFF));
                        ^
/home/wangwei/fbsource/fbcode/third-party2/gcc/9.x/centos7-native/3bed279/lib/gcc/x86_64-redhat-linux-gnu/9.x/include/stdint-gcc.h:254:21: note: expanded from macro 'UINT64_C'
#define UINT64_C(c) __UINT64_C(c)
                    ^
/home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/amdgcn/src/target_impl.hip:186:26: error: use of undeclared identifier '__UINT64_C'
  hi = (uint32_t)((val & UINT64_C(0xFFFFFFFF00000000)) >> 32);
                         ^
/home/wangwei/fbsource/fbcode/third-party2/gcc/9.x/centos7-native/3bed279/lib/gcc/x86_64-redhat-linux-gnu/9.x/include/stdint-gcc.h:254:21: note: expanded from macro 'UINT64_C'
#define UINT64_C(c) __UINT64_C(c)
                    ^
2 errors generated.

Hello @weiwang, thank you for reporting this. Can you please provide some details how to reproduce it?

One strange thing is that the pre-built /data/users/wangwei/tp2/llvm-build/platform009/build_nopic/bin/clang-13 includes stdint-gcc.h, where it takes the definition for UINT64_C. In my builds clang takes the definition from its own lib/clang/13.0.0/include/stdint.h.

Thanks for the quick response. It may not be easily reproducible since the build script that triggers this sets up its own environment. This is part of the company's internal build system. During my local try, clang built clang always works, but the build script uses gcc to build clang. Maybe gcc would insert its own library headers into search path, and this could cause some confusing about the order of include paths? But again, we have always used gcc to build clang, and it never had issue until now. I am not sure how this change would change anything.

If you do not need libomptarget for your package, you may pass -DOPENMP_ENABLE_LIBOMPTARGET=OFF to cmake.

With -DOPENMP_ENABLE_LIBOMPTARGET=OFF, the error is gone. I'll check internally to see if libomptarget can be disabled. Meanwhile, it would still be great to know what went wrong.

@weiwang, I hope you do not mind if I ask you to run some experiments on your side? Otherwise, I am not sure how to proceed :)

Can you please run the command that fails, pass -E to it and check where the header files are coming from? I.e. run this:

cd /data/users/wangwei/tp2/llvm-build/platform009/build_nopic/projects/openmp/libomptarget/deviceRTLs/amdgcn && /data/users/wangwei/tp2/llvm-build/platform009/build_nopic/bin/clang-13 -xc++ -c -std=c++14 -ffreestanding -target amdgcn-amd-amdhsa -emit-llvm -Xclang -aux-triple -Xclang x86_64-unknown-linux-gnu -fopenmp -fopenmp-cuda-mode -Xclang -fopenmp-is-device -D__AMDGCN__ -Xclang -target-cpu -Xclang gfx700 -fvisibility=default -Wno-unused-value -nogpulib -O2 -I/home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/amdgcn/src -I/home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/common/include -I/home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs /home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/amdgcn/src/target_impl.hip -E

Regarding your question "how this change would change anything", can you please check for "Not building AMDGCN device RTL: AOMP not found" message in your "old" builds? I suppose my change for find_package invocation might have caused different behavior in your setup. Before my change we were looking for LLVM in the following paths:

$ENV{AOMP}
$ENV{HOME}/rocm/aomp
/opt/rocm/aomp
/usr/lib/rocm/aomp
${LIBOMPTARGET_NVPTX_CUDA_COMPILER_DIR}
${LIBOMPTARGET_NVPTX_CUDA_LINKER_DIR}
${CMAKE_CXX_COMPILER_DIR}

Not we look for LLVM in all paths that cmake examines by default: https://cmake.org/cmake/help/latest/command/find_package.html#search-procedure

weiwang added a comment.EditedMay 10 2021, 7:06 PM

@weiwang, I hope you do not mind if I ask you to run some experiments on your side? Otherwise, I am not sure how to proceed :)

Can you please run the command that fails, pass -E to it and check where the header files are coming from? I.e. run this:

cd /data/users/wangwei/tp2/llvm-build/platform009/build_nopic/projects/openmp/libomptarget/deviceRTLs/amdgcn && /data/users/wangwei/tp2/llvm-build/platform009/build_nopic/bin/clang-13 -xc++ -c -std=c++14 -ffreestanding -target amdgcn-amd-amdhsa -emit-llvm -Xclang -aux-triple -Xclang x86_64-unknown-linux-gnu -fopenmp -fopenmp-cuda-mode -Xclang -fopenmp-is-device -D__AMDGCN__ -Xclang -target-cpu -Xclang gfx700 -fvisibility=default -Wno-unused-value -nogpulib -O2 -I/home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/amdgcn/src -I/home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/common/include -I/home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs /home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/amdgcn/src/target_impl.hip -E

Regarding your question "how this change would change anything", can you please check for "Not building AMDGCN device RTL: AOMP not found" message in your "old" builds? I suppose my change for find_package invocation might have caused different behavior in your setup. Before my change we were looking for LLVM in the following paths:

$ENV{AOMP}
$ENV{HOME}/rocm/aomp
/opt/rocm/aomp
/usr/lib/rocm/aomp
${LIBOMPTARGET_NVPTX_CUDA_COMPILER_DIR}
${LIBOMPTARGET_NVPTX_CUDA_LINKER_DIR}
${CMAKE_CXX_COMPILER_DIR}

Not we look for LLVM in all paths that cmake examines by default: https://cmake.org/cmake/help/latest/command/find_package.html#search-procedure

Right. The "old" build cmake outputs "LIBOMPTARGET: Not building AMDGCN device RTL: AOMP not found", and the "new" build cmake outputs "LIBOMPTARGET: Building AMDGCN device RTL. Using clang from in-tree build".

It seem "AMDGCN device RTL" was not built due to missing clang in old build, and this change kind of re-enabled it. I wonder if there is a cmake flag to explicitly disable "AMDGCN device RTL" but still build the rest of libomptarget. I think keep disabling "AMDGCN device RTL" is fine since we've never really built it anyway.

I have a guess that clang-resource-headers are not built at the point, where we invoke clang. Can you please check this workaround in openmp/libomptarget/deviceRTLs/amdgcn/CMakeLists.txt?

add_custom_command(
  OUTPUT ${bc1_filename}
  COMMAND ${cu_cmd} ${file} -o ${bc1_filename}
  DEPENDS ${file} ${h_files} clang-resource-headers)

I added clang-resource-headers dependency after ${h_files}.

weiwang added a comment.EditedMay 10 2021, 8:31 PM

I have a guess that clang-resource-headers are not built at the point, where we invoke clang. Can you please check this workaround in openmp/libomptarget/deviceRTLs/amdgcn/CMakeLists.txt?

add_custom_command(
  OUTPUT ${bc1_filename}
  COMMAND ${cu_cmd} ${file} -o ${bc1_filename}
  DEPENDS ${file} ${h_files} clang-resource-headers)

I added clang-resource-headers dependency after ${h_files}.

Tried the work-around, deviceRTLs is still being built and same error.

Also used -H to show the header includes

#include "..." search starts here:
#include <...> search starts here:
 /home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/amdgcn/src
 /home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/common/include
 /home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs
 /home/wangwei/fbsource/fbcode/third-party2/libgcc/9.x/platform009/9202ce7/include/c++/9.x
 /home/wangwei/fbsource/fbcode/third-party2/libgcc/9.x/platform009/9202ce7/include/c++/9.x/x86_64-facebook-linux
 /home/wangwei/fbsource/fbcode/third-party2/libgcc/9.x/platform009/9202ce7/include/c++/9.x/backward
 /home/wangwei/fbsource/fbcode/third-party2/gcc/9.x/centos7-native/3bed279/include
 /home/wangwei/fbsource/fbcode/third-party2/gcc/9.x/centos7-native/3bed279/lib/gcc/x86_64-redhat-linux-gnu/9.x/include         <--- used definition here
 ... ...
 /usr/local/include
 /data/users/wangwei/tp2/llvm-build/platform009/build_pic/lib/clang/13.0.20190721/include.      <--- should use here
 /usr/include
End of search list.
. /home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/common/omptarget.h
.. /home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/common/allocator.h
.. /home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/common/debug.h
... /home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/common/device_environment.h
.... /home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/amdgcn/src/target_impl.h
..... /home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/amdgcn/src/amdgcn_interface.h
...... /home/wangwei/fbsource/fbcode/third-party2/gcc/9.x/centos7-native/3bed279/lib/gcc/x86_64-redhat-linux-gnu/9.x/include/stdint.h
....... /home/wangwei/fbsource/fbcode/third-party2/gcc/9.x/centos7-native/3bed279/lib/gcc/x86_64-redhat-linux-gnu/9.x/include/stdint-gcc.h
..... /home/wangwei/fbsource/fbcode/third-party2/gcc/9.x/centos7-native/3bed279/lib/gcc/x86_64-redhat-linux-gnu/9.x/include/stddef.h
... /home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/target_interface.h
... /home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/common/support.h
.... /home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/interface.h
..... /home/wangwei/fbsource/fbcode/third-party2/gcc/9.x/centos7-native/3bed279/lib/gcc/x86_64-redhat-linux-gnu/9.x/include/stddef.h
.. /home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/common/state-queue.h
... /home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/common/state-queuei.h
.... /home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/common/state-queue.h
.. /home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/common/omptargeti.h

/home/wangwei/fbsource/fbcode/third-party2/gcc/9.x/centos7-native/3bed279/lib/gcc/x86_64-redhat-linux-gnu/9.x/include comes before /data/users/wangwei/tp2/llvm-build/platform009/build_pic/lib/clang/13.0.20190721/include

I wonder if there is a cmake flag to explicitly disable "AMDGCN device RTL" but still build the rest of libomptarget. I think keep disabling deviceRTL is fine since we've never really built it anyway.

weiwang added a comment.EditedMay 10 2021, 8:48 PM

Looks like -DLIBOMPTARGET_AMDGCN_GFXLIST="" would disable Device RTL build, but still build rest of the libomptarget.

Since this flag sets mcpus empty, maybe we can just print a log and return here before searching for clang in openmp/libomptarget/deviceRTLs/amdgcn/CMakeLists.txt:

# create libraries
set(mcpus gfx700 gfx701 gfx801 gfx803 gfx900 gfx906)
if (DEFINED LIBOMPTARGET_AMDGCN_GFXLIST)
  set(mcpus ${LIBOMPTARGET_AMDGCN_GFXLIST})
endif()

if (NOT mcpus)
  libomptarget_say("Not building AMDGCN device RTL: empty mcpus list")
  return()
endif()

Yes, you can disable the device RTL build using this control.

It looks like your scripts define one of these PATH variables. And it has something to do with -ffreestanding. I was able to reproduce it with the following example:

#include <stdint.h>

uint64_t foo() {
  return UINT64_C(0x1);
}

$ CPLUS_INCLUDE_PATH=<path>/x86_64-linux-gnu/9.2.0/include clang test.cpp -c -ffreestanding

test.cpp:4:10: error: use of undeclared identifier '__UINT64_C'
  return UINT64_C(0x1);
         ^
<path>/x86_64-linux-gnu/9.2.0/include/stdint-gcc.h:254:21: note: expanded from macro 'UINT64_C'
#define UINT64_C(c) __UINT64_C(c)
                    ^
1 error generated.

I am not sure whether clang is supposed to work in this conditions or not, but the error is not specific to amdgcn device RTL build. Basically, any target that uses the in-tree clang may fail the same way (e.g. LIT tests).

Yes, you can disable the device RTL build using this control.

It looks like your scripts define one of these PATH variables. And it has something to do with -ffreestanding. I was able to reproduce it with the following example:

#include <stdint.h>

uint64_t foo() {
  return UINT64_C(0x1);
}

$ CPLUS_INCLUDE_PATH=<path>/x86_64-linux-gnu/9.2.0/include clang test.cpp -c -ffreestanding

test.cpp:4:10: error: use of undeclared identifier '__UINT64_C'
  return UINT64_C(0x1);
         ^
<path>/x86_64-linux-gnu/9.2.0/include/stdint-gcc.h:254:21: note: expanded from macro 'UINT64_C'
#define UINT64_C(c) __UINT64_C(c)
                    ^
1 error generated.

I am not sure whether clang is supposed to work in this conditions or not, but the error is not specific to amdgcn device RTL build. Basically, any target that uses the in-tree clang may fail the same way (e.g. LIT tests).

Ah, this is good finding! I do find the include path in both CPLUS_INCLUDE_PATH and C_INCLUDE_PATH from the env created by script.

JonChesterfield added a comment.EditedMay 11 2021, 12:05 AM

There's no particular reason to expect GCC headers to work on amdgcn. The GCC backend for it is incompatible with the clang one.

That's part of why the library builds as ffreestanding. The idea is to encourage use of the headers that ship with clang, as they are aware of the architecture and others are unlikely to be.

Setting CPLUS_INCLUDE_PATH appears to be overriding that, forcing clang to build with headers that don't work. Could you not do that?

I'm probably open to removing stdint et al as a dependency, but would like to understand your use case before writing the boilerplate involved.

Edit: ideal would probably be a clang flag telling it to ignore environment variables specifying include paths and only use its own headers. Such a flag may already exist

We are getting build errors internally with this change. They are all related to libomptarget. Our internal build script uses gcc to build llvm.

One example:

[108/122] Generating target_impl.gfx700.bc
FAILED: projects/openmp/libomptarget/deviceRTLs/amdgcn/target_impl.gfx700.bc
cd /data/users/wangwei/tp2/llvm-build/platform009/build_nopic/projects/openmp/libomptarget/deviceRTLs/amdgcn && /data/users/wangwei/tp2/llvm-build/platform009/build_nopic/bin/clang-13 -xc++ -c -std=c++14 -ffreestanding -target amdgcn-amd-amdhsa -emit-llvm -Xclang -aux-triple -Xclang x86_64-unknown-linux-gnu -fopenmp -fopenmp-cuda-mode -Xclang -fopenmp-is-device -D__AMDGCN__ -Xclang -target-cpu -Xclang gfx700 -fvisibility=default -Wno-unused-value -nogpulib -O2 -I/home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/amdgcn/src -I/home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/common/include -I/home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs /home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/amdgcn/src/target_impl.hip -o target_impl.gfx700.bc
/home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/amdgcn/src/target_impl.hip:185:25: error: use of undeclared identifier '__UINT64_C'
  lo = (uint32_t)(val & UINT64_C(0x00000000FFFFFFFF));
                        ^
/home/wangwei/fbsource/fbcode/third-party2/gcc/9.x/centos7-native/3bed279/lib/gcc/x86_64-redhat-linux-gnu/9.x/include/stdint-gcc.h:254:21: note: expanded from macro 'UINT64_C'
#define UINT64_C(c) __UINT64_C(c)
                    ^
/home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/amdgcn/src/target_impl.hip:186:26: error: use of undeclared identifier '__UINT64_C'
  hi = (uint32_t)((val & UINT64_C(0xFFFFFFFF00000000)) >> 32);
                         ^
/home/wangwei/fbsource/fbcode/third-party2/gcc/9.x/centos7-native/3bed279/lib/gcc/x86_64-redhat-linux-gnu/9.x/include/stdint-gcc.h:254:21: note: expanded from macro 'UINT64_C'
#define UINT64_C(c) __UINT64_C(c)
                    ^
2 errors generated.

I think, that D101265 also has an impact here: The patch tries to use the just-built clang as compiler for the bc files, without letting CMake run full compiler detection. The just-built clang fails to find the C++ headers.

@weiwang you could try to build OpenMP as runtime rather than project: use LLVM_ENABLE_RUNTIMES=openmp instead of LLVM_ENABLE_PROJECTS=openmp.

@JonChesterfield do you mean, that amdgcn is only compatible with LLVM's libc++ and cannot be built against libstdc++? As I understand, clang will use GNU's headers, if libstdc++ is used?

JonChesterfield added a comment.EditedMay 11 2021, 2:32 AM

@JonChesterfield do you mean, that amdgcn is only compatible with LLVM's libc++ and cannot be built against libstdc++? As I understand, clang will use GNU's headers, if libstdc++ is used?

Nope. Neither libc++ nor libstdc++ work with amdgpu as far as I know, and ffreestanding stops clang trying to use either.

It uses stdint.h, stddef.h, maybe a couple of others for convenience. Limited to the header only libc headers that clang provides.

We can drop those by providing typedefs for uint64_t etc, but my preference is to continue using the C headers that ship with clang. I'm not sure if -nostdinc plus explicitly including from the clang install tree would work with that environment variable set.

CPLUS_INCLUDE_PATH is handled by ToolChains/Clang.cpp. Looks like it is unconditionally translated to -cxx-isystem and appended to the include path. It may therefore be possible to override it by manually passing an -I or -isystem path to the amdgcn devicertl

export CPLUS_INCLUDE_PATH=/usr/lib/gcc/x86_64-linux-gnu/9/include
./build.sh
reproduces the above on my system, followed by a lot more errors from glibc including xmmintrin.h which contains loads of intrinsics which don't exist on amdgcn. Attempting to hack around.

Tried a bunch of workarounds:

  • Passing a valid include path by -I, -isystem, -cxx-isystem. All overridden by the environment variable
  • Pass -nostdinc++, no effect

It looks like the environment variable wins out over all other options, even when it's trying to use headers for an unrelated architecture that can never work.

So I think the options here are:

  • provide a cmake flag to disable the amdgcn runtime for environments that otherwise try to build it with a toolchain that can't work
  • persuade bug reporter to stop doing the include path override
  • add a flag to clang that tells it to ignore these environment variables

1 & 3 seem like the stronger options here. Cmake flag at D102229, don't know if I'll have time to patch clang to optionally ignore the environment variables.

Thanks for helping on this issue!

openmp/libomptarget/deviceRTLs/amdgcn/CMakeLists.txt
27

This line of code is problematic. If I build OpenMP standalone and set CMAKE_C_COMPILER to the right clang, and the right clang is not in $PATH, it still finds the one in $PATH.

vzakhari added inline comments.May 17 2021, 12:02 PM
openmp/libomptarget/deviceRTLs/amdgcn/CMakeLists.txt
27

I am not sure I understand the problem. These find_program invocations explicitly look the tools (including clang) in ${LLVM_TOOLS_BINARY_DIR}, which is supposed to be set up by find_package (which also sets LLVM_DIR the causes cmake to hit this clause). Is it the case that in your build you get valid LLVM_DIR and invalid LLVM_TOOLS_BINARY_DIR? Is it possible that your pre-installed LLVM package is corrupted?

Can you please insert libomptarget_say("Using: ${LLVM_DIR}; using ${LLVM_TOOLS_BINARY_DIR}") here and tell me what it is printing?

openmp/libomptarget/deviceRTLs/amdgcn/CMakeLists.txt
27

Okay. I see the problem here. This patch was trying to abandon out-of-tree build of libomptarget, which exactly what I'm using. So the root cause is, I have multiple version of LLVM installed in my system. find_package(LLVM) can return one of them, but not the one I want to use (I cannot control the system so I cannot remove those LLVMs in my system). As a result, LLVM_TOOLS_BINARY_DIR is set to the wrong place, therefore the clang detected is also wrong, no matter what compiler I set for CMake. So in order to let CMake pick up the one I want, LLVM_ROOT needs to be set when invoking CMake.

openmp/libomptarget/deviceRTLs/amdgcn/CMakeLists.txt
27

So actually, even with find_package(LLVM), CMake can still pick up a wrong version. That's why I guard it with a version in https://reviews.llvm.org/D102587.

vzakhari added inline comments.May 17 2021, 1:19 PM
openmp/libomptarget/deviceRTLs/amdgcn/CMakeLists.txt
27

Thank you for the explanation. Can you please clarify what you mean by "a wrong version"? Is there any code in libomptarget or plugins that requires some minimal LLVM version? In other words, what kind of a breakage you get in your build?

tianshilei1992 added inline comments.May 20 2021, 3:02 PM
openmp/libomptarget/deviceRTLs/amdgcn/CMakeLists.txt
27

libomptarget uses some LLVM functions that they were in a different form compared with current one. My system has LLVM 8. If there is no version constraint, LLVM 8 can be picked up, and its headers will be used for compilation. However, the function signatures have already changed, leading to a compilation error.

It looks like we are using the latest version of LLVM. From my perspective, it's fine to assume it is 12 for now. In the future, once those functions are changed, the pre check in test will fail, which requires the author to update OpenMP code as well. We can revise the version later.

vzakhari added inline comments.May 20 2021, 4:06 PM
openmp/libomptarget/deviceRTLs/amdgcn/CMakeLists.txt
27

Thank you for the explanation! Yes, it makes sense to assume that people using out-of-tree builds update their LLVM's to be able to build libomptarget, if they need it. Thank you for the changes in D102587.

kaz7 added a subscriber: kaz7.Wed, Jul 14, 10:05 AM

I've noticed this patch breaks libomptarget for VE. The VE is an accelarator, so we create and install clang/llvm for VE as a cross compiler. Then, we build libomptarget for VE using this cross compiler.

After this patch, cmake tries to build libomptarget.so for target architecture using libLLVMSupport.a for host architecture. Is ther any good way to solve this kind of conflicts? Thanks in advance.

Could you clarify - are you running libomptarget itself on the accelerator, as opposed to only the device runtime? If so I expect you can compile llvm support for the accelerator too

kaz7 added a comment.Wed, Jul 14, 7:17 PM

Thank you for the clarify. As far as I know, our build system for omptarget for VE generates libomptarget.so for VE. Maybe it's the source of problem. I'll discuss with people who implement it for VE about the possibility of separating omptarget itself and runtime. Thanks.