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

[Uday Bondhugula via Phabricator via llvm-commits]

bondhugula added a comment.

Mostly minor suggestions on readability



================
Comment at: mlir/lib/Transforms/Utils/LoopUtils.cpp:1171
+  OpBuilder builder(outer);
+  OpBuilder innerBuilder(inner.getBody(), inner.getBody()->begin());
+  auto loopPieces =
----------------
`OpBuilder innerBuilder(inner.getBody())` will be sufficient.


================
Comment at: mlir/lib/Transforms/Utils/LoopUtils.cpp:1236
 
+void mlir::collapsePLoops(loop::ParallelOp loops,
+                          ArrayRef<std::vector<unsigned>> combinedDimensions) {
----------------
But to spell this out? PLoops -> ParallelLoops?


================
Comment at: mlir/lib/Transforms/Utils/LoopUtils.cpp:1257
+
+  // Combine iteration spaces
+  SmallVector<Value, 3> lowerBounds;
----------------
Nit: period at the end.


================
Comment at: mlir/test/Transforms/parallel-loop-collapsing.mlir:5-14
+  // CHECK:           [[VAL_0:%.*]] = constant 6 : index
+  // CHECK:           [[VAL_1:%.*]] = constant 7 : index
+  // CHECK:           [[VAL_2:%.*]] = constant 9 : index
+  // CHECK:           [[VAL_3:%.*]] = constant 10 : index
+  // CHECK:           [[VAL_4:%.*]] = constant 12 : index
+  // CHECK:           [[VAL_5:%.*]] = constant 13 : index
+  // CHECK:           [[VAL_6:%.*]] = constant 3 : index
----------------
Can you drop the extra indent? Also VAL_0 -> C6, VAL_1 -> C7, ...?


================
Comment at: mlir/test/Transforms/parallel-loop-collapsing.mlir:31
+
+  // CHECK:           loop.parallel ([[VAL_10:%.*]], [[VAL_11:%.*]], [[VAL_12:%.*]]) = ([[VAL_7]], [[VAL_7]], [[VAL_7]]) to ([[VAL_9]], [[VAL_8]], [[VAL_8]]) step ([[VAL_8]], [[VAL_8]], [[VAL_8]]) {
+  loop.parallel (%i0, %i1, %i2, %i3, %i4) = (%c0, %c3, %c6, %c9, %c12) to (%c2, %c5, %c8, %c11, %c14)
----------------
Can you use more descriptive names? VAL_10 -> IV0, VAL_11 -> IV1, ...



================
Comment at: mlir/test/Transforms/parallel-loop-collapsing.mlir:34
+                                          step (%c1, %c4, %c7, %c10, %c13) {
+    // CHECK:             [[VAL_13:%.*]] = remi_signed [[VAL_10]], [[VAL_9]] : index
+    // CHECK:             [[VAL_14:%.*]] = divi_signed [[VAL_10]], [[VAL_8]] : index
----------------
Drop the additional indent between the CHECK and the string?


================
Comment at: mlir/test/Transforms/parallel-loop-collapsing.mlir:49
+// CHECK:             loop.yield
+// CHECK:           }
+// CHECK:           return
----------------
CHECK-NEXT


================
Comment at: mlir/test/Transforms/parallel-loop-collapsing.mlir:50-51
+// CHECK:           }
+// CHECK:           return
+// CHECK:         }
+
----------------
CHECK-NEXT


================
Comment at: mlir/test/Transforms/single-parallel-loop-collapsing.mlir:26
+    // CHECK:             [[VAL_13:%.*]] = addi [[VAL_12]], [[VAL_3]] : index
+    // CHECK:             [[VAL_14:%.*]] = "magic.op"([[VAL_13]], [[VAL_11]]) : (index, index) -> index
+    %result = "magic.op"(%i0, %i1): (index, index) -> index
----------------
VAL_14 isn't used; no need to capture it.


================
Comment at: mlir/test/Transforms/single-parallel-loop-collapsing.mlir:32-33
+// CHECK:             loop.yield
+// CHECK:           }
+// CHECK:           return
+// CHECK:         }
----------------
CHECK-NEXT 


================
Comment at: mlir/test/Transforms/single-parallel-loop-collapsing.mlir:34
+// CHECK:           return
+// CHECK:         }
+
----------------
Not needed.


================
Comment at: mlir/test/Transforms/single-parallel-loop-collapsing.mlir:35
+// CHECK:         }
+
+
----------------
Drop trailing blank lines.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76363