[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: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:
> > 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.


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

https://reviews.llvm.org/D156811