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

[Antonio Frighetto via Phabricator via llvm-commits]

antoniofrighetto added a comment.

Thanks for reviewing it, and for the note! Updated proofs & changes, and rebased.



================
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
----------------
goldstein.w.n wrote:
> Can you remove the NSW from the comment / all the alive2 links. It makes it seem like the `nsw` is necessary for correctness.
Dropped, thanks.


================
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))
----------------
goldstein.w.n wrote:
> goldstein.w.n wrote:
> > This also works for `urem`, unless that already gets properly folded, can you also support that here?
> If `urem` already gets properly folded, then if its in some clear place can you just update that codes to work for the srem case as well (instead of adding an entirely new sequence for srem).
`urem` is directly folded into an `and` upon entering `visitURem` as it can only take positive values. `srem` requires dedicated handling instead.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2616
+        Pred == ICmpInst::ICMP_SLT && CmpCond == Rem))
+    return nullptr;
+
----------------
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`.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2618
+
+  Value *Mask = ConstantInt::get(Rem->getType(), I0->getZExtValue() - 1);
+  return BinaryOperator::CreateAnd(Op, Mask);
----------------
goldstein.w.n wrote:
> 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.
Right, `getZExtValue` does not really look good when handling `i128`. Moved to using `getAllOnesValue`, thanks.


================
Comment at: llvm/test/Transforms/InstCombine/select-divrem.ll:239
+  ret i32 %sel
+}
----------------
goldstein.w.n wrote:
> 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).
Tests updated, I'll be pre-comitting them should they look OK.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156811