I am not quite familiar with Clang driver set up, I will add Greg for more comments. But I have hacked one the latest YKT tree to support simple AMDGCN path the same way as NVPTX. The last patch is here

In D46842#1100723, @Hahnfeld wrote:

In D46842#1099392, @guansong wrote:

Can we double check this LLVM IR backward compatible thing? My understanding is that it is not that good, at least for the major versions.

In D46842#1099415, @guansong wrote:

I just want to comment this last point, my understanding is that it will not work even you try to inline older LLVM IR into a newer one.

I did just that: I built libomptarget-nvptx-bc with Clang 3.9.1 (which is actually the only released version that you can currently build with) and inlined the result with LLVM/Clang trunk which will become 7.0. It worked without problems for all simple test cases I tried. Do you have a scenario that fails for you?

In D46842#1103727, @gtbercea wrote:

I am not at all convinced. I think there are absolutely no good arguments against allowing the just built compiler to build the BCLIB (as a default, this is simply a default which can be overwritten at any time by the user). The drawbacks you mention have no detrimental impact on the quality of life of the compiler developer. Developers improving other parts of the compiler who do not care about OpenMP and device offloading can just disable the building of the BCLIB because it is optional after all. The BCLIB is something that is required for performance not correctness so it can be disabled at any time. Even when BCLIB building is turned on there is no perceptible overhead to it, compared to the time it takes to compile a cpp file somewhere in the source code, the BCLIB builds instantly.

Your argument that an older version is better is actually wrong. Using an older version to build the BCLIB is always a BAD idea. The reason why you want to build the BCLIB is so that the runtime functions actually get inlined. If you use an older compiler the functions will not be inlined which defeats the purpose of having the BCLIB in the first place. I stumbled again upon this problem today after updating to the latest changes.

I do not understand why you are opposing this, so far I haven't heard a single strong argument against this.

In D46842#1099393, @gtbercea wrote:

Can we double check this LLVM IR backward compatible thing? My understanding is that it is not that good, at least for the major versions.

In D44992#1098572, @gtbercea wrote:

Thanks for the response. Can you point me to the final solution for this or re-explain it? From the comments I'm not sure I can distill the solution you want.

Update as suggested.

Update name as suggested.

In D46210#1085335, @grokos wrote:

May I suggest that we use something like LIBOMPTARGET_DEVICE_RTL_DEBUG instead of LIBOMPDEVICE_DEBUG? I think this name is more descriptive of what the env var does.

In D46185#1081368, @Hahnfeld wrote:

In D46185#1081362, @guansong wrote:

In D46185#1081124, @Hahnfeld wrote:

Anyway, why do you want to support sm_30? Is there a particular hardware that you want to support? Why does AMD care?!?

I will double check with Greg on the way atomicCAS used in the code. For the last question here. We have sm_30 machine for testing. Besides, from my perspective, I care the overall implementation of openmp, particularly the offloading part, not necessarily amdgcn code only :)

Okay, then the question is: Will the implementation get better if it supports 6 years old hardware with yet another code path that only few people care about and that will get less testing? From intuition I doubt that because extra code for old hardware won't get much support and attention by definition.

In D46185#1081124, @Hahnfeld wrote:

In D46185#1081111, @grokos wrote:

I don't think this code does atomic max. Actually, I don't think it does anything at all. First off, old_value and elem are variables which do not change inside the while-loop. So the first loop condition is either always true or always false. Secondly, atomicCAS returns the value that existed at address Buffer (regardless of whether or not the CAS operation succeeded) and not a boolean result. Also, old_value must be updated before each CAS attempt because some other thread may have changed it.

A correct implementation would look somewhat like this:

uint64_t old_value = *Buffer;
uint64_t expected_old_value;
do {
  expected_old_value = old_value;
  old_value = atomicCAS((unsigned long long *) Buffer,
                        (unsigned long long) expected_old_value,
                        (unsigned long long) max(expected_old_value, elem));
} while (expected_old_value != old_value);

What this code does is: check that the value at Buffer is still what I expect it to be, i.e. no other thread has changed it, and replace it with max(old_value, elem).

Fully agree with George's analysis here. The proposed implementation may be tweaked to early exit if elem < *Buffer.

Anyway, why do you want to support sm_30? Is there a particular hardware that you want to support? Why does AMD care?!?

Use PRIu64 for chuck

With the proper position of include file, I can use PRId64 now.

In D45528#1072542, @guansong wrote:

I checked again on the format you suggested. To use it, you need #include <inttypes.h> For example the macro was defined like this in my version of the inttypes.h

# if __WORDSIZE == 64
#  define __PRI64_PREFIX        "l"
#  define __PRIPTR_PREFIX       "l"
# else
#  define __PRI64_PREFIX       "ll"
#  define __PRIPTR_PREFIX
# endif

# define PRId8 "d"
# define PRId16 "d"
# define PRId32 "d"
# define PRId64 __PRI64_PREFIX "d"

But the header causes a compilation issue for cuda clang. I am not sure how to solve that problem.

I checked again on the format you suggested. To use it, you need #include <inttypes.h> For example the macro was defined like this in my version of the inttypes.h

In D45528#1067671, @grokos wrote:

In D45528#1067615, @guansong wrote:

In D45528#1067520, @grokos wrote:

In D45528#1067487, @guansong wrote:

update as suggested (except PRId64)

Isn't PRId64 working?

I did not find any usage of it on the device side. Not confident on using it. We can remove the warning first, and improve the format later?

It's not giving a warning because in CUDA long long int happens to be 64 bits long, so you can use %lld to print an int64_t. This is not guaranteed to be true on every platform, therefore the portable way to print an int64_t is using the PRId64 macro. That's what we do in the base library.

In D45528#1067520, @grokos wrote:

In D45528#1067487, @guansong wrote:

update as suggested (except PRId64)

Isn't PRId64 working?

update as suggested (except PRId64)

Ok, let me abandon this one and think on this again.

update according to suggestion, thanks.

In D44992#1062015, @Hahnfeld wrote:

In D44992#1061982, @guansong wrote:

In D44992#1061827, @Hahnfeld wrote:

! In D44992#1061781, @guansong wrote:
I have revert the change, let's continue to discuss how to enable bc file path better.

You are still not subscribed to openmp-commits, so again no commit email! For reference, the commit is rL329576 (and you shouldn't quote git commit hashes, there is a script in LLVM to revert SVN changes from git-svn.)

I was trying to use my office email to subscribe the mailing list, it may not be approved yet. How can I check on that? Currently I only get openmp-commits message from my gmail.

For reverting, I revert the patch from git, and commit it use git svn dcommit. I guess you are referring the following comment in the log

This reverts commit 6849e31c36d712d97433bca9af39b7a09c8c1207.

generated during the process. What is the proper script to use?

There is utils/git-svn/git-svnrevert in the main LLVM repository.

In D44992#1061815, @guansong wrote:

In D44992#1061781, @guansong wrote:

I can see the advantage of dynamic testing. But to test everything dynamically maybe tedious. For bc file handling, I consider this is more of a project need to embed LLVM itself, using find_package is more suitable. http://llvm.org/docs/CMake.html#embedding

proper link: http://llvm.org/docs/CMake.html#embedding-llvm-in-your-project

We probably don't need to test all flags on their own, we could group the basic flags together. And I still don't get which variable you want to use from that CMake integration?

Unless the grouped flags get accepted or rejected together by the compiler, we won't know which flag caused the failure. For the moment, I intend to use LLVM_VERSION_MAJOR and LLVM_INSTALL_PREFIX/LLVM_BUILD_BINARY_DIR only. But I can see more if we further explore the bc files.

I'd group the current set of flags and add a second check for -fcuda-rdc. I'm saying this a third time: There should be no hardcoding of versions!

In D44992#1061827, @Hahnfeld wrote:

! In D44992#1061781, @guansong wrote:
I have revert the change, let's continue to discuss how to enable bc file path better.

You are still not subscribed to openmp-commits, so again no commit email! For reference, the commit is rL329576 (and you shouldn't quote git commit hashes, there is a script in LLVM to revert SVN changes from git-svn.)

In D44992#1061781, @guansong wrote:

I can see the advantage of dynamic testing. But to test everything dynamically maybe tedious. For bc file handling, I consider this is more of a project need to embed LLVM itself, using find_package is more suitable. http://llvm.org/docs/CMake.html#embedding

I can see the advantage of dynamic testing. But to test everything dynamically maybe tedious. For bc file handling, I consider this is more of a project need to embed LLVM itself, using find_package is more suitable. http://llvm.org/docs/CMake.html#embedding

In D44992#1060659, @Hahnfeld wrote:

In D44992#1058800, @guansong wrote:

Currently we only enable the bc file path when we specified on the cmake command line with these flags -DLIBOMPTARGET_NVPTX_CUDA_COMPILER and -DLIBOMPTARGET_NVPTX_SELECTED_BC_LINKER.

We already enable it as soon as the user says -DLIBOMPTARGET_NVPTX_ENABLE_BCLIB and we find the Clang compiler suitable. This change breaks the build with Clang 3.9.1 and I will revert it early next week to fix this regression.

I see. I was trying to set the bc file utilities using find_pacakge(llvm...) instead of using COMPILER_ID and common it to an upper level as we search things like elf and ffi.

That is fine. I don't think this conflicts with what you said?

This is only for compiler and linker used for building bclib. It is not the default build compiler.

In D44992#1058783, @Hahnfeld wrote:

In D44992#1058770, @guansong wrote:

In D44992#1058469, @Hahnfeld wrote:

In D44992#1058461, @guansong wrote:

I don't think this is a regression through, as it is broken right now unless one uses an older (less 4.0) clang.

... which is enough to satisfy the definition of a regression. Yes, Clang also regressed but that's no excuse to regress the runtime library. Please revert this change or let me know if you can't and I'll do it.

I can add this flag conditionally as you suggested, how about something like this? where we check llvm version?

# Set flags for Clang cuda compilation.  Only Clang is supported because there is
# no other compiler capable of generating bitcode from cuda sources.
if (LLVM_VERSION_MAJOR GREATER 6)
  set(CUDA_FLAGS
    -emit-llvm
    -fcuda-rdc
    -O1
    -Xclang -target-feature
    -Xclang +${CUDA_PTX_VERSION}
    --cuda-device-only
    -DOMPTARGET_NVPTX_TEST=0 -DOMPTARGET_NVPTX_DEBUG=0
    )
else()
  set(CUDA_FLAGS
    -emit-llvm
    -O1
    -Xclang -target-feature
    -Xclang +${CUDA_PTX_VERSION}
    --cuda-device-only
    -DOMPTARGET_NVPTX_TEST=0 -DOMPTARGET_NVPTX_DEBUG=0
    )
endif()

No, it needs to be a compiler check. LLVM_VERSION won't be set if building openmp standalone.

In D44992#1058469, @Hahnfeld wrote:

In D44992#1058461, @guansong wrote:

I don't think this is a regression through, as it is broken right now unless one uses an older (less 4.0) clang.

... which is enough to satisfy the definition of a regression. Yes, Clang also regressed but that's no excuse to regress the runtime library. Please revert this change or let me know if you can't and I'll do it.

In D44992#1057123, @Hahnfeld wrote:

In D44992#1057094, @gregrodgers wrote:

So , will the deviceRTLs/nvptx change? Instead of extern __shared__, what will it use for those data structures?

I don't question the flag, as I said libomptarget-nvptx was the main reason to implement that functionality. What's bad about this is adding this flag unconditionally, the build system needs to properly detect if the compiler supports it.

If you can't work on that right now, I hereby request this change to be reverted as it results in a build regression.

I see. We compared the change in (4.0?) and after, realized the extern shared is turned off. We reenabled (basically allow it to pass parsing) and everything is expected.

My search got the same conclusion as Greg did, I don't think -fcuda-rdc will change the IR code, except allowing multiple compilation units as specified in the options.td

this fix I had is similar to 751ae2ab8, which is exactly for this.

I mean using -DLIBOMPTARGET_NVPTX_ENABLE_BCLIB=1 on cmake command line, to build the bc libraries. Did you use that flag?

Here is a general comment, we should follow Alex's suggestion to make a generic GPU device and extend that to NVPTX and AMDGCN. My current idea is:

Here is a question, do we require that the alloca size to be compile time constant?

LGTM.

In D33227#775415, @grokos wrote:

In D33227#775337, @guansong wrote:

Just to share what I have locally. I have OFFLOAD_DEBUG=1 for the plugin side debug messages, and DEVICE_DEBUG=1 for the deviceRTL side messages.

So are you suggesting that each plugin has its own env variable to turn debug output on/off?

Just to share what I have locally. I have OFFLOAD_DEBUG=1 for the plugin side debug messages, and DEVICE_DEBUG=1 for the deviceRTL side messages.

Thanks, update as suggested.

As suggested, mv the dir to the empty dir down below

Use ElfNote instead of AMD_PT_NOTE as suggested.

add a test

Should we mention that the runtime can be built "in-tree" as well? This simplifies users experience to access OpenMP support.

I found the patch does not work with the latest trunk. Could you please rebase, or provide a version number the patch is based on?
BTW, I am using git mirror, it will be appropriated to show me how to locate the base. Are we using git log? I did not find any tags...