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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. |
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. |
mlir/lib/Transforms/Utils/LoopUtils.cpp | ||
---|---|---|
195 | Sure, fine to do that in a follow up patch. |
Thanks, Andy!
mlir/lib/Transforms/Utils/LoopUtils.cpp | ||
---|---|---|
140 | 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 ? | |
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? |
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. |
@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.
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 ?