[Mlir-commits] [mlir] [mlir][affine] Fix inverted epilogue check in affineForOpBodySkew (PR #217553)

[Alessandro Potenza]

https://github.com/alepot55 commented:

The change is right, and I think the reason it has no test is worth putting in the description rather than leaving a reviewer to wonder.

**The condition is inverted, and the state it selects is reachable inside the function.** `!epilogue` is true exactly when `epilogue` is null, so the branch fires only when there is no epilogue to unroll, and passes a null `AffineForOp` to `loopUnrollFull`. Meanwhile a real epilogue is never unrolled, which is the opposite of what the doc comment on `affineForOpBodySkew` promises. That combination is producible: the first loop can set both `prologue` and `epilogue` from the same `res`, and the trailing `opGroupQueue` loop can then reassign `epilogue` from a `generateShiftedLoop` that returns null while `prologue` stays set. The line above it, for the prologue, already has the shape yours gives the epilogue.

**It cannot be reached from in-tree today**, and that is the missing sentence. `unrollPrologueEpilogue` defaults to `false` in `LoopUtils.h`, and the only caller of `affineForOpBodySkew` in the tree is `PipelineDataTransfer.cpp:381`, which does not pass it. So both unroll calls are dead through every pass, which is why this survived and why there is no test to write against a pass. I built `11e915f2b75d` with the patch and `mlir/test/Dialect/Affine` is 72 of 72, unchanged, which is consistent with that rather than evidence of anything.

So the question for a reviewer is not whether the fix is correct but whether an obviously inverted condition on a path no in-tree caller enables should land as is. I would say yes and I would say so in the description, because "no test, and here is why one cannot be written through a pass" is a much easier thing to approve than silence.

Two smaller things:

- The description hedges twice ("my understanding is", "may be causing"). Neither is needed. The `!` is a slip, the line above shows what the author meant, and you can say that flatly.
- No reviewers are requested. @bondhugula has by far the most commits in `Affine/Utils/LoopUtils.cpp` and would be the person to ask.

https://github.com/llvm/llvm-project/pull/217553