[Mlir-commits] [mlir] Lower affine modulo by powers of two using bitwise AND (PR #146311)

[Uday Bondhugula]

bondhugula wrote:

> Sorry, i didn't look at this until i got a "rerequest review" notification. Thanks for answering why it cannot be done on remsi/remui, that sounds fair, and I wouldve dismissed my review.
> 
> > If you had `x mod 2`, with x being a non-negative IV, the range analysis that exists should conclude that the result of `remsi` is always positive, optimizing away the cmp + select. But that doesn't accomplish the better lowering you are enabling here -- because if the LHS is negative, we can still generate a single `andi` for a mod expression in affine maps. So, if it's a canonicalization, it will have to look at `remsi + cmp + add + select`.
> 
> So this optimization works for `x mod 2` even if we have no range information on `x`? Then we should plumb a `math.mod` operation and implement this canonicalization on that. We already have floor : https://mlir.llvm.org/docs/Dialects/MathOps/#mathfloor-mathfloorop, it should be okay to have mod too.
> 
> I usually prefer lowering to an op, and then letting the op canonicalize itself to something better, based on the lowered op's properties, than special casing the lowering, but that's a preference and I don't know what the correct way to do it is, so i'll not block this.

Typically, an op shouldn't be added to such a dialect (like `math`) without a lowering implemented to LLVM. Adding to `math` will also require a separate discussion. I wouldn't block this straightforward and elegant improvement on that. Instead, when such an op is ready to be added, with its LLVM lowering also implemented and ready, we can move the lowering to that. That won't break current paths. Canonicalization on that op would ideally be one more PR.

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