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

[Eli Friedman via Phabricator via llvm-commits]

efriedma 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
----------------
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
}
```


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

https://reviews.llvm.org/D143883