[PATCH] D60395: [InstCombine] Canonicalize (-X s/ Y) to -(X s/ Y)

[ChenZheng via Phabricator via llvm-commits]

shchenz marked 2 inline comments as done.
shchenz added inline comments.


================
Comment at: llvm/test/Transforms/InstCombine/sdiv-canonicalize.ll:7
+; CHECK-NEXT:    [[SDIV1:%.*]] = sdiv i32 [[X:%.*]], [[Y:%.*]]
+; CHECK-NEXT:    [[SDIV:%.*]] = sub i32 0, [[SDIV1]]
 ; CHECK-NEXT:    ret i32 [[SDIV]]
----------------
lebedev.ri wrote:
> Shouldn't this `sub` also be `nsw`?
> ```
> ----------------------------------------
> Optimization: nsw preserved
> Precondition: true
>   %o0 = sub nsw i8 0, %x
>   %r = sdiv i8 %o0, %y
> =>
>   %n0 = sdiv i8 %x, %y
>   %r = sub nsw i8 0, %n0
> 
> Done: 1
> Optimization is correct!
> 
> ----------------------------------------
> Optimization: exact preserved
> Precondition: true
>   %o0 = sub nsw i8 0, %x
>   %r = sdiv exact i8 %o0, %y
> =>
>   %n0 = sdiv exact i8 %x, %y
>   %r = sub i8 0, %n0
> 
> Done: 1
> Optimization is correct!
> 
> ----------------------------------------
> Optimization: both preserved
> Precondition: true
>   %o0 = sub nsw i8 0, %x
>   %r = sdiv exact i8 %o0, %y
> =>
>   %n0 = sdiv exact i8 %x, %y
>   %r = sub nsw i8 0, %n0
> 
> Done: 1
> Optimization is correct!
> 
> ```
I have one question here before I set `sub` with `nsw`.
```
  %o0 = sub nsw i8 0, %x
  %r = sdiv i8 %o0, %y
=>
  %n0 = sdiv i8 %x, %y
  %r = sub i8 0, %n0
```

Is this a valid transformation? I know we get a affirmative answer in https://rise4fun.com/Alive/9G4. But assume this input `%x` is -128, `%y` is -2, for the source `%o0` is a position value so `%r` is a position value too. But for the target `%n0` is 64, and `%r` is -64? So source `%r` and target `%r` is not equal? Can you please help to point out what's wrong here? Thanks @lebedev.ri @spatel 


================
Comment at: llvm/test/Transforms/InstCombine/sdiv-canonicalize.ll:11
   %neg = sub nsw i32 0, %x
   %sdiv = sdiv i32 %neg, %y
   ret i32 %sdiv
----------------
lebedev.ri wrote:
> There is a test that will show that we propagate `exact`?
will add one.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60395