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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri 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]]
----------------
shchenz wrote:
> 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 
`-128` is `SINT_MIN` for `i8`, therefore the `sub nsw 0, -128` is UB since it overflows despite the `nsw` flag.
https://godbolt.org/z/sWekHw
Therefore any and every answer is correct.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60395