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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur requested changes to this revision.
Meinersbur added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:341
+                                         const Twine &Name = "loop",
+                                         bool CreatePreheader = false);
 
----------------
[serious] This is //really// ugly from a software architecture point of view. Where previously all could expect a CanonicalLoopInfo has a preheader has to exist, it now any loop-handling code has to check for it.

The description of CanonicalLoopInfo needs to be updated.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:616-617
+  /// \param IfCond The optional if clause
+  void applySimd(DebugLoc DL, ArrayRef<CanonicalLoopInfo *> Loops,
+                 Value *IfCond = nullptr, ConstantInt *Simdlen = nullptr);
+
----------------
What this function does should be achievable with 3 elementary steps:
1. Code versioning (`IfCond`)
2. `collapseLoops`
3. `applySimd(CanonicalLoopInfo, SimLen)`

Step 1 does not yet exist as an OpenMPIRBuilder method but but maybe should be made available independent of `applySimd` to by usable for anything that implements an `if` clause.

I would not object to have such a helper function doing all 3 steps together, but the description should reflect that.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2566-2570
+  if (ComputeIP.isSet()) {
     Builder.restoreIP(ComputeIP);
-  else
+  } else {
     Builder.restoreIP(Outermost->getPreheaderIP());
+  }
----------------
[nit] unrelated


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2874-2887
+  SmallVector<Metadata *> NewLoopProperties;
+  NewLoopProperties.push_back(nullptr);
+
+  assert(Latch && "A valid CanonicalLoopInfo must have a unique latch");
+  MDNode *Existing = Latch->getTerminator()->getMetadata(LLVMContext::MD_loop);
+  if (Existing)
+    append_range(NewLoopProperties, drop_begin(Existing->operands(), 1));
----------------
Please avoid copy&paste code. The common code can be extracted into a separate function.

However, I don't think a version taking a `Loop` info is even necessary. See comment below.


================
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(); });
+
----------------
[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.




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

https://reviews.llvm.org/D129368