[PATCH] D92476: [OpenMPIRBuilder] introduce createStaticWorkshareLoop

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Only Nits from me. @Meinersbur wdyt?



================
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.
+  ///
----------------
The preheader (only) solution is for static schedules but that is OK for now.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:664
+///  |   <...>   |
+///  |    | |    |
 ///   \--Latch   |
----------------
nice!


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:748
+  }
+
   /// Returns the instruction representing the current logical induction
----------------
@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.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:994
+// CanonicalLoopInfo.
+static FunctionCallee getKmpcForStaticInitForType(Type *type, Module &M,
+                                                  OpenMPIRBuilder &OMPBuilder) {
----------------
Nit: `Ty` or `IVType`


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


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1077
+    U->set(UpdatedIV);
+  }
+
----------------
Add a TODO above this block stating this should eventually move to the CLI or a "CLIUpdate" interface.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92476