[PATCH] D92476: [OpenMPIRBuilder] introduce createStaticWorkshareLoop

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:270
+  /// the current thread, updates the relevant instructions in the canonical
+  /// loop and calls to an OpenMP runtime finalization function after the loop.
+  ///
----------------
jdoerfert wrote:
> The preheader (only) solution is for static schedules but that is OK for now.
The method;s name is "createStaticWorkshareLoop"


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:281
+  /// \returns An insertion point after the finalization call.
+  InsertPointTy createStaticWorkshareLoop(const LocationDescription &Loc,
+                                          CanonicalLoopInfo *LoopInfo,
----------------
Return the modified `CanonicalLoopInfo` instead. The IP can be determined using getAfterIP().

Mostly for consistency with additional createXYZLoop methods, which create a new CanonicalLoopInfo instead of modifying an existing one.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:743
+  /// interpreted as an unsigned integer.
+  void setTripCount(Value *TripCount) {
+    Instruction *CmpI = &Cond->front();
----------------
Maybe make this member private (to the OpenMPIRBuilder implementation)? Other canonical loops such as tiling or OpenMPIRBuilder itself assuming a specific trip count during `finialize()` might depend on a specific trip count. 

There's nothing like this now nor do I plan to add such things, but until we have a use case why front-end should directly modify the trip-cout, I'd hide this from the API.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:747
+    CmpI->setOperand(1, TripCount);
+  }
+
----------------
add a call to assertOK() here?


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:748
+  }
+
   /// Returns the instruction representing the current logical induction
----------------
jdoerfert wrote:
> @Meinersbur: Eventually we might want to keep pointers identifying these different parts as we will add instructions to the blocks and therefore might break these invariants. Not necessary now though.
Uses of it within the CanonicalLoop still needs to be identified, for changes like this. This makes explicitly storing it redundant. Same would also apply to most BasicBlocks which have a fixed position relative to the header block, so we could traverse the CFG to find them instead of storing them.

I don't have a good reason for either direction, I suggest to do what's easier for now.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1007
+OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::createStaticWorkshareLoop(
+    const LocationDescription &Loc, CanonicalLoopInfo *LoopInfo,
+    InsertPointTy AllocaIP, Value *Chunk) {
----------------
jdoerfert wrote:
> I'm not a fan of `LoopInfo` as a name, `CLI` could be our "go to" acronym?
+1

LoopInfo clashes with `llvm::LoopInfo` and `clang::LoopInfo` types. The latter already makes it impossible to include some LLVM headers into `CGLoopInfo.cpp`


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1080-1081
+  // After the loop, call the "fini" function.
+  Builder.restoreIP(LoopInfo->getAfterIP());
+  Builder.CreateCall(StaticFini, {SrcLoc, ThreadNum});
+
----------------
Could you emit the fini call into the getExit block instead? getAfterIP is meant for user code that executes, well, after the runtime boilerplate has finished.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1083
+
+  return Builder.saveIP();
+}
----------------
Call LoopInfo->assertOK() when the changes are done?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92476