[PATCH] D79184: [MLIR][LoopOps] Adds the loop unroll transformation for loop::ForOp.

[Andy Davis via Phabricator via llvm-commits]

andydavis1 marked 12 inline comments as done.
andydavis1 added a comment.

Thanks Alex! Will rebase this with changes in a bit...



================
Comment at: mlir/lib/Transforms/Utils/LoopUtils.cpp:140
+// where divis is rounding-to-zero division.
+static Value ceilDivPositive(OpBuilder &builder, Location loc, Value dividend,
+                             Value divisor) {
----------------
ftynse wrote:
> Would it make sense to share this with affine expression lowering https://github.com/llvm/llvm-project/blob/d3588d0814c4cbc7fca677b4d9634f6e1428a331/mlir/lib/Conversion/AffineToStandard/AffineToStandard.cpp#L149  ?
Thanks for pointing that out. I do think it makes sense to combine those at some point, as the implementation you reference appears more general. I'll give that some thought, perhaps it would make the unrolling implementation more general.


================
Comment at: mlir/lib/Transforms/Utils/LoopUtils.cpp:489
+  // so that we know what to clone (since we are doing this in-place).
+  Block::iterator srcBlockEnd = std::prev(loopBodyBlock->end(), 2);
+
----------------
ftynse wrote:
> Since this is only used inside `std::next` below, how about taking `std::prev(..., 1)` and dropping `std::next` ?
In this case, we need to keep the last non-terminator because the loop body is being cloned in place std::next(srcBlockEnd) can change as unrolled loop bodies are cloned in-place.


================
Comment at: mlir/lib/Transforms/Utils/LoopUtils.cpp:630
+    // Dynamic loop bounds computation.
+    // TODO(andydavis) Add dynamic asserts for negative lb/ub/step.
+    auto lowerBound = forOp.lowerBound();
----------------
ftynse wrote:
> If you take the ceildiv implementations from AffineApplyExpander, you may be able to support negative dividends.
Thanks. Captured in the TODO here.


================
Comment at: mlir/test/Dialect/Loops/loop-unroll.mlir:20
+//
+//   UNROLL-BY-2-DAG:  %[[V0:.*]] = subi %[[UB]], %[[LB]] : index
+//   UNROLL-BY-2-DAG:  %[[C1:.*]] = constant 1 : index
----------------
ftynse wrote:
> Why is it a DAG? Is there some non-determinism in operation order?
I think that I did see some non-determinism, but these are also DAG because the ordering here for these particular ops is not critical to the transformation (these ops just need to be ordered correctly w.r.t dependences). I've used CHECK and CHECK-NEXT for ops that are critical for the test to show the transformation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79184