[PATCH] D76363: [MLIR] Add parallel loop coalescing.

[River Riddle via Phabricator via llvm-commits]

rriddle added a comment.

(Just some drive-by nits)



================
Comment at: mlir/include/mlir/InitAllPasses.h:114
   createParallelLoopTilingPass();
+  createParallelLoopCoalescingPass();
 
----------------
Let's keep these sorted.


================
Comment at: mlir/include/mlir/Transforms/LoopUtils.h:229
 
+void coalescePLoops(loop::ParallelOp loops,
+                    std::vector<std::vector<unsigned>> combinedDimensions);
----------------
Can you please add a comment here?


================
Comment at: mlir/lib/Transforms/ParallelLoopCoalescing.cpp:23
+
+static llvm::cl::OptionCategory clOptionsCategory(DEBUG_TYPE " options");
+
----------------
Can you use pass options instead? https://mlir.llvm.org/docs/WritingAPass/#instance-specific-pass-options


================
Comment at: mlir/lib/Transforms/Utils/LoopUtils.cpp:976
 
-// Transform a loop with a strictly positive step
-//   for %i = %lb to %ub step %s
-// into a 0-based loop with step 1
-//   for %ii = 0 to ceildiv(%ub - %lb, %s) step 1 {
-//     %i = %ii * %s + %lb
-// Insert the induction variable remapping in the body of `inner`, which is
-// expected to be either `loop` or another loop perfectly nested under `loop`.
-// Insert the definition of new bounds immediate before `outer`, which is
-// expected to be either `loop` or its parent in the loop nest.
-static void normalizeLoop(loop::ForOp loop, loop::ForOp outer,
-                          loop::ForOp inner) {
-  OpBuilder builder(outer);
-  Location loc = loop.getLoc();
-
+// Return the new lower bound, upper bound, and step in that order. Insert any
+// additional bounds calculations before the given builder and any additional
----------------
nit: Use /// for top-level comments


================
Comment at: mlir/lib/Transforms/Utils/LoopUtils.cpp:1001
   // on step positivity, whatever gets implemented first.
-  Value diff =
-      builder.create<SubIOp>(loc, loop.upperBound(), loop.lowerBound());
-  Value numIterations = ceilDivPositive(builder, loc, diff, loop.step());
-  loop.setUpperBound(numIterations);
-
-  Value lb = loop.lowerBound();
-  if (!isZeroBased) {
-    Value cst0 = builder.create<ConstantIndexOp>(loc, 0);
-    loop.setLowerBound(cst0);
+  if (isZeroBased && isStepOne) {
+    return {lowerBound, upperBound, step};
----------------
nit: Please drop all trivial braces.


================
Comment at: mlir/lib/Transforms/Utils/LoopUtils.cpp:1035
+
+// Transform a loop with a strictly positive step
+//   for %i = %lb to %ub step %s
----------------
nit: Please use /// for top-level comments.


================
Comment at: mlir/lib/Transforms/Utils/LoopUtils.cpp:1122
+  SmallVector<Value, 3> normalizedUpperBounds;
+  for (unsigned i = 0; i < loops.getNumLoops(); i++) {
+    OpBuilder insideLoopBuilder(loops.getBody(), loops.getBody()->begin());
----------------
nit: Cache the end iterator of the loop, and prefer pre-increment.


================
Comment at: mlir/lib/Transforms/Utils/LoopUtils.cpp:1155
+  OpBuilder insideBuilder(newPloop.getBody(), newPloop.getBody()->begin());
+  for (unsigned i = 0; i < combinedDimensions.size(); i++) {
+    Value previous = newPloop.getBody()->getArgument(i);
----------------
Same here and below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76363