LGTM, one nit you might want to consider

commit message?

This works for me. @arichardson you want to continue the review or should we take this and if needed improve upon it in subsequent patches?

In D87841#2279743, @lebedev.ri wrote:

As openmp and llvm are now developed in the same monorepo, the only uses this
dependency can break are those that download the monorepo, then delete the llvm
directory before calling cmake. That is not a compelling use case.

That is factually not accurate.
There are standalone tarballs, including one for omp:
https://releases.llvm.org/download.html#10.0.1 OpenMP Source code (.sig)

LGTM. Thx for the many tests.

LGTM

We have quite a few "outlining" passes but most are module passes I suppose.
Assuming we want to work towards CGSCC passes, and maybe, maybe parallelize them at some point, we should allow proper updates of the CG.

In D87623#2275540, @asbirlea wrote:

I'd like to revisit this and open a discussion.

LGTM

tests?

LGTM

In D83004#2274573, @greened wrote:

In D83004#2168427, @jdoerfert wrote:

Due to D82995 I realized we should have a test of this in llvm/test/tools/UpdateTestChecks as well.

Do you mean a test for recognizing the option in common.py? I'm not sure what you're asking for here.

Some comments but I haven't gone through all of it. The tests are still missing.

In D87340#2271039, @kparzysz wrote:

This should be NFC for everything that doesn't involve masked loads/stores.

OK, I think we need to revisit assume but an otherwise empty loop with an assume doesn't seem to be particularly useful. We might want to hoist loop invariant assumes, but that can be revisited. I think this solves a problem and is reasonable until we have time and ideas.

Does it make sense to copy the unit test and keep the original one as well?

I forgot to submit the first part of this response a few days ago, apologies.

In D86071#2268185, @kiranchandramohan wrote:

Sure: http://lists.flang-compiler.org/pipermail/flang-dev_lists.flang-compiler.org/2019-May/000197.html

@jdoerfert This is probably the old flang-dev mailing list.

Is it OK for @jdoerfert and @ftynse If I initiate the discussion in discourse?

In D86071#2267754, @ftynse wrote:

In D86071#2267750, @jdoerfert wrote:

In D86071#2267473, @ftynse wrote:

In D86071#2266254, @jdoerfert wrote:

In D86071#2266169, @ftynse wrote:

It has to be the OpenMPIRBuilder that lowers it into a CFG (eventually) because it is not "a fortran/mlir/affine/... loop" but an OpenMP worksharing loop with all what that entails.

If there is an OpenMPIRBuilder::CreateForLoop or a plan to have it? So far, it looks a there is a non-negligible amount of code in Clang that emits the IR for loops, and replicating that code in mlir-translate is a no go.

In D86071#2266169, @ftynse wrote:

It has to be the OpenMPIRBuilder that lowers it into a CFG (eventually) because it is not "a fortran/mlir/affine/... loop" but an OpenMP worksharing loop with all what that entails.

If there is an OpenMPIRBuilder::CreateForLoop or a plan to have it?

Yes.

Is there a written version somewhere we can see?

A written version of the plan? Yes. A written version of the code, not yet.

Could you post a link?

In D86071#2267473, @ftynse wrote:

In D86071#2266254, @jdoerfert wrote:

In D86071#2266169, @ftynse wrote:

It has to be the OpenMPIRBuilder that lowers it into a CFG (eventually) because it is not "a fortran/mlir/affine/... loop" but an OpenMP worksharing loop with all what that entails.

If there is an OpenMPIRBuilder::CreateForLoop or a plan to have it? So far, it looks a there is a non-negligible amount of code in Clang that emits the IR for loops, and replicating that code in mlir-translate is a no go.

In D86071#2266169, @ftynse wrote:

It has to be the OpenMPIRBuilder that lowers it into a CFG (eventually) because it is not "a fortran/mlir/affine/... loop" but an OpenMP worksharing loop with all what that entails.

If there is an OpenMPIRBuilder::CreateForLoop or a plan to have it?

Yes.

Is there a written version somewhere we can see?

See comments below. High-level: Why don't we say FullPluginName = Name, then try to load, if that fails, get the CWD and prepend it to FullPluginName before trying again. So LoadLibFromCWD will become bool getOrCreateCWD(std::string &CachedCWD) { if (CachedCWD) return true; ... } and a lot of the logic should be simpler. Also consider moving the actual loading part DP("Loading library '%s'...\n", Name); void *dynlib_handle = dlopen(Name, RTLD_NOW); into a helper so you can just call it twice instead of duplicating logic there again.

In D86071#2266169, @ftynse wrote:

It has to be the OpenMPIRBuilder that lowers it into a CFG (eventually) because it is not "a fortran/mlir/affine/... loop" but an OpenMP worksharing loop with all what that entails.

If there is an OpenMPIRBuilder::CreateForLoop or a plan to have it? So far, it looks a there is a non-negligible amount of code in Clang that emits the IR for loops, and replicating that code in mlir-translate is a no go.

In D86071#2265964, @ftynse wrote:

What exactly do you expect to do with OpenMP worksharing loops on this level which is problematic with CFGs?

It's the inverse that is problematic: having loop ops where CFG is expected. I would like to avoid seeing something like

omp.do <...> {
  omp.for %i = <...> {
    llvm.store <...>
    // other LLVM operations here
  }
}
// LLVM operations as CFG here

go into mlir-translate, which will have to outline and lower the loop during _translation_ in this case.

In D86071#2265876, @ftynse wrote:

In particular, it is not clear to me how this construct will connect to loops and what the lowering flow is. Does it expect an scf.for/scf.parallel as the only nested op? Is there a plan for a separate omp.for? How long do loops persist when we go to LLVM, given that OpenMPIRBuilder does not handle loop constructs and we really want to avoid converting loops to CFG during MLIR->LLVM IR translation.

In D87149#2264766, @chandlerc wrote:

[...]

Given that, it seems very hard to argue that a volatile memory access must not trap, and must not unwind the stack -- signal handlers are allowed to do that, and it seems like a long held valid use case for volatile access to memory is to trigger a specific signal handler. Perhaps this use case just didn't come up in the prior discussions?

Rebase on top of D87304

In D87149#2264099, @chandlerc wrote:

Regarding access to volatile memory...

If volatile memory access is known to not trap, then the guidance on LLVM's own blog is wrong:
https://blog.llvm.org/posts/2011-05-13-what-every-c-programmer-should-know/

In D87149#2263777, @stephan.yichao.zhao wrote:

In D87149#2263692, @nikic wrote:

Although executing __builtin_unreachable is undefined, removing the code before it deletes their side effects.

I believe this behavior is correct. LangRef explicitly states that execution continues past a volatile store. As such, unreachable must be reached, which is undefined behavior. As such, we are free to optimize as we wish, including removing a preceding volatile store.

I think the problem is if that reaching unreachable instructions is undefined means the behavior when a program hits builtin_unreachable is undefined and the behavior before builtin_unreachable is hit is also undefined.

The behavior when a program hits __builtin_unreachable is definitely undefined, because a runtime can abort or hang or keep on going. The LangRef does not define it.

In D87149#2263725, @lebedev.ri wrote:

In D87149#2263708, @nikic wrote:

@lebedev.ri I don't think the printf() case is any different. Keep in mind that undefined behavior acts retroactively, so if we know that unreachable will be executed later, we are free to optimize away side-effects before it as well. (If printf is not considered willreturn, then we cannot optimize it away, and won't.)

IOW you are saying that in https://godbolt.org/z/TEo6oj tail call void @_Z5errorv() should ideally be optimized out?

In D87149#2263708, @nikic wrote:

@lebedev.ri I don't think the printf() case is any different. Keep in mind that undefined behavior acts retroactively, so if we know that unreachable will be executed later, we are free to optimize away side-effects before it as well. (If printf is not considered willreturn, then we cannot optimize it away, and won't.)

This broke the windows bot, I'm pushing the revert as soon as my tests come back.

LGTM

LGTM

LGTM

LGTM

@serge-sans-paille did update the .bc file, many thanks!

Split into D87304 and D87306

Can you pre-commit the test so we can see the changes. Make it an NFC change w/o review.

In D87165#2261026, @jhuber6 wrote:

Added the target kernel name and device ID to the message. Added a new method to get the offload entry given the pointer. I can't decide if it's better to just always print the device ID if we have it versus putting it in the macro itself.

ping

In D87260#2261006, @hamax97 wrote:

In D87260#2260098, @jdoerfert wrote:

Tests?

The regression test for this is hide_mem_transfer_latency.ll. But there's still the issue with the AAManager, so the test has the fixmes of where the set up instructions should be moved.

@eugenis accepted before and this looks OK to me.

Add a test case with multiple loops in the caller and callee please. also nested ones. And one with the callee loop not having the metadata so we see it won't after.
And a test with both caller and callee having the attribute so no metadata is added.

Tests?

@okura @uenoku Can we copy this logic into AAConstantRange too?

LGTM thx

In D86157#2258063, @fhahn wrote:

This looks fine, but might be better to split up into 2 patches (1 for undef, 1 for align)? Small nit for the title: this patch fixes AttributeFuncs::typeIncompatible, which as a consequence fixes DeadArgElim, but potentially other places too. Might be good to reflect that in the title.