Page MenuHomePhabricator

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

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



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.

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.

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.

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

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

Thanks, Andy!


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


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


Nit: drop trivial braces


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


Nit: drop trivial braces


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


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


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


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.


Nit: pad until 80 characters


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...


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.


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.


Thanks. Captured in the TODO here.


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 for a helper function.