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

[Alex Zinenko via Phabricator via llvm-commits]

ftynse added a comment.

> 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.

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.

> 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.

> 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.

> 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.

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;
- 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86071