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

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2616
+        FalseVal == RemRes &&
+        IC.isKnownToBeAPowerOfTwo(Remainder, /*OrZero*/ true)))
+    return nullptr;
----------------
craig.topper wrote:
> This uses `isKnownToBeAPowerOfTwo`, but the tests only check for constants. `isKnownToBeAPowerOfTwo` handles more than that.
> This uses `isKnownToBeAPowerOfTwo`, but the tests only check for constants. `isKnownToBeAPowerOfTwo` handles more than that.

+1


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2616
+        Pred == ICmpInst::ICMP_SLT && CmpCond == Rem))
+    return nullptr;
+
----------------
antoniofrighetto wrote:
> goldstein.w.n wrote:
> > antoniofrighetto wrote:
> > > goldstein.w.n wrote:
> > > > 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.
> > > Moved above, and moved from using `m_Power2` to the generic `isKnownToBeAPowerOfTwo`.
> > Please update you proofs to replace `8` with a variable `C` and use assume to provide its constraints (in your case pow2 or zero).
> I had tried with something [[ https://alive2.llvm.org/ce/z/_NfVxt | similar ]], but i128 seems to lead Alive2 to timeout, and there's no assume for vectors. Updated proofs with [[ https://alive2.llvm.org/ce/z/9_KG6c | this ]], if it suffices.
> I had tried with something [[ https://alive2.llvm.org/ce/z/_NfVxt | similar ]], but i128 seems to lead Alive2 to timeout, and there's no assume for vectors. Updated proofs with [[ https://alive2.llvm.org/ce/z/9_KG6c | this ]], if it suffices.

Yeah, there is noneed to use i128 in the alive2 proofs, just having a test for it is fine.


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

https://reviews.llvm.org/D156811