[PATCH] D32916: [DAGCombine] (addcarry 0, 0, X) -> (ext/trunc X)

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2146
+
+  // fold (addcarry 0, 0, X) -> (ext/trunc X) and no carry.
+  if (isNullConstant(N0) && isNullConstant(N1))
----------------
deadalnix wrote:
> RKSimon wrote:
> > I'm not sure if you can do this - aren't carry bits boolean types? So won't they be TargetLowering::BooleanContent? 
> > 
> > getBoolExtOrTrunc doesn't quite work - you're wanting a 0 or 1 result......
> No they are whatever the target decides to use as boolean. All i1 are one after legalization.
> 
> I'm not sure, however, what's the correct way to extends this. What's wrong with getBoolExtOrTrunc ?
The bug may not be visible if x86 is the only target to allow ISD::ADDCARRY so far because x86 has "ZeroOrOneBooleanContent" for scalars.

So we're seeing something like this:
        t23: i64,i8 = uaddo t2, t4
      t24: i64,i8 = addcarry Constant:i64<0>, Constant:i64<0>, t23:1

The 2nd result of the uaddo is an i8 which is "a boolean that indicates if an overflow occurred".
Now, if the target has "ZeroOrNegativeOneBooleanContent", that result will be "i8 -1" when overflow occurred for the uaddo. If you then use getBoolExtOrTrunc() on that to get the i64, you'll get a sign-extend to i64 -1. That's wrong - we wanted a "i64 1" as the first result of the addcarry. We must mask ('and x, 1') or trunc the CarryIn operand before casting it to N0.getValueType().


https://reviews.llvm.org/D32916