[Mlir-commits] [mlir] [MLIR][OpenMP] Add canonical loop LLVM-IR lowering (PR #147069)

[Michael Kruse]

================
@@ -108,6 +110,41 @@ class ModuleTranslation {
     return blockMapping.lookup(block);
   }
 
+  /// Find the LLVM-IR loop that represents an MLIR loop.
+  llvm::CanonicalLoopInfo *lookupOMPLoop(omp::NewCliOp mlir) const {
----------------
Meinersbur wrote:

The proposal seems to be driven from how the implementation should look be layered. I think this is backwards: the IR should be designed after what makes sense for the IR itself, and the implementation follow after that. In this case it would have been nice if ModuleTranslation would only need to care about the LLVM dialect, but it does not due to the past design to reuse the OpenMPIRBuilder as-is. It feels weird if today this past design decision drives the structure of the OpenMP dialect itself, just because we do not have a dedicated "OpenMP dialect to LLVM dialect" pass. The Flang codgen would probly just emit a top-level omp.loop_context region surrounding everything. and just leaking the implementation detail that ModuleTranslation needs some persistent storage into the IR design.

This does not mean the proposal is not without merit, but should have been discussed in the past RFCs, round tables, etc. we had about the design. At some point that discussion must come to a close. My thoughts about it: The design was inspired by the [Transform dialect](https://mlir.llvm.org/docs/Dialects/Transform/) which does not have scopes. It uses selectors but we wanted to refer to specific loops by identifier. It is a local mlir::Value now, but I could also imagine inter-procedural cli's to allow e.g. fusing loops that are going to be inlined. If an omp.loop_context does not just  appear in the outermost scope, it may impede analyses/transformations by passes that are unrelated to OpenMP due to them not knowing about it. For LLVM-IR, llvm.dbg.declare and llvm.dbg.value also had this property and were removed because of this. Having the %cli be the arguments of omp.loop_context makes adding more cli's (e.g. by an optimization pass) very difficult. I think the only way is to create a new omp.loop_context with more arguments and move the region to it while RAUW all occurrences of %cli. It does not integreate that well with loop wrappers because the apply clause allows applying e.g. wsloop on inner loops (e.g. after tiling, or loops sequences such as loop distribution) as well.

An advantage is that the possible scope of transformations become explicit and there is a guarantee that once the omp.loop_context is gone due to all transformation being applied, the loops stay as-is. I did not find a situation where this would be useful though.


https://github.com/llvm/llvm-project/pull/147069