[PATCH] D79369: [InstCombine] "X - (X / C) * C == 0" to "X & C-1 == 0"

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

Please feel free to add weekly-ish "ping" comments, i lost track of this, sorry :/
This looks good for the time being modulo nits.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp:1309-1310
 
+  // ((X / C1) << C2) + X => X % C1
+  // where C1 = 1 << C2
+  const APInt *C1, *C2;
----------------
lebedev.ri wrote:
> ```
>   // ((X / C1) << C2) + X => X % -C1
>   // where -C1 = 1 << C2
> ```
Eeeh, i walked into that one myself, but let's explicitly spell out sigdness of ops:
`((X s/ C1) << C2) + X => X s% -C1`



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp:1312
+  const APInt *C1, *C2;
+  if (match(LHS, m_Shl(m_SDiv(m_Value(A), m_APInt(C1)), m_APInt(C2)))) {
+    if ((RHS == A) && C1->abs().isPowerOf2() && C1->isNegative()) {
----------------
`if (match(LHS, m_Shl(m_SDiv(m_Specific(RHS), m_APInt(C1)), m_APInt(C2)))) {`


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp:1313-1315
+    if ((RHS == A) && C1->abs().isPowerOf2() && C1->isNegative()) {
+      APInt one(C2->getBitWidth(), 1);
+      if ((C1->abs() == one.shl(*C2)) && C2->sgt(one)) {
----------------
I'm not sure why all these preconditions are needed?
I think you only want `-C1 == 1 << C2`, i.e. `-(*C1) == (one << *C2)`.
https://rise4fun.com/Alive/wslBO


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp:1316
+      if ((C1->abs() == one.shl(*C2)) && C2->sgt(one)) {
+        Constant *NewRHS = ConstantInt::get(RHS->getType(), C1->abs());
+        return BinaryOperator::CreateSRem(RHS, NewRHS);
----------------
Why not just `-(*C1)`?
While there, we now do that twice, so cache it in some new variable.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79369/new/

https://reviews.llvm.org/D79369