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

[Stephan Herhut via Phabricator via llvm-commits]

herhut requested changes to this revision.
herhut added a comment.
This revision now requires changes to proceed.

Thanks. Looks great with some nits.



================
Comment at: mlir/lib/Transforms/ParallelLoopCoalescing.cpp:44
+class ParallelLoopCoalescingPass
+    : public FunctionPass<ParallelLoopCoalescingPass> {
+public:
----------------
Can you make this an `OperationPass` instead?


================
Comment at: mlir/lib/Transforms/ParallelLoopCoalescing.cpp:52
+    func.walk([&](loop::ParallelOp op) {
+      std::vector<std::vector<unsigned>> combinedLoops(3);
+      combinedLoops[0] = clCoalescedIndices0;
----------------
bondhugula wrote:
> List initialize?
Use `llvm::SmallVector` instead?


================
Comment at: mlir/lib/Transforms/Utils/LoopUtils.cpp:979
+// conversion back to the original loop induction value inside the given Block.
+static std::tuple<Value, Value, Value>
+normalizeLoop(OpBuilder &boundsBuilder, OpBuilder &insideLoopBuilder,
----------------
Maybe have a little struct here instead of a tuple? Or use `std::tie` at use sites to improve readability.


================
Comment at: mlir/lib/Transforms/Utils/LoopUtils.cpp:983
+              Value inductionVar) {
+  Value newLowerBound, newUpperBound, newStep;
   // Check if the loop is already known to have a constant zero lower bound or
----------------
Move these closer to their first use.


================
Comment at: mlir/lib/Transforms/Utils/LoopUtils.cpp:1008
+
+  if (isZeroBased) {
+    newLowerBound = lowerBound;
----------------
Maybe `Value newLowerBound = isZeroBased ? lowerBound : boundsBuilder.create<ConstantIndexOp>(loc, 0)`?


================
Comment at: mlir/lib/Transforms/Utils/LoopUtils.cpp:1014
+
+  if (isStepOne) {
+    newStep = step;
----------------
Here, too?


================
Comment at: mlir/lib/Transforms/Utils/LoopUtils.cpp:1053
+  loop.setLowerBound(std::get<0>(loopPieces));
+  loop.setStep(std::get<2>(loopPieces));
+  loop.setUpperBound(std::get<1>(loopPieces));
----------------
Mega-nit: The order lower, step, upper is strange...


================
Comment at: mlir/lib/Transforms/Utils/LoopUtils.cpp:1141
+  for (unsigned i = 0; i < combinedDimensions.size(); i++) {
+    Value newUpperBound = outsideBuilder.create<ConstantIndexOp>(loc, 1);
+    for (auto idx : combinedDimensions[i]) {
----------------
Why not `newUpperBound = cst1` here?


================
Comment at: mlir/lib/Transforms/Utils/LoopUtils.cpp:1157
+    Value previous = newPloop.getBody()->getArgument(i);
+    for (unsigned idx = 0, e = combinedDimensions[i].size(); idx < e; ++idx) {
+      unsigned ivar_idx = combinedDimensions[i][idx];
----------------
A comment what this computes would help readability.


================
Comment at: mlir/lib/Transforms/Utils/LoopUtils.cpp:1161
+        previous = insideBuilder.create<SignedDivIOp>(
+            loc, previous, loops.upperBound()[ivar_idx]);
+
----------------
Should this be the normalized upper bound?


================
Comment at: mlir/lib/Transforms/Utils/LoopUtils.cpp:1163
+
+      Value iv = (idx == e - 1)
+                     ? previous
----------------
It would read easier for me if updating previous was also done here except for the last case. Would that make sense?


================
Comment at: mlir/lib/Transforms/Utils/LoopUtils.cpp:1166
+                     : insideBuilder.create<SignedRemIOp>(
+                           loc, previous, loops.upperBound()[ivar_idx]);
+      replaceAllUsesInRegionWith(loops.getBody()->getArgument(ivar_idx), iv,
----------------
Normalized here, too?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76363