[PATCH] D86071: [MLIR][OpenMP] Add omp.do operation

Tue Sep 15 09:15:23 PDT 2020

kiranchandramohan added a comment.

>> The question I have for @jdoerfert is which one of these will be preferable for the OpenMP IRBuilder? And the question I have for @ftynse is what is the issue that is there in these schemes? 
>> First, the loop belongs to the omp.do. If you want to lower an omp.do (w/ or w/o the OpenMPIRBuilder) you need the loop information (=bounds + step). The loop body is irrelevant at this point. The interface will somewhat look like this: InsertPos CreateWorksharingLoop(..., LowerBound, UpperBound, Step, ..., BodyCodeGenCallback)

Thanks @jdoerfert. I am thinking that there is some additional requirement since (for.eg. for a static schedule) the kmpc_static_init call has to be inserted in the loop header, the kmpc_static_fini has to be inserted in the loop footer.

>> The question I have for @jdoerfert is which one of these will be preferable for the OpenMP IRBuilder? And the question I have for @ftynse is what is the issue that is there in these schemes?
>
> If `scf.for` persists until the point where we perform the MLIR-to-LLVM-IR //translation//, the translation will have to do (the equivalent of) scf-to-std and std-to-llvm passes. which contradicts the "progressive lowering" principle of MLIR. (Note that we originally had a translation from the standard dialect directly to LLVM IR, but we introduced the LLVM dialect specifically to avoid that translation growing in complexity). If there is no explicit loop, the translator will have to analyze the CFG and essentially raise back to the loop form to be able to call the OpenMP IRBuilder, which would expect loop bounds.
>
> To actually make the loops persist, we would need some fine-grained control over the SCF-to-std lowering that would ignore loops contained in `omp.do` somehow, but only the outermost (?). The entire layering of scf-to-std lowering depending on the OpenMP dialect is not clear to me. There are also hard edges on dialect mixture due to type system differences: SCF does not work on LLVM types and LLVM operations don't work on standard types. We'd need cast operations, type conversions and canonicalizations that remove redundant back-and-forth cast chains, all in translation, which sounds messy and poorly maintainable.

The broad plan is to always to get to OpenMP operation with LLVM dialect. So the plan is not for scf to persist until MLIR to LLVM IR translation. The plan is to have a conversion pattern (or does this fall under transformations?) which converts an scf.for nested inside an omp.do to a loop like operation in the OpenMP dialect. A user can invoke this with the -convert-openmp-to-llvm conversion option with mlir-opt. The question I had is that most of the loops in MLIR seems to be SizedRegion<1>. Is there any issue with having a loop like operation (with bounds, step) and is Anyregion with multiple blocks? The spv.loop seems to be AnyRegion but is a collection of blocks with control flow but has a well-defined structure and no bounds.

>> I don't see why this would be the case. As mentioned above, the loop, whatever "op" it might be, belongs to the omp do. There is no omp do without loop, there is no "loop" once omp do has been lowered (to runtime calls). The omp do lowering is also not duplicating scf.for code if you use the OpenMPIRBuilder.
>
> It sounds like a satisfactory solution if we have an explicit loop-like construct in the OpenMP dialect, compatible with the LLVM dialect type system. At translation time, it is expected to contain LLVM+OpenMP dialect inside and the translation itself is straightforward thanks to a dedicated OpenMPIRBuilder method. It's still suboptimal to test this within MLIR, but we could trust OpenMPIRBuilder to be tested properly in LLVM.

OK.  I was worried you had an objection here.

>> Please, do not duplicate OpenMPIRBuilder functionality.
>
> +1.
>
>> I see. So you are suggesting just preserving the omp.do all the way into the LLVM dialect and then use the OpenMPIRBuilder in the MLIR LLVM dialect to LLVM IR *translator*?
>
> This was one of my initial concerns, the translator should be kept simple and OpenMPIRBuilder did not have a "create loop" function when I looked at it. So it was unclear to me if the folks implementing OpenMP intend to replicate it within MLIR, extend it in LLVM or do something else, hence my request for an RFC+discussion.

There were more things to discuss, like

1. should we have both the directive like omp.do operation and the loop like omp.do operation (name can be omp.wsloop) or should there be only the loop like omp.do operation. @DavidTruby was writing an RFC on this.
2. How flang fits into the picture. The FIR developers informed that scf.for might not be in the path and it is possible that fir.do loop operation might get converted directly to LLVM dialect. In this case there has to be additional conversions or passes in Flang to convert and fir.do inside an omp.do. Can the loop like omp.do operation be the target of fir.do that is concurrent?
3. Should we handle affine.for nested inside an omp.do? Is affine.for guaranteed to be converted to scf.for at some time and will the handling of scf.for inside omp.do automatically kick in?
4. Should we use the information that the omp.do loop is parallel to do additional optimisation or even convert to something like affine loops if it is possible?

> Two other options that are worth considering are:
>
> - using an attribute to annotate SCF loops as OpenMP loops; this adds a verification hook without requiring to duplicate an operation but the layering is still not very clear to me;

I was thinking for operations that are already parallel there will be a conversion operation to convert to the OpenMP dialect. -convert-scf-parallel-to-openmp. This can convert the parallel scf loop to the loop like operation in the OpenMP dialect.

> - making OpenMPIRBuilder somehow extensible so that it can build the LLVM dialect instead of LLVM IR, at which point we can have an OpenMP-to-LLVM dialect conversion that uses it directly and is testable within MLIR, and an actually trivial translation.
>
> I would be interested to hear from @mehdi_amini and @nicolasvasilache among others... @jdoerfert seems to have an account on Discourse, so he would normally receive an email if mentioned explicitly even if he doesn't follow the forum in general.

We are OK with submitting the RFC in discourse.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86071/new/

https://reviews.llvm.org/D86071