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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


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


================
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
----------------
antoniofrighetto wrote:
> craig.topper wrote:
> > craig.topper wrote:
> > > craig.topper wrote:
> > > > antoniofrighetto wrote:
> > > > > 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.
> > > > Use `InstCombiner::isSignBitCheck` so you can handle the case where the condition is inverted and the add and srem operands are swapped.
> > > Or does something already canonicalize to have the compare be ICMP SLT?
> > Ok there is a canonicalization inside `InstCombinerImpl::foldSelectInstWithICmp` to prefer the SLT form. But it only triggers if the select is the only use of the condition.
> > 
> > This srem transform doesn't have any one use checks. So we might not want to rely on that canonicalization and should use `InstCombiner::isSignBitCheck` to handle both cases.
> I suppose this would require to have something like:
> ```
>   ConstantInt *RHS;
>   bool TrueIfSigned = false;
> 
>   if (!(match(CondVal, m_ICmp(Pred, m_Value(RemRes), m_ConstantInt(RHS))) &&
>         IC.isSignBitCheck(Pred, RHS->getValue(), TrueIfSigned)))
> ```
> And possibly also continue checking that `RHS->getZExtValue() == 0`. Wouldn't this make the code a bit slightly less readable? Considering the canonicalization, maybe we could use `m_OneUse` instead?
Should be `m_APInt` instead of `m_ConstantInt` to support vector splats. See `foldSelectToCopysign` for similar. I don't think you need RHS->getZExtValue() == 0, since the SGT form uses -1 instead of 0.


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

https://reviews.llvm.org/D156811