wrong selection :(

LG, thanks

LG, we need to backport this too. And we need to document that profiling multi-threaded applications needs this flag in the build.

LG, some suggestions for the final version.

In D95572#2527082, @vzakhari wrote:

In D95572#2527005, @grokos wrote:

I've tested the change and everything seems to work fine. So let's commit it. Thanks for the patch!

Do you mean I can ignore the pre-merge checks fail? I do not see how my changes could have affected libomp build.

In D95572#2527005, @grokos wrote:

I've tested the change and everything seems to work fine. So let's commit it. Thanks for the patch!

please add to the backport list

LG

This is not NFC but other than that reasonable. LG, @grokos ?

also need to be backported to 12

turn this build off by default if cuda is missing (what we used to have)

LG

In D95398#2525516, @ggeorgakoudis wrote:

In D95398#2525170, @JonChesterfield wrote:

This should probably be applied to the 12.0 release branch as well

@JonChesterfield I landed, is there anything else I should do?

we need to put this in 12 as well, I think we need to open a bug that blocks the 12 meta-bug and start adding these patches (or git hashes) to be included in the release branch.

Does O1 create TBAA? we might need -O3 -Xclang -disable-llvm-optzns or -Xclang -disable-O0-optnone if we can also remove the noinline that way.

In D95539#2525648, @lebedev.ri wrote:

This is already caught by -Wimplicit-float-conversion/-Wdouble-promotion though
https://godbolt.org/z/o5Pzh4

In D93727#2524396, @vzakhari wrote:

offloading/dynamic_module_load.c fails due to this commit in my environment with x86_64 offload. It fails because std::call_once cannot be used without libpthread, and libomptarget.rtl.x86_64.so is not linked with libpthread. Basically, gdb shows that include/c++/7.3.0/x86_64-linux-gnu/bits/gthr-default.h::__gthread_active_p() returns false, when std::call_once is called from libomptarget.rtl.x86_64.so. It does return true, though, when std::call_once is called from libomptarget.so, because libomptarget.so is linked with libpthread.

It looks like GCC11 will have a fix: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55394
For the time being, all plugins have to be linked with libpthread, since Debug.h is considered to be common code for all plugins.

Any disagreement?

vzakhari removed a reviewer: jdoerfert.

I may be doing something wrong, but D95468 did not help very much looking at these numbers it seems...

LG

LG

LG, with the nits I mentioned before.

A few nits but given the opt-in semantics now I don't see a reason not to include this plugin (as we included other plugins before).

LG, with the nit @grokos pointed out.

Known issues resolved, AMDGPU is not yet a supported target and hard to test right now. LG

LG

In D95371#2520747, @AndreyChurbanov wrote:

That's right. Regardless that the code is written in C++ the intention is to not have any dependency on C++ runtime. The libomp.so should work fine with C applications as well as with Fortran codes those often cannot link C++ runtime.

Right. With profile support we got C++ dependences. So if you disable the profile support you can remove the dependence. What if we set the linker language to CXX if the profile support is enabled? Is that a problem?

But if profile support is enabled by default and will got into LLVM release, then people with pure C codes of Fortran codes will be surprised to see their OpenMP application depends on C++ runtime.
E.g. if they provide Fortran redistributables with Fortran application, it won't work because of C++ runtime dependency.
Or somebody may setup flang without clang, or gfortran without gcc, and link with libomp.
Not sure it is immediate problem, but in length of time such things will happen.

And all exported from libomp.so symbols supposed to be C symbols.

In D95274#2520112, @kkwli0 wrote:

In D95274#2520002, @JonChesterfield wrote:

What version of cuda are you using? It might be useful to know when things changed.

I think we can dlopen cuda, and try to load cuMemcpyDtoHAsync_v2, and then try to load cuMemcpyDtoHAsync on failure. That would get us some degree of robustness to different symbol versions.

$ cat /usr/local/cuda/version.txt 
CUDA Version 10.2.89

LGTM, thanks

Yeah, I agree that mustprogress, argmemonly and nosync should imply willreturn. However, FuncAttrs currently infers neither nosync nor argmemonly, so it would need those first. I think that the argmemonly check here was there to cover some common intrinsics like llvm.memcpy, but those are explicitly willreturn now.

In D95274#2520002, @JonChesterfield wrote:

We can unblock you by going back to the non-suffixed versions in rtl.cpp and adding the macros back to dynamic_cuda/cuda.h. That will mean the following combinations work:

In D95282#2518314, @tianshilei1992 wrote:

Seems like this patch introduces many unrelated changes.

In D95318#2518566, @AndreyChurbanov wrote:

I am afraid such action may be needed regularly, unless we freeze the clang-format version and write it in the documentation.
Because each version of clang-format may produce different result of formatting, unfortunately.
At least that was the case couple of years ago.

Remove leftover

More profiling and selective at to avoid too coarse grained scopes

In D93040#2517758, @xbolva00 wrote:

http://llvm-compile-time-tracker.com/compare.php?from=344afa853fcfcc085cb5c957b4a07c7ea013bb1b&to=2b9a834c43cb1f93d33958c14b695896bb4e9c1e&stat=size-text

Codesize regression 1% for tramp3d. Can you check it?

Logic seems sounds to me. Code as well, one minor nit below. I have no objections to this.

LG

LG

Still not perfect but getting there:

LGTM. Thanks for following up on this!

Switch the commit message with the subject though. LGTM

LGTM

Can we add something along the lines of the commit message for D95260

Calls that aren't willreturn should not be eliminated, as this may remove an infinite loop.

One nit wrt. _v2 not _v2, otherwise LGTM.

Pretty much good to go. CMAKE variable, one potential change in the comment below, maybe name the declarations _v2.

Looks reasonable to me. Test coverage is nice now, thanks.

LGTM. Maybe wait a bit if someone else wants to take a look.

+1 on the motivation. When we landed the other patch we didn't consider all the implications and, so far, they clearly outweigh the benefits of having partial undef offsets "work".

Interesting. Should we do this instead of the pure macro stuff? Is this going to break if the argument type doesn't match the declaration parameter type, e.g.,

void f(int);

In D95122#2512747, @aqjune wrote:

Since this issue only happens when the undef value is involved, another solution is to revert D87994 as suggested & make load/store's pointer noundef & rely on the future that the undef value will be finally removed.
I don't think this will cause a problem in practice as well even if undef persists; memory sanitizer is already catching the case.

LGTM one nit

In D95122#2512575, @aqjune wrote:

In D95122#2512404, @jdoerfert wrote:

LGTM. FWIW, nonnull should imply noundef because if it was undef we could pick null.

I did not look into the changes in this patch yet, but does it mean that from

f(i8* nonnull %p)

it is allowed to attach noundef?

f(i8* nonnull noundef %p)

I think this shouldn't be allowed, because the former one isn't UB when %p was null but the latter one raises UB.

What is left after we merged the loop unswitch solution?

LGTM. FWIW, nonnull should imply noundef because if it was undef we could pick null.

Given the langref text I'm unsure what it means to link an append linkage variable to a non-append linkage one. I can see how extern is different but what would happen before & now if you remove the extern. And what happens before/now if you have append on both @var? We should add those tests if possible.

In D94745#2511188, @JonChesterfield wrote:

Yeah... That's not gone well. Unless @jdoerfert can point us at a better way to spell these in openmp, let's add 'fix target atomics' to the todo pile and use clang intrinsics for the time being.

LGTM

Look for CUDA functions on demand and once only, inform libomptarget if CUDA was not found.

Load the cuda functions at runtime

Looks OK to me, @Meinersbur can accept at some point.

LGTM

LGTM, one comment below though.

In D95062#2510856, @tianshilei1992 wrote:

None of the three atomic operations provided by OpenMP, read, write, and update, sound like similar semantics as the four operations here.

LGTM

In D76127#2510562, @craig.topper wrote:

In D76127#2510504, @jdoerfert wrote:

TBH, I feel "X is readonly and has side effects" sends the wrong message to begin with. It is a contradiction (in the IR world) as basically shown by the need for this patch. Given that there are no examples in-tree I don't understand why one would mark a side-effect intrinsic as readonly (or similar). Long story short, I would argue this should be a loud error, not silently ignored.

Why shouldn't the intrinsics file be able to express the semantics we want even if we can't represent it in IR today?

In D95062#2510574, @JonChesterfield wrote:

In D95062#2510535, @jdoerfert wrote:

LG, can you replace it with clang or OpenMP atomics next?

Yep, expect so. OpenMP probably expects the variables to have atomic types, C++-style, and they're presently a mix of unadorned and volatile.

LG, can you replace it with clang or OpenMP atomics next?

LGTM

TBH, I feel "X is readonly and has side effects" sends the wrong message to begin with. It is a contradiction (in the IR world) as basically shown by the need for this patch. Given that there are no examples in-tree I don't understand why one would mark a side-effect intrinsic as readonly (or similar). Long story short, I would argue this should be a load error, not silently ignored.

In D93525#2509836, @t-tye wrote:

In D93525#2509796, @jdoerfert wrote:

At the moment this patch defines compatibility as exact string match of bundler entry ID.
[...]
Supporting target ID requires little more work and discussion.

Let's get this in first, then revisit target ID support as we need it.

I do not think this patch should ignore target ID as that is now upstreamed and documented. What is involved in correcting the compatibility test to be correct by the target ID rules? There are examples of doing this in all the runtimes and I can help if that is useful.

At the moment this patch defines compatibility as exact string match of bundler entry ID.
[...]
Supporting target ID requires little more work and discussion.

Let's split this. Declare target in all the source files can go first, doesn't hurt anyone.
Forward declarations should also work fine with existing compilation, so that would be two.
Conditional defines and a SHARED(...) macro would be three.
Four is the cmake magic. Potentially a different folder to build it as C++ + OpenMP alternatively.

LGTM, I think this is correct. Thanks a lot!

We should replace the OMP_NUM_TEAMS handling in the plugins with this, right? Separate patch or can we do it right away?