Thanks @jhuber6
I would love to hear @gkistanova opinion on the patch before I land it.

This appears to have broken the AMDGPU buildbot https://lab.llvm.org/staging/#/builders/247/builds/360

Thanks for moving this forward.

In D147756#4252032, @jhuber6 wrote:

In D147756#4251909, @jdoerfert wrote:

It sounds like this might break some invariants we have about plugins, devices, and images.

If this does not work, some alternative ideas:
What if we make the plugins detect that they have different architectures and they create one instance per arch?
Though, we could also force the plugin to choose. If device 0 works with in image and device 1 does not, we ignore 1 from now on.

Yes, the problem right now is that we assign device ID's based on the logical devices found, not whether or not they loaded an image. So to make this work we would need to initialize the devices in order of images we find them compatible for. So on a system offloading to gfx90a and gfx908, if we had a gfx908 image, we would find it matches gfx908 and assign that device to zero. This is definitely a break from the current logic where we simply initialize all the devices found.

Landed in https://github.com/llvm/llvm-zorg/commit/8027f9019719ab80a72a5842470b69b5729436bd

Address inline comment

Abandoning in favor of https://reviews.llvm.org/D148808

Hey Krzysztof,
I believe this broke the openmp offload buildbot https://lab.llvm.org/buildbot/#/builders/193/builds/30576

One environment variable, defaulting to 2 seconds timeout for active waiting, rename, const

• rebase the patch
• add documentation
• address reviewer comments

Thanks for all the comments, I think there is a bit of confusion due to my lack of documentation of the code.

In D148576#4277344, @jhuber6 wrote:

In D148576#4277340, @jplehr wrote:

Works for me on gfx90a

Can you verify whether or not __builtin_trap or the s_trap 2 invocation hangs on those boxes? I can't tell if it's just my install or a wider problem.

Works for me on gfx90a

Tested on AMDGPU (gfx90a)

-- OpenMP tools dir in libomptarget: /home/janplehr/git/trunk17.0/build/llvm-project/runtimes/runtimes-bins/openmp/runtime/src
-- LIBOMPTARGET: Building offloading runtime library libomptarget.
-- LIBOMPTARGET: Not building aarch64 offloading plugin: machine not found in the system.
-- LIBOMPTARGET: Building AMDGPU plugin linked against libhsa
-- LIBOMPTARGET: Not building CUDA offloading plugin: LIBOMPTARGET_BUILD_CUDA_PLUGIN is false
-- LIBOMPTARGET: Not building PPC64 offloading plugin: machine not found in the system.
-- LIBOMPTARGET: Not building PPC64le offloading plugin: machine not found in the system.
-- LIBOMPTARGET: Not building nec-aurora plugin: libveo or libveosinfo not found.
-- LIBOMPTARGET: Building x86_64 offloading plugin.
-- LIBOMPTARGET: Not building aarch64 NextGen offloading plugin: machine not found in the system.
-- LIBOMPTARGET: Building AMDGPU NextGen plugin linked against libhsa
-- LIBOMPTARGET: Not building CUDA NextGen offloading plugin: LIBOMPTARGET_BUILD_CUDA_PLUGIN is false
-- LIBOMPTARGET: Not building PPC64 NextGen offloading plugin: machine not found in the system.
-- LIBOMPTARGET: Not building PPC64le NextGen offloading plugin: machine not found in the system.
-- LIBOMPTARGET: Building x86_64 NextGen offloading plugin.

I'll get back to this soon, enjoyed vacation. ;)

In D140722#4219182, @nextsilicon-itay-bookstein wrote:

Minor fix to the clang/CodeGen/ppc64le-varargs-f128.c test.

@jdoerfert Does the PR CI run these, or are there build bots that cover the different target-offloading variants? Can I somehow check this prior to merging without having all hardware variants on-hand?

LGTM

LGTM

It seems that the automation does not correctly enable libc or openmp as runtimes when only given as depend_on_projects.
The CMake invocation that actually happened was

cmake -G Ninja ../llvm.src/llvm -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=ON '-DLLVM_LIT_ARGS=-vv --show-unsupported --show-xfail -j 32' -DCMAKE_INSTALL_PREFIX=/home/ompworker/bbot/openmp-offload-amdgpu-runtime-experimental/llvm.inst -DCMAKE_BUILD_TYPE=Release -DCLANG_DEFAULT_LINKER=lld '-DLLVM_TARGETS_TO_BUILD=X86;AMDGPU' -DLLVM_ENABLE_ASSERTIONS=ON -DCMAKE_C_COMPILER_LAUNCHER=ccache -DCMAKE_CXX_COMPILER_LAUNCHER=ccache -DLIBOMPTARGET_FOUND_AMDGPU_GPU=ON -DLLVM_LIBC_FULL_BUILD=ON -DLIBC_GPU_BUILD=ON -DLIBC_GPU_ARCHITECTURE=gfx906 -DLIBC_GPU_TEST_ARCHITECTURE=gfx906 '-DLLVM_ENABLE_PROJECTS=llvm;libc;clang;lld;compiler-rt;openmp'

So, it only enables those as projects. This leads to a compiler error as we actually need the Clang to build the runtimes.

Removed accidentally added code

Fix bug to corectly support the maximally supported dimensions as required by the spec.

Remvoed the enable_runtimes argument

Please let me know if this patch needs more revision after the changes I implemented, and thanks for all the help.

Addressed feedback: removed extra CMake flags to enable runtimes and projects manually. Instead rely on enable_runtimes and depends_on_projects lists from the buildbot factory.
The builtbot before specified the CMake flag to enable the openmp runtime manually (despite listing it in enable_runtimes), so I removed that completely.

In D146722#4228797, @gkistanova wrote:

Could you elaborate on why did you specify LLVM_ENABLE_PROJECTS explicitly, and why it does not match what's in the depends_on_projects, please? The same question is for LLVM_ENABLE_RUNTIMES and enable_runtimes respectively.

With this patch the openmp-offload-amdgpu-runtime-experimental builder would not listen for compiler_rt changes, but would build that. Meaning false blame lists every time when a compiler_rt commit breaks a build. This does not look right. What am I missing?

Rebased and enabled tests for generic devices.
Resulted in one test failure

Rebase to make ready for land

I wanted to do this today, my afternoon. This should give us some time to reasonably react if things don't go the way we want.

In D146849#4225134, @ye-luo wrote:

Unfortunately my original problem still persists with this patch. The active wait timed out and then still stuck in the blocking wait.
Tested 3, 30, 300, 3000, 30000, 300000, 3000000 on my W6800. I have to consider some software between hsa and the hardware is broken.

Rebase and review feedback

Rebase and ready to land

Added missed check-clang target to lit tests

Some initial comments

Thanks for moving this forward.

I compared it to the HIP implementation linked and from that point of view it looks reasonable to me, but I don't have a good understanding of the internals yet. @JonChesterfield can you comment on the topic?

Actually followed Johannes' suggestion.

Followed Johannes suggestion and rebased

Adds assert and assert messages.
It felt safer to assert exactly three elements to be consistent with the name of the lambda.

Updated test and removed two unused variables.

LGTM

Just to reiterate what I think this does is: It adds tests which currently ensure that the call backs can not be registered (as it should).
The tests would then be updated for the other patches, when these call backs can be registered, right?

Addresssed final nits

Reworked implementation to always print standard info.
Introduced optional and refactored test.

Addressed nits and rebased. Please see if the changes to the test case are acceptable.

Addressed more reviewer comments and added test

• Removed: curlies for single statement blocks, unncessary llvm:: qualification, leftover empty if, unneccessary using ELFT statements
• Added check if kernel name has associated metadata and changed return type to Expected<T> to allow for error propagation

In D144521#4147666, @jhuber6 wrote:

In D144521#4147647, @jplehr wrote:

Am I right to assume you mainly asking about the code that obtains the metadata for the different kernels in the image?
Without executing a OpenMP target offload program on an AMDGPU, I see mostly one option: Construct a msgpack from a textual representation (e.g., YAML) and query that for the relevant information. This should be somewhat straight forward to do in Google Test. But, it would not automatically react to changes in the metadata produced in the backend. For that, we probably would need to compile and run something on an AMDGPU to test the output.

Shouldn't this get populated automatically if you compile an OpenMP program? We don't need to check. We have an existing info.c test that you could copy and make AMD only.

Am I right to assume you mainly asking about the code that obtains the metadata for the different kernels in the image?
Without executing a OpenMP target offload program on an AMDGPU, I see mostly one option: Construct a msgpack from a textual representation (e.g., YAML) and query that for the relevant information. This should be somewhat straight forward to do in Google Test. But, it would not automatically react to changes in the metadata produced in the backend. For that, we probably would need to compile and run something on an AMDGPU to test the output.

Thanks Joseph.
My impression about the llvm-readobj was that they have quite some machinery around the printing. However, I believe I understood that the actual printing is mainly a conversion of the MsgPackDocument into a YAML and then print that. So I think that there is not too much in common.

Rewrite using what is available upstream to read the ELF metdata for AMDGPU.
Removes the copied-over code from the old plugin.

LGTM

Rebased

Rebased