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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur accepted this revision.
Meinersbur added a comment.
This revision is now accepted and ready to land.

LGTM, this turned out really nice. Thank you!

Some nitpicks left. Please apply before committing.



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:622
+  /// \param NamePrefix Optional name prefix for if.then if.else blocks.
+  void createIfVersion(CanonicalLoopInfo *Loop, Value *IfCond,
+                       ValueToValueMapTy &VMap, const Twine &NamePrefix = "");
----------------
This could be made private as front-ends are encouraged to use the `IfCond` argument for the relevant instead that may implement specialized behavior (like passing the value to the runtime).

It still might be useful for front-ends (eg implementing dynamic metadirective), but I'd wait to such use cases to occur before adding this to the public API.

Edit: I noticed it is tested in the unit test, which requires it to be public.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2983
+    // Add metadata to the cloned loop which disables vectorization
+    Value *MappedValue = VMap[CanonicalLoop->getLatch()];
+    assert(Mapped &&
----------------
`operator[]` may modify the underlaying hashtable.

The name `MappedLatch` is more meaningful than `MappedValue`.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2986
+           "Cannot find value which corresponds to original loop latch");
+    assert(BasicBlock::classof(MappedValue) &&
+           "Cannot cast mapped latch block value to BasicBlock");
----------------



================
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(); });
+
----------------
Meinersbur wrote:
> 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²).
> 
Could you add a `TODO`/`FIXME` comment mentioning that we should not depend on a pass manger?


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

https://reviews.llvm.org/D129368