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

[Antonio Frighetto via Phabricator via llvm-commits]

antoniofrighetto added inline comments.


================
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
----------------
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?


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

https://reviews.llvm.org/D156811