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

[Roger Ferrer Ibanez via Phabricator via llvm-commits]

rogfer01 added a comment.

Hi @efriedma, thanks again for the comments. I have updated the diff with your suggestions.

Please find a comment for the `ARMTargetLowering::computeKnownBitsForTargetNode`, I think it is now OK but I will appreciate some more insight.

Kind regards,
Roger



================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:12823
+    }
     // These nodes' second result is a boolean
     Known.Zero |= APInt::getHighBitsSet(BitWidth, BitWidth - 1);
----------------
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.


https://reviews.llvm.org/D35192