Differential D86071
[MLIR][OpenMP] Add omp.wsloop operation Authored by kiranchandramohan on Aug 17 2020, 6:25 AM.
Details This adds a simple definition of a "workshare loop" operation for The schedule clause also does not yet accept the modifiers that are
Diff Detail
Event Timeline
Comment Actions A few general comments about the design of the workshare loop operation.
Openmp for/do loops with static scheduling are implemented by the two following runtime call. How will these values be provided to the OpenMP IRBuilder if these are not carried in the loop operation?
Unlike parallel, the loop operation will always have a loop associated with it. All higher-level dialects have loop operations. But since we lower to OpenMP + LLVM dialect (and llvm has no loop operation) we need to have any region and not just one block. An alternative would be to have two omp.do operations one which sits with the do loop (of one block) and one which sits without (any region).
Comment Actions @DavidTruby, In general, I was asking whether all clause enumeration values can begin with a Capital letter? Just the reserved keywords would look odd right? Also, can you mark fixed comments as done? I guess for the general workshare loop design issues we can have an RFC in discourse. But this patch can go ahead.
Comment Actions Oh I see. Yes, we can do that. I'll make the same change for parallel in a separate patch. Comment Actions
I haven't seen answers to the questions about lowering to LLVM IR + OpenMP runtime, and it sounds suboptimal to push the patch before discussing and agreeing on the actual design. 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. Comment Actions The last part is unclear to me TBH. What exactly do you expect to do with OpenMP worksharing loops on this level which is problematic with CFGs? Comment Actions
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 herego into mlir-translate, which will have to outline and lower the loop during _translation_ in this case. Comment Actions I think I might simply misunderstand because I'm oblivious to the "MLIR way" but the loop belongs to the OpenMP directive. Comment Actions
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. Comment Actions Yes.
All OpenMP related LLVM-IR code generation is (eventually) going to be moved into OpenMPIRBuilder so we do not duplicate the rather nontrivial parts in two places which would become a maintenance nightmare. That is (among other things) the point ;) Comment Actions Is there a written version somewhere we can see?
This sounds great! What I am missing is how this connects to MLIR. Two particular issues: how the OpenMP dialect interacts with other dialects, and can the OpenMPIRBuilder be designed so as to eventually take an mlir::Builder instead of llvm::IRBuilder<> and produce LLVM dialect instead of LLVM; there may be others. So far, the implementation of omp.do proposed here does not align, if not contradicts, the original RFC. Bottomline, this deserves a proper discussion in a forum with better visibility than code review. Part of the discussion seemingly happens in flang channels and should be somehow summarized for mlir folks as OpenMP dialect being independent of any Fortran construct was a condition for accepting it in core. Comment Actions Thanks @ftynse for the feedback. Yes, the whole flow for the OpenMP worksharing loop requires a detailed discussion in discourse. I was only suggesting that since the operation in this patch only models what is an OpenMP do directive it can go ahead. The RFC for the workshare operation is next on @DavidTruby's list. It is fine to have the RFC discussion before submitting this patch. Comment Actions A written version of the plan? Yes. A written version of the code, not yet.
So far that was not on my TODO list and it seems like a lot of work assuming you do not port various other things into MLIR land. Could you help me understand what we would gain by generating LLVM dialect? Comment Actions Could you post a link?
Would you mind moving this to discourse / mailing list? Comment Actions Sure: http://lists.flang-compiler.org/pipermail/flang-dev_lists.flang-compiler.org/2019-May/000197.html
No, feel free to reply to the thread above or start a new one. Comment Actions
@jdoerfert This is probably the old flang-dev mailing list. Is it OK for @jdoerfert and @ftynse If I initiate the discussion in discourse? Comment Actions I'm fine with anything that is not a code review, which most people would just ignore Comment Actions That's the main discussion channel for MLIR, we expect RFCs to go there. We can go to llvm-dev@ for the OpenMPIRBuilder discussion, I think it is actually the best place given how it cuts across LLVM, Clang and Flang. Comment Actions OK. I had almost typed in discourse. I primarily wanted to ask the following question before posing a general question in llvm-dev about the dialect or the OpenMP IRBuilder. Consider that we want to parallelize an SCF loop with OpenMP. We can add the omp.do operation around the loop. This would look like. func @some_op_inside_loop(%arg0: index, %arg1: index, %arg2: index) {
omp.do {
scf.for %i = %arg0 to %arg1 step %arg2 {
"some.op"(%i) : (index) -> ()
}
}
return
}One way to pass this to the OpenMP IR Builder would be as follows. The loop exists as control flow. (Question: Can we have index, start, end and step variables here as operands or attributes?) llvm.func @some_op_inside_loop(%arg0: !llvm.i64, %arg1: !llvm.i64, %arg2: !llvm.i64) {
omp.do {
llvm.br ^bb1(%arg0 : !llvm.i64)
^bb1(%0: !llvm.i64): // 2 preds: ^bb0, ^bb2
%1 = llvm.icmp "slt" %0, %arg1 : !llvm.i64
llvm.cond_br %1, ^bb2, ^bb3
^bb2: // pred: ^bb1
"some.op"(%0) : (!llvm.i64) -> ()
%2 = llvm.add %0, %arg2 : !llvm.i64
llvm.br ^bb1(%2 : !llvm.i64)
^bb3: // pred: ^bb1
}
llvm.return
}Another way would be as follows where the loop exists without the control flow of the loop. llvm.func @some_op_inside_loop(%arg0: !llvm.i64, %arg1: !llvm.i64, %arg2: !llvm.i64) {
omp.do index(%i: !llvm.i64) start(%arg0: !llvm.i64) stop(%arg1: !llvm.i64) step(%arg2: !llvm.i64) {
"some.op"(%i) : (!llvm.i64) -> ()
}
return
}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? Comment Actions Great to see this. Some minor comments.
Comment Actions +1 on having clarity on the lowering part to LLVM dialect. I think the op itself doesn't place any restrictions on what's inside. There could be scf.for ops nested inside or surrounding an omp.do given that they both use the same type subsystem (standard types). One would expect an scf.parallel to be converted to an omp.do? The remaining scf.for's would be lowered to LLVM dialect the usual way. It looks like we need some sort of a pre-pass on the LLVM dialect to lower/handle the omp.do and some of the implementation therein would have to duplicate/reuse the logic of scf.for lowering. @DavidTruby @kiranchandramohan - do you have an example sketch of how the LLVM dialect would like for a simple / minimal omp.do loop right before it's translated out to LLVM IR proper? Would it require duplicating LLVM's OpenMPIRBuilder functionality here on the LLVM dialect? Comment Actions Thanks @bondhugula for the review and comments. Very helpful. We will post out an RFC in a week or so and take it forward from there. Will try to answer your questions in the RFC. We will likely need some help to complete the flow. Comment Actions I forgot to submit the first part of this response a few days ago, apologies. This is not parallelization but worksharing, but I think I get what you're saying.
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. InsertPos CreateWorksharingLoop(..., LowerBound, UpperBound, Step, ..., BodyCodeGenCallback) 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.
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.
Please, do not duplicate OpenMPIRBuilder functionality. Comment Actions
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*? Comment Actions
It's generally hard to prove that something is not contained in a region in MLIR. You can always have "unknown.op"() in there, and you'd have to treat it conservatively by assuming it may be an equivalent of the forbidden call or it lowers to such call. The presence of additional constraints on what is allowed inside the loop body suggests that OpenMP loops should a different operation than scf.for or scf.parallel, potentially sharing interfaces/traits with the latter. That being said, introducing yet another "loop-like" operation also sounds suboptimal.
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.
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.
+1.
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. Two other options that are worth considering are:
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. Comment Actions
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 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.
OK. I was worried you had an objection here.
There were more things to discuss, like
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.
We are OK with submitting the RFC in discourse. Comment Actions I have submitted an RFC on the openmp do loop design here: https://llvm.discourse.group/t/openmp-worksharing-loop-rfc/1815 Comment Actions Taking over from @DavidTruby on this patch. Contains the following modifications,
Comment Actions Thanks @Meinersbur for your comments. I will have a look soon. I suspect these are only used now for generating string enum classes in mlir openmp. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
For the default clause in parallel we have used a prefix "def" to fix this issue. I think we need to standardize this. Would converting the first letter to caps be a reasonable workaround since reserved keywords do not have a letter with caps as first letter?