review and more questions

Consider adding a test.

Thanks for following up on this!

Looks great!

The noalias issue is one problem but I think we also need to model some kind of memref that is lowered to a bare pointer (e.g., if we want to pass a pointer to an external function). Creating specific dialects to cover each and every one of these special case doesn't scale well.

Hmm, this may not be as straightforward as it seems. The pointer in the unranked memref descriptor is to the ranked descriptor struct rather than to the data. Since we don't have two memory spaces in the type, we need to at least define a convention. One can be that the descriptor of the memref is allocated in the same memory space as the memref itself, which requires changes to memref_cast lowering and thinking about what it means for descriptors that live in registers. Another convention can be that the descriptor itself lives in the default memory space, and then this change isn't necessary, only making sure that memref_cast produces the llvm.bitcast to a descriptor struct with correct memory spaces inside.

I have sent an RFC to which this diff is in illustration. Please do not review diffs with work-in-progress / WIP in the title.

Thanks for the cleanup!

There are review comments that remain unaddressed on this. Please address or comment as to why you chose not to address them.

As discussed on the forum. Thanks!

By naming the patterns "properly" I am able to put them in the right position so that the Filecheck agrees.

As a general observation, do you folks actually want the bare-pointer convention, or is it only there to attach noalias argument attribute? Maybe we should consider investing into expressing aliasing information in MLIR rather than keep the maintenance burden.

This looks useful.

Thanks for iterating!

Do you want to update those in this patch or have me do them in a follow-up?

Also update Python bindings

Is my understanding correct? Or I missed somthing important?

You can lower scf.parallel into omp parallel for/do *if* you prove the body does not contain (certain) OpenMP runtime calls and directives. So without analysis you cannot.

Rebase

In D86071#2268408, @jdoerfert wrote:

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?

I'm not following discourse.

I'm fine with anything that is not a code review, which most people would just ignore

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.

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.

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.

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

I guess for the general workshare loop design issues we can have an RFC in discourse. But this patch can go ahead.