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

[Amaury SECHET via Phabricator via llvm-commits]

deadalnix 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))
----------------
spatel wrote:
> 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().
Good catch, I need to fix this. Thanks.


https://reviews.llvm.org/D32916