[PATCH] D143883: [InstCombine] canonicalize urem as cmp+select

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/test/Transforms/InstCombine/urem-via-cmp-select.ll:105
+start:
+  %cmp = icmp ult i64 %x, %n
+  %add = add i64 %x, 1
----------------
efriedma wrote:
> nikic wrote:
> > %cmp is a dead instruction here. The request wasn't to just drop the assume, but to establish the `%x < %n` precondition without using one. It's a bit tricky to do this without also triggering other folds, but maybe this would work?
> > ```
> > define i64 @test(i64 %arg) {
> >   %x = shl nuw i64 1, %arg
> >   %n = shl nuw i64 3, %arg
> >   %add = add i64 %x, 1
> >   %out = urem i64 %add, %n
> >   ret i64 %out
> > }
> > ```
> > Unfortunately this example still doesn't show the need for freeze (https://alive2.llvm.org/ce/z/FJCdt4) ... I'm pretty sure it's needed in the general case, but I'm not sure how to come up with an example.
> If I'm understanding your question correctly, the following demonstrates the need for freeze.
> 
> ```
> define i8 @src(i8 %arg, i8 %arg2) {
>   %x = urem i8 %arg, %arg2
>   %add = add i8 %x, 1
>   %out = urem i8 %add, %arg2
>   ret i8 %out
> }
> 
> define i8 @tgt(i8 %arg, i8 %arg2) {
>   %xx = urem i8 %arg, %arg2
>   %x = freeze i8 %xx
>   %add = add i8 %x, 1
>   %cmp = icmp eq i8 %add, %arg2
>   %out = select i1 %cmp, i8 0, i8 %add
>   ret i8 %out
> }
> ```
Thanks, that's exactly what I was looking for!


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

https://reviews.llvm.org/D143883