[PATCH] D92476: [OpenMPIRBuilder] introduce createStaticWorkshareLoop

[Alex Zinenko via Phabricator via llvm-commits]

ftynse 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.
+  ///
----------------
Meinersbur wrote:
> jdoerfert wrote:
> > The preheader (only) solution is for static schedules but that is OK for now.
> The method;s name is "createStaticWorkshareLoop"
I only implemented the static schedule for now. My idea was to have createStatic, createDynamic, etc and eventually add a createWorkshareLoop that additionally takes a schedule kind flag and dispatches to the right function.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:282
+  InsertPointTy createStaticWorkshareLoop(const LocationDescription &Loc,
+                                          CanonicalLoopInfo *LoopInfo,
+                                          InsertPointTy AllocaIP,
----------------
Meinersbur wrote:
> kiranchandramohan wrote:
> > No change required but just asking a question for my understanding,
> > Can createWorkshareLoop call createCanonicalLoop internally?
> In the patch I am working on, I extracted most of `createCanonicalLoop` into a private `createCanonicalLoopSkeleton` to be used by other `createXYZLoop` methods (in my case: tiling).
> 
> However, this patch modifies an existing CanonicalLoopInfo, instead of creating a new one.
If I understood @Meinersbur's comment on another patch correctly, the overall design is to create canonical loops and transform them later (e.g., tile or workshare) with dedicated functions. `createStaticWorkshareLoop` follows this idea and therefore does _not_ create the loop, only the worksharing constructs.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:743
+  /// interpreted as an unsigned integer.
+  void setTripCount(Value *TripCount) {
+    Instruction *CmpI = &Cond->front();
----------------
Meinersbur wrote:
> 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.
Moved to implementation.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1076
+        continue;
+    U->set(UpdatedIV);
+  }
----------------
Meinersbur wrote:
> Did you consider using `replaceAllUsesWith`? `setTripCount` should already have removed the instances ub cond and latch.
This replaces the uses of the induction variable (phi of the header block) with induction-variable + lower-bound-from-runtime-call, `setTripCount` updated the trip count (another argument in `icmp` in the condition block). 

Switched to `replaceAllUsesWithIf` though.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1082
+  Builder.CreateCall(StaticFini, {SrcLoc, ThreadNum});
+
+  return Builder.saveIP();
----------------
kiranchandramohan wrote:
> A barrier insertion with a TODO to look at nowait attribute?
Added a flag to the function. Let's have the caller decide if they need a barrier or not. For example, clang doesn't insert a barrier after the last loop in the "parallel" segment even if the loop doesn't have "nowait".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92476