Page MenuHomePhabricator

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

Authored by andydavis1 on Thu, Apr 30, 10:40 AM.

Details

Summary

Adds the loop unroll transformation for loop::ForOp.
Adds support for promoting the body of single-iteration loop::ForOps into its containing block.
Adds check tests for loop::ForOps with dynamic and static lower/upper bounds and step.
Care was taken to share code (where possible) with the AffineForOp unroll transformation to ease maintenance and potential future transition to a LoopLike construct on which loop transformations for different loop types can implemented.

Diff Detail

Event Timeline

andydavis1 created this revision.Thu, Apr 30, 10:40 AM
bondhugula requested changes to this revision.Fri, May 1, 12:57 AM
bondhugula added a subscriber: bondhugula.
bondhugula added inline comments.
mlir/lib/Transforms/Utils/LoopUtils.cpp
195

I don't think you need a separate method. Just change the promoteIfSingleIteration to work on LoopLikeOp. You'll just need to add a new getLowerBound interface method (pass the builder for it to create an affine.apply if necessary). The rest like getConstantTripCount / getConstantLowerBound are all already there or easy to add.

This revision now requires changes to proceed.Fri, May 1, 12:57 AM
andydavis1 marked an inline comment as done.Fri, May 1, 8:22 AM
andydavis1 added inline comments.
mlir/lib/Transforms/Utils/LoopUtils.cpp
195

Thanks. Yes, I agree that we can do more code sharing by moving these changes towards a LoopLike interface kind of a thing (I mentioned that a bit in the description). But if possible, I'd like to do that in a follow up change, as its not a simple as switching loop::ForOp to LoopLikeOp, and I'd like to minimize the changes in this patch. Thanks.

andydavis1 requested review of this revision.Fri, May 1, 8:23 AM
bondhugula marked an inline comment as done.Sat, May 2, 9:22 AM
bondhugula added inline comments.
mlir/lib/Transforms/Utils/LoopUtils.cpp
195

Sure, fine to do that in a follow up patch.

ftynse accepted this revision.Tue, May 5, 3:01 AM

Thanks, Andy!

mlir/lib/Transforms/Utils/LoopUtils.cpp
140
211

Nit: it feels like you could just call iv.replaceAllUsesWith and let it do nothing if there are no uses

489

Since this is only used inside std::next below, how about taking std::prev(..., 1) and dropping std::next ?

504

Nit: drop trivial braces

603

Please document this precondition. I don't think loop::ForOp disallows negative bounds.

613

Nit: drop trivial braces

655

If you take the ceildiv implementations from AffineApplyExpander, you may be able to support negative dividends.

671

This comment looks confusing because it doesn't account for step. (Same issue with the affine version)

mlir/test/Dialect/Loops/loop-unroll.mlir
21

Why is it a DAG? Is there some non-determinism in operation order?

48

Hmm, could you just use the same input (i.e. @dynamic_loop_unroll) and match it with different prefixes? All input functions are transformed by all four RUNs and most of them are just ignored in the test.

mlir/test/lib/Transforms/TestLoopUnrolling.cpp
2

Nit: pad until 80 characters

47

Nit: since the required depth is known upfront, how about just storing the loops of this depth in a vector, instead of filtering the vector of all loops later?

This revision is now accepted and ready to land.Tue, May 5, 3:01 AM
andydavis1 marked 12 inline comments as done.Tue, May 5, 10:04 AM

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

mlir/lib/Transforms/Utils/LoopUtils.cpp
140

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.

489

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.

655

Thanks. Captured in the TODO here.

mlir/test/Dialect/Loops/loop-unroll.mlir
21

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.

addressing review comments

This revision was automatically updated to reflect the committed changes.

@andydavis1 : can you cleanup commit messages from extra phabricator stuff before pushing please? See https://mlir.llvm.org/getting_started/Contributing/#using-arcanist for a helper function.