[PATCH] D38942: [DAG] Promote ADDCARRY / SUBCARRY

[Roger Ferrer Ibanez via Phabricator via llvm-commits]

rogfer01 added a comment.

We want to do here is to calculate the `ADDCARRY` / `SUBCARRY ` with a wider type and then use that result to compute the carry/borrow. I think that by sign extension of the operands, the carry/borrow of wider operation should exactly be the same as in a narrower type:

- to generate carry in a case like `ADDCARRY x, y` at least one of them must have the most significant bit (n-1) set (if the most significant bit is not set the largest number we can represent is 2ⁿ⁻²-1, if we add it twice we get 2ⁿ⁻¹-2 which does not generate carry). This means that, in the case of carry, at least one of the operands will be all ones in its higher (after sign extension), these higher bits should have the effect of propagating the carry of the narrow operation to the carry of the wider operation.
- to generate a borrow in a case like `SUBCARRY x, y` we need `x < y` and this property should be preserved by sign extension.

@efriedma from your first comment above it looks like I'm plain wrong. Mind to shed some light on this? Thanks! :)



================
Comment at: lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:777-778
+
+  SDValue LHS = GetPromotedInteger(N->getOperand(0));
+  SDValue RHS = GetPromotedInteger(N->getOperand(1));
+
----------------
efriedma wrote:
> jgreenhalgh wrote:
> > rogfer01 wrote:
> > > efriedma wrote:
> > > > rogfer01 wrote:
> > > > > This looks like a lot like the "dual" of `PromoteIntRes_Overflow` but replacing the users of `:1` (instead of `:0`). It generates correct code in Arm but I may be missing some subtlety here.
> > > > Yes, you're missing a very important piece here: the output carry bit is wrong.  You have to check whether the addition would overflow in the narrow type, not just whether it would overflow in the promoted type.
> > > > 
> > > > What code is producing an ADDCARRY like this?
> > > Ah! Of course!
> > > 
> > > Thanks a lot Eli. This will need fixing. 
> > > 
> > > A testcase (found by csmith)
> > > ```
> > > unsigned a, b, e;
> > > void fn1() {
> > >   long d = (a + b < b) - e;
> > >   short c = d + 1;
> > >   if (c)
> > >    for (;;)
> > >    ;
> > > }
> > > ```
> > I'm not sure this comment is correct. Because you're sign extending, you want to treat the narrow mode as a signed value, which means you only have 3 cases to consider:
> > 
> > 1) Addition of two negative numbers - In two's compliment this will always set the carry flag. Sign extension would leave a 1 in the most significant bit for both widened input values, and so addition in the wider mode would also always set the carry flag.
> > 
> > 2) Addition of two positive numbers - In two's compliment this can never set the carry flag in the narrow mode. Sign extension leave a 0 in the two most significant bits for both widened input values, and so addition in the wider mode would also never set the carry flag.
> > 
> > 3) Addition of a positive number and a negative number - This will sometimes set the carry flag in the narrow mode. The insight here is that the negative number will propagate 1 to all widened bits when sign extended, without changing the bits of the two narrow values. If a carry would be generated from the narrow values, this would also propagate through the upper bits of the widened addition, and a carry would be generated. If no carry is generated from addition in the narrow mode, then no carry is generated in the wider mode.
> > What code is producing an ADDCARRY like this?
> 
> I meant, what code in the ARM backend is producing an ISD::ADDCARRY node with an illegal type?
I think this is wrong and should be

```lang=cpp
SDValue LHS = SExtPromotedInteger(N->getOperand(0));
SDValue RHS = SExtPromotedInteger(N->getOperand(1));
```

because I pretend to use the extended bits (for the carry/borrow propagation) but then I get an apparently redundant (at least in this test case) sign extension from i16 to i32.


https://reviews.llvm.org/D38942