[PATCH] D35192: [ARM] Use ADDCARRY / SUBCARRY

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:12823
+    }
     // These nodes' second result is a boolean
     Known.Zero |= APInt::getHighBitsSet(BitWidth, BitWidth - 1);
----------------
rogfer01 wrote:
> efriedma wrote:
> > rogfer01 wrote:
> > > efriedma wrote:
> > > > Is this true?
> > > Maybe I do not understand what this function computes.
> > > 
> > > The result of the first operand (`Op.getResNo() == 0`) should be either 0 or 1 in the given nodes so all bits are zero but the lowermost one.
> > I was asking specifically about the comment "These nodes' second result is a boolean".  (I know it isn't new, but we should fix it while we're here.)
> My understanding is that `ISD::ADDE`/`ISD::SUBE` and  (the now deprecated) `ISD::ADDC`/`ISD::SUBC` are immediately lowered into `ARMISD::ADDE` and `ARMISD::ADDC` respectively so I think that for the second result nothing has changed. The extra check for the first is because the extra hops that we do now when lowering `ADDCARRY `/ `SUBCARRY`.
> 
> That said, you were right to point me at this part of code because I introduced a bug here, which I fixed in this update.
> 
> Sadly I'm not sure how can I test that these bits are correctly accounted for this second operand (for the first operand is easy and already caght by the regression testsuite: not knowing it will introduce spurious `and` operations to make sure only 1 bit is used).
> 
> I find difficult to generate an `ARMISD::ADDE` (using IR) where some combiner wants to check the outcome of the second operand, even more now that the patch legalizes `ADDCARRY` / `SUBCARRY` and we do some extra hops to get in and out the carry flag itself. It would be a bit worrying that this were wrong at this point because it'd mean I cannot do exactly that.
> 
> It could well be that I'm missing something obvious here.
The second result of ARMISD::SUBE isn't a boolean; it's a copy of CPSR.


https://reviews.llvm.org/D35192