[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:2604
+  // %cnd = icmp slt i32 %rem, 0
+  // %add = add nsw i32 %rem, %n
+  // %sel = select i1 %cnd, i32 %add, i32 %rem
----------------
craig.topper wrote:
> 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.
Personally think with a comment the readability will be fine. If its that big a concern we should add `m_ICmpSignTest` is its a fairly common match req.


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

https://reviews.llvm.org/D156811