[PATCH] D124976: [AArch64] Fix sub with carry

[Kazu Hirata via Phabricator via llvm-commits]

kazu requested review of this revision.
kazu added a comment.

I've fixed the treatment of the incoming carry as well as `foldOverflowCheck`.  Please take a look.  Thanks!



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:3366
+      IsSigned ? overflowFlagToValue(Sum.getValue(1), VT1, DAG)
+               : carryFlagToValue(Sum.getValue(1), VT1, DAG, InvertCarry);
 
----------------
efriedma wrote:
> efriedma wrote:
> > It's a bit concerning to me that you're changing OutFlag, but not OpCarryIn.  If we're inverting the output flag, don't we need to invert the input flag too?  Or is it already getting inverted correctly?
> > 
> > It might make sense to also add tests for i256 addition and subtraction.
> Just checked; this is what we currently generate for i256 subtraction:
> 
> ```
>         subs    x0, x0, x4
>         sbcs    x1, x1, x5
>         cset    w8, hs
>         cmp     w8, #1
>         sbcs    x2, x2, x6
>         cset    w8, hs
>         cmp     w8, #1
>         sbcs    x3, x3, x7
>         ret
> ```
> 
> I guess maybe the input is already handled correctly.
It turns out that we have to invert the incoming carry, too.

We appear to generate correct code -- `subs` immediately followed by `sbcs`, but that's because wrong semantics is canceled by another piece of wrong semantics.  `lowerADDSUBCARRY` generates wrong code, taking in the incoming carry and feeding it  to take in the incoming carry without inverting it.  Meanwhile, `foldOverflowCheck` expects the wrong code from `lowerADDSUBCARRY` and folds it away.

That is, if you disable `foldOverflowCheck` by teaching it to always return `SDValue()`, then you would get wrong code between `subs and `sbcs`.

In the latest revision, of the patch I've corrected both -- the treatment of the incoming carry and the folding done in `foldOverflowCheck`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124976