Unroll-and-jam currently doesn't work when the loop being unroll-and-jammed or any of its inner loops have iter_args This patch modifies the unroll-and-jam utility to support loops with iter_args.
Details
Diff Detail
Event Timeline
| mlir/include/mlir/Analysis/AffineAnalysis.h | ||
|---|---|---|
| 43–46 | Pass output arg by reference unless it can be null. https://mlir.llvm.org/getting_started/DeveloperGuide/#style-guide | |
| mlir/lib/Dialect/Affine/Transforms/LoopUnrollAndJam.cpp | ||
| 79–80 | This requires a code comment. | |
| mlir/lib/Transforms/Utils/LoopUtils.cpp | ||
| 1414 | Please leave a space after "1." - here and above as well. | |
| 1490–1538 | This method has become too long (350 lines) and should be split I think. This revision is adding a substantial amount of code to it. | |
Thanks for creating this more comprehensive revision. Comments below.
| mlir/lib/Transforms/Utils/LoopUtils.cpp | ||
|---|---|---|
| 1415–1427 | This function can be made a static helper along isBoundDependent? | |
| 1419 | This is making an assumption that the first few operands would be the control operands. Instead, please use llvm::zip(..getLowerBoundOperands(), getUpperBoundOperands()) or can you add an accessor getControlOperands()? | |
| 1536 | Short code comment here. | |
| mlir/lib/Transforms/Utils/LoopUtils.cpp | ||
|---|---|---|
| 1490–1538 | But please feel free to let me know if there are reasons you think it's still better this way. | |
| mlir/lib/Dialect/Affine/Transforms/LoopUnrollAndJam.cpp | ||
|---|---|---|
| 79–80 | Removed the check. | |
| mlir/lib/Transforms/Utils/LoopUtils.cpp | ||
| 1415–1427 | Added helper areInnerBoundsInvariant. | |
| 1419 | Added getControlOperands() | |
| 1490–1538 | I have moved some code out to the helper functions. I was thinking to move other parts like cloning the subblocks out, but it would be hard for the readers to understand this old/new concepts like oldNumResults in a standalone function. | |
Overall, this is looking good. Some more comments below.
| mlir/include/mlir/Dialect/Affine/IR/AffineOps.td | ||
|---|---|---|
| 268 | Just to make it clear that it's the lower bound operands followed by the upper bounds operands, consider adding something like: "... in that order." | |
| mlir/lib/Transforms/Utils/LoopUtils.cpp | ||
| 1384–1385 | This statement isn't right. A loop does not have to be a parallel loop for its unroll-and-jam to be valid. Being a parallel loop is a sufficient condition but nowhere close to being necessary for unroll-and-jam. In fact, we need no statement here on validity - the utility just does the unroll-and-jam. It's up to the user to ensure that that is a valid transformation. As an aside: if it's valid to make a loop an innermost loop, it's valid to unroll-and-jam that loop by any factor -- this sufficient condition is closer to a necessary and sufficient condition. | |
| 1480–1486 | Code comments here will help. | |
| 1525–1532 | Code comments here will help. | |
| 1553–1554 | You can't use the result of getDefiningOp unchecked - it can be null. An assert is missing here since getReductionOp provides you that guarantee. | |
| 1556 | Nit: -> "Replace all uses except ..." | |
| mlir/lib/Transforms/Utils/LoopUtils.cpp | ||
|---|---|---|
| 1385 | The doc comment on this utility (which exists in the header file) should be expanded to indicate the support for reductions you've added, which is great. For a publicly exposed utility, we don't have to actually duplicate the doc comment on the function definition itself. | |
Address review comments.
| mlir/lib/Transforms/Utils/LoopUtils.cpp | ||
|---|---|---|
| 1384–1385 | Thanks. Deleted the statement. | |
| mlir/include/mlir/Transforms/LoopUtils.h | ||
|---|---|---|
| 70 ↗ | (On Diff #375012) | -> ... can be loops with iteration arguments performing supported reductions. ? |
@bondhugula Thank you for the review. We'll merge it tomorrow if there are no more comments.
Pass output arg by reference unless it can be null. https://mlir.llvm.org/getting_started/DeveloperGuide/#style-guide
If it can be null, please document that. If the goal is to just return things, I'm not sure why you need to accept a null argument and a pointer.