Go higher for GPUs, 128 or so. And we really need a metadata check to avoid looking for kernels.

Rebase + address comment

Fix tests

address reviews

Improve according to feedback

LGTM, some nits. Thanks for fixing this up so quickly.

Rebase

In D102107#2821976, @ABataev wrote:

We used this kind of codegen initially but later found out that it causes a large overhead when gathering pointers into a record. What about hybrid scheme where the first args are passed as arguments and others (if any) are gathered into a record?

Introduc a helper

ping

ping

ping

Address review

I don't understand why we have an event in the async_info object at all. I would have assumed only to have some plugin specific "lock" in the table.

Can you add a test case with a callback and one with an unknown function pointer call. Maybe also one with a function pointer and callee metadata + a FIXME.

More comments, also, please fix the clang tidy warnings, they make it hard to read.

(answered now, I forgot to submit this yesterday -.- )

This makes sense to me, wording seems to be alright, though it's late. Thx for working on this!

LG

I thought the problem was that a block with an assume caused us to miss out on some backed optimization.
The proposed solution was to eliminate blocks that only contain an assume as side effect early as middle-end
optimizations would also benefit from a simplified CFG.

LG, thanks

ping?

In D103862#2812033, @kuter wrote:

The code for the AAPotentialValues and AAValueSimplify is the same. Maybe we could move this to a separate function with a callback for doing the unions ?

Can you please reupload after clang formatting the code :)

// Loads/stores with an invariant.group metadata are ok to hoist/sink.

FWIW, if someone adds documentation what #pragma clang loop vectorize(assume_safety) actually means (somewhere), we can go back and make it imply if-conversion. However, we should not conflate it with the access groups/parallel metadata. Instead, annotate loads as "speculatable" (or sth. similar) and we go with that.

This fixes a bug and people are in favor. If we need to add a new mechanism for this we can afterwards still. LGTM

This starts to sound an awful lot like convergent to me, basically: Do not change the control conditions of this call.
Still unsure, maybe you can add a LangRef draft so we know what you try to do, or a nice example what you don't want to happen.

Please bear with me, I'm updating examples and documentation. I didn't think anybody would look at this while [WIP]. :-)

In D103958#2808767, @nickdesaulniers wrote:

The first talk from https://www.youtube.com/watch?v=FFjV9f_Ub9o (https://github.com/ClangBuiltLinux/plumbers-2020-slides/blob/master/LPC_2020_--_Dependency_ordering.pdf) might be helpful to link to at some point from the commit message, for a little additional context.

LG, two nits

WIP here is fine, would help to include a test that shows/explains what problem is actually solved though. I don't understand form this patch alone.

Thanks for splitting this. I quickly went over it only.

LG, one nit. Once the nit is resolved I can commit this for you.

LG, one nit

squash the commits so that we see the diff not from the last change but the entire change set

Put in a little more to reduce the follow up size

Does this by any chance also fix https://bugs.llvm.org/show_bug.cgi?id=49650 ?

In D103316#2802200, @markus wrote:

We provide a function say bool isBlockEmptyExceptAssumes(BasicBlock *BB) and we update the decision making of all the existing CFG transformations to use that. We provide another function say void hoistAssumesFromBlockKnownToBeOtherwiseEmpty(BasicBlock *BB) that all those CFG transformations need to call during their transformation if they used that fact that any of the blocks checked out as true with the former function.

Now maybe there aren't that many CFG transformations where this is relevant and maybe it integrates easily, I don't know, but it does seem like a somewhat larger change.

Some minor thing but overall this looks sensible.

In D103316#2800110, @reames wrote:

In D103316#2787008, @jdoerfert wrote:

In D103316#2786946, @nikic wrote:

This kind of transformation should probably eventually reside in lib/Transforms/Utils/SimplifyCFG.cpp but for now it seemed easier and a smaller change to just leave it in CodeGenPrepare.

I don't think this transform makes a lot of sense in CGP, I'd prefer it to go directly to SimplifyCFG (after which we can drop the CGP code).

I agree.

I think we would be better off to just drop these assumes. The framing I'm thinking of here is that SimplifyCFG should generally be ignoring ephemeral values in transforms. So if normally SimplifyCFG would drop a block that contains no instructions, then it should also drop a block that only contains ephemeral values.

I disagree.

if (x)
  assume(y)

is a natural way to spell something for the user.
We should not just drop it without having a good reason.
While we might not use assume(x && y) directly, we will use it if we specialize x to true in some places.

Er, I think you misread the comment you were replying to. The comment was discussing assume(A || B), your response is discussing assume(A && B). Given the context of the discussion, that difference seems important.

My own opinion is closer to that of @nikic. I think we should not be letting assumes block transforms. We should attempt to salvage assume information where we can, but only on a best effort basis. I do think we should generate the assume(A || B) form if that's the logical salvage even if nothing currently can infer from that.

LG, one nit below.

LG, one nit

LG, one nit

Do we have to update OpenMPIRBuilder::createCancel too?

LG

I'd change the default and pass a non-default value in getAAFor

In D102883#2791123, @JonChesterfield wrote:

Does anyone have a reference for what we're supposed to do with one of these things?

Amdgpu openmp isn't implemented in terms of any of those (it is a layer on HSA), but it's possible we can cobble together enough of a HIP or opencl 'instance' on the fly to satisfy the interface. I can't find any information on what that interface is meant to be though.

In D99202#2787617, @mnadeem wrote:

The following two tests are causing assertion failures when run with the legacy pass manager (force using -enable-new-pm=0).

• Transforms/OpenMP/hide_mem_transfer_latency.ll
• Transforms/OpenMP/values_in_offload_arrays.ll

Waiting on the bugzilla account request to make a ticket but here is the trace.

opt: llvm/lib/Analysis/CallGraphSCCPass.cpp:281: bool (anonymous namespace)::CGPassManager::RefreshCallGraph(const llvm::CallGraphSCC &, llvm::CallGraph &, bool): Assertion `!CheckingMode && "CallGraphSCCPass did not update the CallGraph correctly!"' failed.
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.
Stack dump:
0.      Program arguments: opt -S -openmp-opt-cgscc -aa-pipeline=basic-aa -openmp-hide-memory-transfer-latency -enable-new-pm=0
1.      Running pass 'CallGraph Pass Manager' on module '<stdin>'.
 #0 0x00000000020bdf43 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (opt+0x20bdf43)
 #1 0x00000000020bbc00 llvm::sys::RunSignalHandlers() (opt+0x20bbc00)
 #2 0x00000000020be3f6 SignalHandler(int) llvm/lib/Support/Signals.cpp:0:0
 #3 0x00007f15159bf330 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x10330)
 #4 0x00007f15143a3c37 raise /build/eglibc-ripdx6/eglibc-2.19/signal/../nptl/sysdeps/unix/sysv/linux/raise.c:56:0
 #5 0x00007f15143a7028 abort /build/eglibc-ripdx6/eglibc-2.19/stdlib/abort.c:91:0
 #6 0x00007f151439cbf6 __assert_fail_base /build/eglibc-ripdx6/eglibc-2.19/assert/assert.c:92:0
 #7 0x00007f151439cca2 (/lib/x86_64-linux-gnu/libc.so.6+0x2fca2)
 #8 0x000000000100ab4f (anonymous namespace)::CGPassManager::RefreshCallGraph(llvm::CallGraphSCC const&, llvm::CallGraph&, bool) llvm/lib/Analysis/CallGraphSCCPass.cpp:0:0
 #9 0x00000000010093ac (anonymous namespace)::CGPassManager::runOnModule(llvm::Module&) llvm/lib/Analysis/CallGraphSCCPass.cpp:0:0
#10 0x00000000018c6788 llvm::legacy::PassManagerImpl::run(llvm::Module&) (opt+0x18c6788)
#11 0x00000000007d06e5 main (opt+0x7d06e5)
#12 0x00007f151438ef45 __libc_start_main /build/eglibc-ripdx6/eglibc-2.19/csu/libc-start.c:321:0
#13 0x00000000007ba679 _start (opt+0x7ba679)
[1]    27229 abort      opt -S -openmp-opt-cgscc -aa-pipeline=basic-aa   -enable-new-pm=0

In D103316#2786946, @nikic wrote:

This kind of transformation should probably eventually reside in lib/Transforms/Utils/SimplifyCFG.cpp but for now it seemed easier and a smaller change to just leave it in CodeGenPrepare.

I don't think this transform makes a lot of sense in CGP, I'd prefer it to go directly to SimplifyCFG (after which we can drop the CGP code).

In D96418#2783541, @leonardchan wrote:

Well, no, I'm afraid it is actually clear that that code does have UB according to the C++ standard. Perhaps you mean that it *shouldn't* have UB, or that Clang should define its behavior despite the standard.

I might agree with you that I don't see the value in using this stronger rule in C++, but I think it would help to understand the problem a little better. I assume this is causing problems for a less trivial test case? Or do you really have code that's relying on that loop not terminating?

I see. I guess it's good (and bad) that this discovered this UB in our code base. The example I posted is almost exactly what's in our test case. For this particular one, we need a non-terminating thread, so a thread_create function is passed do_nothing. Locally, we could get around this by using something like inline assembly to avoid UB.

We need a test. A callback call should do it.

This allows us to remove the switch in the device runtime, right?

Yep, thanks to both of you to solving the problem with general solutions, will help in the long run for sure :)

In D102726#2774657, @nikic wrote:

FYI, this change causes a non-trivial compile-time regression in LTO builds: https://llvm-compile-time-tracker.com/compare.php?from=136ced498ba84f6b6126051626e319f18ba740f5&to=5b6cae5524905bc43cfc21a515f828528d1f2e68&stat=instructions (Worst regression seems to be the ThinLTO kc.link step, which is up 4%.)

LLVM attributes are optimized for immutable usage, and it's hard to overstate just how slow the mutable APIs on them are...

I think this is generally reasonable, haven't looked at all benchmarks and I would prefer some other opinions.

I looked over this again because I think we will later move the logic into an AbstractAttribute. The reason is that we want to reuse some of the analysis in AAKernelInfo to determine if we can use SPMD mode. Basically, if the shared memory is only used in a nice way and fees into an actual parallel51, it is compatible with SPMD mode. If we go to SPMD mode we have (or better should) eliminate the shared memory indirection. Maybe we move all of this into AAHeapToStack and make a more general AAAllocationInfo out of it. Let's see.

[Wrong review -.- ]

In D101973#2770283, @JonChesterfield wrote:

I can't see a change to a script in this diff. Should there be one? Looks like the test change was done programmatically

Addressed comments
Also moved logic from the AAKernelInfoFunction::updateImpl to
AAKernelInfoCallSite::initialize as it looks cleaner to fix the
state of runtime calls right away.

LGTM, two minor nits.

Drop the weak attribute, will solve the problem differently

This doesn't work properly yet :(
When I run this on the OpenMP tests I get sometimes a total order where there should be none.

Accidentally deleted too many check lines this time ...

Nit: Commit subject (not message) is old, maybe change prior to push locally

Make the script remove leftover CHECK: lines that would now trigger given
the absence of a specific check prefix for the -fopenmp-simd invocations.

LGTM, thanks for fixing this now and for the future!

Diagnostic Info parts seems very mechanical and ok to me.
OpenMPOpt parts make sense.
Test seem to be always tied to an analysis (e.g., D29023) so LGTM