[PATCH] D92974: [OpenMPIRBuilder] Implement tileLoops.

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur marked an inline comment as done.
Meinersbur added inline comments.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:701
+
+  /// Create the control flow structure of a canonical OpenMP loop.
+  ///
----------------
SouraVX wrote:
> NIT: Would it make sense to do this as a separate maybe (NFCI) patch ? FWIW it will reduce the size of this patch.
> WDYT?
I can extract out the createLoopSkeleton, but I also think this patch is manageable to review. Note that there asl API changes such as introducing the `ComputeIP` and `Name` parameters that are not NFCI, unless you consider public API changes as non-functional in which case this entire patch is NFC.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:829
 
+  /// Return the type of the induction variable (and the trip count).
+  Type *getIndVarType() const { return getIndVar()->getType(); }
----------------
SouraVX wrote:
> NIT: these helper functions can also be moved, as mentioned in above comment,
I don't think it makes sense to introduce the helpers to another patch that do not also add a use for them. Please also consider that each additional patch adds churn.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1208
+std::vector<CanonicalLoopInfo *>
+OpenMPIRBuilder::tileLoops(DebugLoc DL, ArrayRef<CanonicalLoopInfo *> Loops,
+                           ArrayRef<Value *> TileSizes) {
----------------
SouraVX wrote:
> Please help me understand this: Why `DebugLoc` ? Why not `LocationDescription` ? Rest of the API's consistently uses `LocationDescription`.
tileLoops does not need an insert location, it transforms an existing loop where it currently is. DebugLoc is for the additional instructions inserted into the existing loop due to the tiling (such as those that compute the number of iterations).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92974