In D104418#2825481, @tianshilei1992 wrote:

Correct, but in the body of the task, BlockA[0] and BlockB[0] are only read. It's valid to mark them as in only.

In D104418#2824738, @tianshilei1992 wrote:

In D104418#2823848, @JonChesterfield wrote:

So is the race this is trying to fix between the host and the target, or between regions running on the target?

Passing data back to the host suggests the former, but that doesn't make sense, because the async operations are queued up within an async object that is not shared between host threads (and has a lexically scoped lifetime, iirc).

If it's a race between different target regions on the device, we should be able to resolve that with no host involvement at all.

It's a data race between host and plugin. https://bugs.llvm.org/show_bug.cgi?id=49940 has detailed analysis. Long story short, let's say we have two concurrent target regions T1 and T2 both reading memory region M. Assume T1 is scheduled to execute first. It then maps M to the device. The mapping consists of several steps:
0. Get the lock of mapping table.

1. Look up into the mapping table to see if M is already mapped. - No.
2. Allocate device memory for M and create a mapping entry for M.
3. Issue data transfer.
4. Release the lock.
5. Do remaining things, such as more data transfer, or directly launch the kernel.

Everything seems fine. Now T2 starts to execute. It also maps M to the device. However, steps are slightly different.
0. Get the lock of mapping table.

1. Look up into the mapping table to see if M is already mapped. - Yes.
2. Since the mapping is already there, T2 will NOT issue data transfer. This can improve performance. It doesn't make sense to transfer same data for several times.
3. Do remaining things, like Step 5 above.

It also looks good for this logic. However, there is a data race. Since we now have async data movement, Step 3 for T1 can only guarantee that movement of M is issued, but it cannot guarantee that after Step 3, the data is already on the device. For T2, its logic is, if the mapping is already in the table, it "assumes" that the data is already on the device, and therefore it doesn't issue data transfer. And then, bang! For the case that T2 is scheduled to execute by the GPU scheduler ahead of the data movement of M, the memory region on the device contains nothing from host. It is random.

So in one sentence, the root cause is the misalignment between assumption of mapping lookup logic and non-atomic data movement operations. It is not a problem for targets not supporting async data movement because everything can be guarded by the lock. Also, for those targets supporting async data movement, we don't want to use synchronous data movement here as well. The reason is obvious, for better performance. Data movement is extremely expensive, and multiple data movements can be potentially parallel, even for one single target region.

What I'm trying to do in this patch is to establish the dependency between the data movement and all other tasks using that data. We want to utilize the device side synchronization to do that by using event. The interesting thing for the event is, from host's perspective, "wait for an event" means just insert the event to the queue, and that's it. It is not like a barrier because it will not block the host. But yes, it is a barrier for the device scheduler for sure that all enqueued operations following the event can only start execution if the event is fulfilled.

I must admit that I have no idea about the internals of libomptarget. But here is my high-level view on this issue:

In D104303#2819905, @jlpeyton wrote:

Were these seen from gcc warnings or a static analysis tool?

It seems like a change in this patch introduced a dependency issue for fresh builds.

Making DataPoolEntry non-virtual

I don't think, this needs ifdefs. As I understand, a compiler is supposed to ignore unknown pragmas.
I fully agree that the suppression will not be compatible with other compilers.

Applied a more recent version of clang-format

Renamed class as suggested. Also applied a more recent version of clang-format.

Implement @Hahnfeld 's suggested changes

In D103767#2801322, @ye-luo wrote:

"libarcher.so, which is automatically loaded if an OpenMP application is built with TSan." Is this valid with gcc + libomp combination?

I think, this reflects what we discussed in the OpenMP in LLVM meeting. Thanks for working on this!

LGTM. Thanks!

The initial fix broke in some cases. I missed the difference between the incomplete and allocated task counters.
This updated patch should fix the issue, but I still run into starvation issues. I'll attach another reproducer in the bug.

In D101509#2749382, @weiwang wrote:

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.

In D101882#2745448, @jlpeyton wrote:

Do we set any affinity for the hidden helper threads, or are they free floating?

The hidden helpers do get their affinity set as if they were normal worker threads.

e.g., with this patch and if KMP_AFFINITY=compact (and two hardware threads per core), then
regular gtid 0 is pinned to the first core
regular gtid 9 is pinned to the first core
regular gtid 10 is pinned to the second core
regular gtid 11 is pinned to the second core
regular gtid 12 is pinned to the third core
...
hidden helper gitd 1 is pinned to the first core
hidden helper gtid 2 is pinned to the second core
hidden helper gtid 3 is pinned to the second core
hidden helper gtid 4 is pinned to the third core
...
hidden helper gtid 8 is pinned to the fifth core

Is there a consensus on if we want them free-floating or not? I assume we do, but want to make sure.

Ok, let's see what people with different test systems report.

lgtm as well.

You can just use UNSUPPORTED.

I still have issues with this patch and I think, it's not the right solution. I wonder, why __kmp_bottom_half_finish_proxy is necessary at all - see the inline comment.
If it's necessary, I tend to change the "imaginary children" code to td_incomplete_child_tasks |= 0x4000000 and td_incomplete_child_tasks &= !0x4000000. Then wait while (td_incomplete_child_tasks & 0x4000000) != 0

I'm not convinced that disabling the tests is a good idea. While it might not be optimal that they don't trigger reliably, their failing shows that something is fundamentally broken in the runtime.

In D99656#2739517, @JonChesterfield wrote:

Where's the logic to skip running these tests if ./amdgpu-arch fails?

@ronlieb Thanks for testing! When I saw this patch, I thought that broken thread-binding might possibly cause the observed performance issue.

In D101509#2733769, @jdoerfert wrote:

In D101509#2725137, @protze.joachim wrote:

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.

Do we set any affinity for the hidden helper threads, or are they free floating?

In D96893#2735978, @AndreyChurbanov wrote:

I temporarily disabled assertion until the bug with dependences is fixed.

In D101498#2727812, @Meinersbur wrote:

In D101498#2724916, @protze.joachim wrote:

The logic to identify whether certain features are supported for testing is in openmp/cmake/OpenMPTesting.cmake.

The file does not contain target-specific features, but "Set up testing infrastructure" (According to the comment in the top-level CMakeLists.txt). At the point when it is invoked, libomptarget has not been processed yet, making it impossible to determine target-dependent features.

Thanks!
lgtm

https://reviews.llvm.org/D101498 will fix this by allowing to distinguish
platforms supporting unified shared memory from platforms that don't.
Do you think, that looking at the gpu generation is sufficient, or does the
host architecture matter as well?

In D101265#2724364, @tianshilei1992 wrote:

In D101265#2718317, @Meinersbur wrote:

In D101265#2715759, @tianshilei1992 wrote:

In D101265#2715749, @Meinersbur wrote:

In D101265#2715714, @tianshilei1992 wrote:

If OpenMP is built via LLVM_ENABLE_PROJECTS, it is *intentional* to build OpenMP w/o using the clang in recent build.

Is this documented somewhere?

You could refer to https://llvm.org/docs/CMake.html for LLVM_ENABLE_PROJECTS and https://llvm.org/docs/BuildingADistribution.html for LLVM_ENABLE_RUNTIMES.

The first link does not mention what is intended to build with what compiler.
The second link is for creating a distributable package, which does not apply here.

The two links show the difference between LLVM_ENABLE_RUNTIMES and LLVM_ENABLE_PROJECTS. It does mention that projects in LLVM_ENABLE_RUNTIMES are built by the recent built clang.

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.

The test tends to fail for x86_64 offloading. When I reduce the BS to 64, the test also fails for nvptx offloading.

Please update the test with a NFC commit.

The logic to identify whether certain features are supported for testing is in openmp/cmake/OpenMPTesting.cmake. Please add checks there to identify unified-shared-memory support.

It might help in the future, if you add comments for each XFAIL you add, saying what feature is missing to make the test case work (like you did with printf).
-> This would naturally result in a separate XFAIL line for each offloading target.

Rebased to master, fixing one conflict.

I rerun the tests for x86_64 and nvptx and removed the XFAIL for all tests succeeding in repeated tests.

On our site, the dependencies are not necessarily found in the same directory when moving from build system to compute system. So, for production use I actually prefer LD_LIBRARY_PATH & CO, which are managed by the environment-modules system.

In D99656#2716049, @pdhaliwal wrote:

With properly configured cmake and lit, it should be possible to add tests run separately for architectures. For e.g. it should be possible to create targets like check-libomptarget-amdgcn-amd-amdhsa which would run tests only for amdgcn and similar for others. These can be grouped under existing check-libomptarget.

I moved the RUN line changes into D101326, which updates all tests.

The majority of tests has 5 identical RUN lines, which just differ in the triple-suffix. These 5 identical lines fold into the generic RUN line like I showed for private_mappings.c
The code I added in lit.cfg effectively makes the generic RUN line an alias for the selected specific RUN line.
I'm not sure, what special handling for nvptx you have in mind.

In D99656#2661379, @JonChesterfield wrote:

I guess there's no XFAIL equivalent here? In that case we should probably leave off the RUN line for the tests that can't work, as they'll otherwise break the build once an amdgpu CI machine goes live

The latest changes fix the issue. I tested with x86_64 and nvidia offloading.

I tested the patch and still get wrong results (I modified the declare_mapper_nested_default_mappers_complex_structure.cpp test to use my verbose printf from bugzilla):

Modified Data Hierarchy:
Object C (0x623e50, 2) Contents:
        Object B (0x623e80, 2) Contents:
                Object A (0x6243f0) Contents:
                        data1 = 6439680  data2 = 0
                Object A (0x6243f8) Contents:
                        data1 = -784902216  data2 = 11062
        Object B (0x623e90, 2) Contents:
                Object A (0x6249e0) Contents:
                        data1 = 6441856  data2 = 0
                Object A (0x6249e8) Contents:
                        data1 = 6439904  data2 = 0
Object C (0x623e60, 2) Contents:
        Object B (0x623ef0, 2) Contents:
                Object A (0x624b90) Contents:
                        data1 = 6438736  data2 = 0
                Object A (0x624b98) Contents:
                        data1 = 6441344  data2 = 0
        Object B (0x623f00, 2) Contents:
                Object A (0x624c60) Contents:
                        data1 = 6444032  data2 = 0
                Object A (0x624c68) Contents:
                        data1 = 6441856  data2 = 0

LGTM. Thanks!

In D99353#2669046, @awarzynski wrote:

Btw, how important are these aliases for you?

Phab also only lets me abandon the review

Thank you for working on this! It works for me.
As you mentioned in D95460, this makes the behavior more similar to gcc.

I created https://bugs.llvm.org/show_bug.cgi?id=49723 to track this issue

LGTM after the latest update.

In D98838#2635538, @protze.joachim wrote:

Please add my reproducers as test cases.

__kmp_hidden_helper_initialize() always initializes all hidden threads at once. Right? In this case, you modifications make sense.

LIBOMP_NUM_HIDDEN_HELPER_THREADS=0 avoids the segfault/assertion for the two test cases I attached to the bugzilla issue. This kind of makes sense, as 0 hidden threads cannot create a hole in the __kmp_threads array.

I filed https://bugs.llvm.org/show_bug.cgi?id=49631 to make release managers aware that we have a problem here.

Hi Andrzej,

I think, the fundamental issue of this patch is, that it broke the implicit assumption, that entries in __kmp_threads are handed out contiguously. After spending quite some effort into trying to identify locations, where this implicit assumption is now broken, I think, much more effort is needed to identify all places which rely on this assumption and are now broken.