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

[Babar Khan]

BabarZKhan wrote:

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


@alepot55: thanks a lot for your comprehensive review and clarifying the original description even better. 

Do you recommend me to update my description that reflects your suggestions or do you think your comment already addresses it?

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