Confirmed that this patch fixes https://github.com/llvm/llvm-project/issues/56251

LGTM. This allows me to write concise compile lines.

Use -O3 in DeviceRTL static library build.

In D128130#3594408, @jhuber6 wrote:

So this changes the compilation to just use the Clang binary found in the install directory and hope it works like we did previously.

We don't do "hope it works" but we are sure it works and I don't think we found undesired clang. If undesired clang gets picked up. Please report issues.
Actually in the case of ENABLE_PROJECTS=openmp, there is no clang binary searching but directly referencing the clang cmake target. The just-built clang is guaranteed to be picked up.
In the case of ENABLE_RUNTIMES=openmp, LLVM_DIR is passed in, I would consider it closer to "hope it works". Even relying on CMAKE_CXX_COMPILER in runtime builds, you actually hope the RUNTIMES setup passes in the desired compiler, it is just a different scripting magic managed by ENABLE_RUNTIMES.

Restored gcc build in my tests.

In D125698#3516252, @tianshilei1992 wrote:

In D125698#3516250, @jhuber6 wrote:

In D125698#3516229, @ye-luo wrote:

In D125698#3516202, @tianshilei1992 wrote:

LGTM.

We are using CMake's command to build the device runtime now. There is no way to tell CMake to use another compiler rather than CMake's compilers to build a specific target. As a result, we either disable it, or we are doomed.
Well, accurately speaking, there seems to be a way to do that, see https://stackoverflow.com/questions/27168094/cmake-how-to-change-compiler-for-individual-target, but I don't think manipulating CMake's internal variables is a good practice.

I didn't say to workaround CMake. I was saying it was working that clang builds DeviceRTL and gcc building all the rest and now it is broken. You can try it out yourself with the commit before D125315

I'll just guard the new code then. Even though it shouldn't break if you're using the standard builds it will probably make it less noisy for people currently using the other methods.

Eventually we will abandon the old method, no?

In D125698#3516202, @tianshilei1992 wrote:

LGTM.

We are using CMake's command to build the device runtime now. There is no way to tell CMake to use another compiler rather than CMake's compilers to build a specific target. As a result, we either disable it, or we are doomed.
Well, accurately speaking, there seems to be a way to do that, see https://stackoverflow.com/questions/27168094/cmake-how-to-change-compiler-for-individual-target, but I don't think manipulating CMake's internal variables is a good practice.

This is either done with a two-step build, where OpenMP is built with
the Clang that was just installed, or through the
-DLLLVM_ENABLE_RUNTIMES=openmp option. This has always been the case,

This is not true. Even before D125315 breakage, all the host libraries can be built with GCC and DeviceRTL with just-built Clang via -DLLVM_ENABLE_PROJECTS=openmp
Maybe we should figure out why D125315 doesn't pick up clang when building DeviceRTL.

This is different from what I'm looking for. DeviceRTL should still be built as it was. Only the part added by D125315 should be disabled.

@qiongsiwu1 Tested the updated patch. Works fine now.

Got some trouble. See https://github.com/llvm/llvm-project/issues/55002

The debian failure seems unrelated. Let me know if you need me to commit the patch.

Expand the test.

check_include_file is like writing an empty main with one include line and gets it compiled. So it doesn't pull any dependency.
To test whether hwloc.h is a valid header file, you'd need to provide CMAKE_REQUIRED_INCLUDES pointing to hwloc.h.
So it is needed.

One minor change requested. All the rest LGTM.

I'm afraid you are abusing existing mutex. Please be specific about what part of the Entry you are trying to protect with TPR lock/unlock.

The new scheme stores the shadow pointers inside the entries.

Makes a lot more sense now.

Two tests array_section_use_device_ptr.c and array_section_implicit_capture.c are ported from https://reviews.llvm.org/D117997

using the multiarch directory

If we can cross compile libomp and libomptarget to the target system. We may have
lib/x86_64-unknown-linux-gnu/libomp.so
lib/aarch64-unknown-linux-gnu/libomp.so
Compile clang once but compile runtime library for multiple architectures.

Change title
threat state
to
thread state

I was using gfx902 in the past but after installing ROCm 5.0.0 today, I got a complaint and I have to use gfx90c by copying gfx902 bc files to gfx90c.
However it is better to do the right thing here.

ROCm 5.0.0 requires me to use this target on my 4700u

In D116908#3262918, @jdoerfert wrote:

In D116908#3262836, @ye-luo wrote:

It sounds like such info will be passed from host to the device once per kernel and the performance impact is negligible. Right?

The information is directly baked into the device image (in form of globals).
The transfer happens as you load the image, basically a few extra bytes per kernel but we had most of the things before as separate globals already anyway.
There is no kernel start cost to speak of, at least I don't expect any.

It sounds like such info will be passed from host to the device once per kernel and the performance impact is negligible. Right?

Please update the allowed string values and also resolve conflict before landing.

Much appreciated. Allowing to debug the host code is already a huge plus.

Confirm that #52938 is fixed by this patch.

In D104418#3221205, @tianshilei1992 wrote:

add clear comments on how Event should be used in current situation.

Renaming Event to EventH2D and also the corresponding functions are my last request. The rest looks good.

In D104418#3221061, @tianshilei1992 wrote:

In D104418#3220189, @ye-luo wrote:

I still think the new/old event exchange should be removed.
If T1 record an event for a D2H transfer and then T2 issues a H2D transfer and create an new event and then wait for the old event. the H2D or D2H may happen together when they are on two distinct streams.
This violates atomic data transfer behavior.
the solution is 1 persistent event per map and always check its status before issuing any transfer.

First, there is no event for D2H for now. This patch tries to make sure there is no race among H2D of the same memory from multiple target tasks, especially when they all read from that memory. D2H is out of the scope, and has nothing to do with the scenario mentioned in this patch. Without this patch, H2D and D2H can still happen at the same time on different streams.
Second, I think the case you describe is already data race caused by *user*. For two target tasks, if one task tries to move data in one direction while another task tries to move data in another direction, the two tasks should not run at the same time. That said, they should have dependency because one tries to read and another tries to write. Even with the "atomic" behavior you mentioned, which I didn't find it on the spec (maybe I missed it. could you pin point where it is?) it is still undetermined that which one comes first.
Third, using one event all the time sounds like a feasible solution. If an entry already has an event, we can reuse that one w/o the need to create a new one and destroy the old one, which can potentially save some overhead.

I still think the new/old event exchange should be removed.
If T1 record an event for a D2H transfer and then T2 issues a H2D transfer and create an new event and then wait for the old event. the H2D or D2H may happen together when they are on two distinct streams.
This violates atomic data transfer behavior.
the solution is 1 persistent event per map and always check its status before issuing any transfer.

In D101109#3213634, @ABataev wrote:

In D101109#3213510, @ye-luo wrote:

This patch caused many test failure in my application on Power9. Although this patch sounds like affecting SLP, adding -fno-slp-vectorize doesn't improve the pass rate but changing -O3 to -O0 does.

Hi, do you have a reproducer?

This patch caused many test failure in my application on Power9. Although this patch sounds like affecting SLP, adding -fno-slp-vectorize doesn't improve the pass rate but changing -O3 to -O0 does.

LGTM

I never liked init-if-passed-nullptr and prefer this explicit approach.