[PATCH] D92974: [OpenMPIRBuilder] Implement tileLoops.

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

I went through almost all of it and left some comments.



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:333
+  /// @param TileSizes For each loop in \p Loops, the tile size for that
+  ///                  dimensions.
+  ///
----------------
Nit: `\param`, `Loop*s* to tile`.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:340
+  tileLoops(DebugLoc DL, ArrayRef<CanonicalLoopInfo *> Loops,
+            ArrayRef<Value *> TileSizes);
+
----------------
Any reason to use a std vector here? Would it make sense to extend Loops instead (after changing the type)? The original loop vector is still "contained" if we append the new ones.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1165
+  BranchInst::Create(Target, Source);
+}
+
----------------
Nit: assertion message. We might want to pass a debug loc as well and set it, or use the builder. 


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1173
+    redirectTo(Pred, NewTarget);
+}
+
----------------
Nit: `auto *Pred` or `BasicBlock*`


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1205
+  DeleteDeadBlocks(BBVec);
+}
+
----------------
Even if we make blocks dead, do we really want to clean up here?


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1210
+                           ArrayRef<Value *> TileSizes) {
+  size_t NumLoops = Loops.size();
+  assert(TileSizes.size() == NumLoops &&
----------------
later you compare it against int, I think, maybe pick int consistently?


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1250
+  Builder.restoreIP(OutermostLoop->getPreheaderIP());
+  SmallVector<Value *, 4> FloorCount, FloorEpilogues, FloorRems;
+  for (int i = 0; i < NumLoops; ++i) {
----------------
So far, I think, you could just replace `FloorEpilogues` with `FloorCount`. Do we expect that they are different later?


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1282
+  std::vector<CanonicalLoopInfo *> Result;
+  Result.reserve(NumLoops * 2);
+
----------------
In case we want to change the signature: At this point, I think, you only use `Loops` to delete some code. Maybe we shouldn't, or even if we do we could still reuse `Loops` to return the new ones?.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1339
+    OutroInsertBefore = TileLoop->getLatch();
+  }
+
----------------
Should we create a helper for this loop and the loop above?


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1355
+    BodyEntered = ExitBB;
+  }
+
----------------
No plain `auto` please. I usually prefer structs with named members over pairs but that is just a suggestion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92974