[PATCH] D156811: [InstCombine] Fold `select` of `srem` and conditional add

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added a comment.

A note about the proofs. You proved this when the remander is 8, not generically for any power of 2. Proofs with constants should generally look more like: https://alive2.llvm.org/ce/z/9_KG6c
i.e instead of declaring with the constant is, declare the constraints you have on it.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2604
+  // %cnd = icmp slt i32 %rem, 0
+  // %add = add nsw i32 %rem, %n
+  // %sel = select i1 %cnd, i32 %add, i32 %rem
----------------
Can you remove the NSW from the comment / all the alive2 links. It makes it seem like the `nsw` is necessary for correctness.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2608
+  if (!(match(TrueVal, m_Add(m_Value(Rem), m_Power2(I0))) &&
+        match(Rem, m_SRem(m_Value(Op), m_Power2(I1))) && FalseVal == Rem &&
+        I0 == I1))
----------------
This also works for `urem`, unless that already gets properly folded, can you also support that here?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2609
+        match(Rem, m_SRem(m_Value(Op), m_Power2(I1))) && FalseVal == Rem &&
+        I0 == I1))
+    return nullptr;
----------------
Just use `m_SpecificInt(I0)` instead of `I1` and drop this equality comparison.

Although really, this doesn't need to be an integer at all, it could be:
```
if (!(match(TrueVal, m_Add(m_Value(RemRes), m_Value(Remainder))) &&
        match(RemRes, m_SRem(m_Value(Op), m_Specific(m_Value(Remainder))) && FalseVal == RemRes &&
        && isKnownToBeAPowerOfTwo(Remainder, ...)))
```


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2616
+        Pred == ICmpInst::ICMP_SLT && CmpCond == Rem))
+    return nullptr;
+
----------------
Can you move this check to before the other match. 1) think its clearer, 2) if you add the generic `isKnownToBeAPowerOfTwo` approach is good to exhaust all the cheap checks before the recursive call.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2618
+
+  Value *Mask = ConstantInt::get(Rem->getType(), I0->getZExtValue() - 1);
+  return BinaryOperator::CreateAnd(Op, Mask);
----------------
getZExtValue is really okay here, what if its an i128?
You can build a ConstantInt directly from an APInt, so just `*I0` should be okay here.


================
Comment at: llvm/test/Transforms/InstCombine/select-divrem.ll:239
+  ret i32 %sel
+}
----------------
The tests should be precommit,.

They are also insufficient. Can you add some negative cases for: incorrect sign test, add with different constant, wrong operand positions in select.

Can you also add a test that use vecs, and add one where the datatype is i128 (as your current code seems to be buggy there).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156811