[PATCH] D129368: [mlir][OpenMP] Lower simd if clause to LLVM IR

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:604
   /// \param Simdlen The Simdlen length to apply to the simd loop.
   void applySimd(CanonicalLoopInfo *Loop, ConstantInt *Simdlen);
 
----------------
As mentioned, I think this can be integrated into the existing `applySimd` methods. All it does need is the `IfCond` argument, and if it is set, implement the loop versioning.  After that, use the same code to attach the metadata.

`collapseLoops` can be called by the translation layer to get a single `CanonicalLoopInfo` handle to be passed to this function.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2983
 void OpenMPIRBuilder::applySimd(CanonicalLoopInfo *CanonicalLoop,
                                 ConstantInt *Simdlen) {
   LLVMContext &Ctx = Builder.getContext();
----------------



================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:3010
   }
 
   // Add access group metadata to memory-access instructions.
----------------



================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2948-2958
+  LLVMContext &Ctx = Builder.getContext();
+  FunctionAnalysisManager FAM;
+  Function *F = Loops.front()->getFunction();
+  FAM.registerPass([]() { return DominatorTreeAnalysis(); });
+  FAM.registerPass([]() { return LoopAnalysis(); });
+  FAM.registerPass([]() { return PassInstrumentationAnalysis(); });
+
----------------
domada wrote:
> Meinersbur wrote:
> > [serious] As far as I understand, the only reason this instantiates a pass manger is to use the `cloneLoopWithPreheader` function. However, it is sufficient to clone all block from CanonicalLoopInfo::getHeader() to CanonicalLoopInfo::getAfter(). Since the ExitBlock is guaranteed to be the only exit out of the loop (at least until I can introduce breaks; D124823 is a step into that direction), all blocks in the loop can be simply enumerated and cloned without cloneLoopWithPreheader.
> > 
> > This functionality is probably useful for implementing any loop versioning for `if`-clauses and should probably made its own method.
> > 
> > 
> I do not use `cloneLoopWithPreheader `function any more, but I still need pass manager to define Loop blocks. Original `applySimd ` function also uses pass manager for defining loop blocks. Please let me know if you have better idea, how to extract all body blocks.
Enumerating loop block without LoopInfo will be possible after D124823. Instantiation of an entire pass manager infrastructure does not scale well and should not be a long-term solution. It runs DominatorTree, LoopInfo etc on the entire function. `n` loops in the function have a complexity of at O(n³), because DominatorTree is already O(n²).



================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:965-971
+  if (loop.if_expr()) {
+    ompBuilder->createSimdLoop(ompLoc.DL, loopInfos,
+                               moduleTranslation.lookupValue(loop.if_expr()),
+                               nullptr);
+  } else {
+    ompBuilder->createSimdLoop(ompLoc.DL, loopInfos);
+  }
----------------



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

https://reviews.llvm.org/D129368