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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp:1309-1310
 
+  // ((X / C1) << C2) + X => X % C1
+  // where C1 = 1 << C2
+  const APInt *C1, *C2;
----------------
```
  // ((X / C1) << C2) + X => X % -C1
  // where -C1 = 1 << C2
```


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp:1311-1312
+  // where C1 = 1 << C2
+  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()) {
----------------
Do we care about non-splat vector support?
This will only work for scalars/splat vectors.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp:1317
+        Constant *NewRHS = ConstantInt::get(RHS->getType(), C1->abs());
+        return replaceInstUsesWith(I, Builder.CreateSRem(RHS, NewRHS, "srem"));
+      }
----------------
Just return the new `srem`.


================
Comment at: llvm/test/Transforms/InstCombine/icmp-div-constant.ll:41
 
+define i1 @is_rem32_pos_decomposed_i8(i8 %x) {
+; CHECK-LABEL: @is_rem32_pos_decomposed_i8(
----------------
Please pull into a new file. Some comments:
* there should be no `icmp` here, this isn't the pattern we are folding (although yes, that is the larger pattern)
* there shouldn't be `nsw` flags since we don't use them
* there shouldn't be `mul`, we are looking for `shl`
* there shouldn't be `sub`, we are looking for `add`

And this needs more tests, in particular:
* splat vector test
* non-splat vector test
* vector splat with undef lane test: there's two constants here, so two tests where either one of them contains `undef` lane, and a test where both of them contain undef lane
* one test with extra uses (see `@use` in this file) on every of the values - we don't care whether there are extra uses
* Negative tests - what are preconditions on the constants?


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

https://reviews.llvm.org/D79369