[CodeGen][SelectionDAG] Fix tiny bug in ExpandIntRes_UADDSUBO

[ibookstein]

Summary

Ternary expression checks for ISD::ADD instead of ISD::UADDO inside DAGTypeLegalizer::ExpandIntRes_UADDSUBO.
This means the ternary expression will evaluate to ISD::SUBCARRY for both ISD::UADDO and ISD::USUBO nodes.
Targets are likely to implement both, so impact will be very limited in practice.

[ibookstein]

Changed diff to be -U999999

[lebedev.ri]

Test missing.

[ibookstein]

As this would only affect a hypothetical target that supports ISD::SUBCARRY but not ISD::ADDCARRY, testing this via IR would seem overkill.
I think the correct level of testing for this would be UT for DAGTypeLegalizer, but the closest thing I could find was some of the things under llvm/unittests/CodeGen, like AArch64SelectionDAGTest.cpp.
Does that sound like it's in the right direction?

[lebedev.ri]

Two remarks:

• Code duplication
• If this is purely theoretical, can the dead code be simply dropped?

llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp

2306–2307

this

2335–2336

this

2343–2344

and this do basically the same thing.
At the very least you want to precompute which carry and non-carry opcodes you'll want to use, thus avoiding the bug altogether.

[ibookstein]

Is this along the lines of what you suggested?

[lebedev.ri]

That'll do, thanks!

[ibookstein]

I failed to mention during the review that I don't have commit access, so could anyone commit this on my behalf?

[ibookstein]

Ping
Is there anything else I should/could to to land this?

[lebedev.ri]

Right.
Now that we have migrated to git, can you paste your Author: name <[e-mail]> that should be used for the patch?

[ibookstein]

Yeah sure, thanks!
Author: Itay Bookstein <[e-mail]>

llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp

 
 void DAGTypeLegalizer::ExpandIntRes_UADDSUBO(SDNode *N,
                                              SDValue &Lo, SDValue &Hi) {
   SDValue LHS = N->getOperand(0);
   SDValue RHS = N->getOperand(1);
   SDLoc dl(N);
 
   SDValue Ovf;
 
-  bool HasOpCarry = TLI.isOperationLegalOrCustom(
-      N->getOpcode() == ISD::ADD ? ISD::ADDCARRY : ISD::SUBCARRY,
-      TLI.getTypeToExpandTo(*DAG.getContext(), LHS.getValueType()));
+  unsigned CarryOp, NoCarryOp;
+  ISD::CondCode Cond;
+  switch(N->getOpcode()) {
+    case ISD::UADDO:
+      CarryOp = ISD::ADDCARRY;
+      NoCarryOp = ISD::ADD;
+      Cond = ISD::SETULT;
+      break;
+    case ISD::USUBO:
+      CarryOp = ISD::SUBCARRY;
+      NoCarryOp = ISD::SUB;
+      Cond = ISD::SETUGT;
+      break;
+    default:
+      llvm_unreachable("Node has unexpected Opcode");
+  }
 
-  if (HasOpCarry) {
+  bool HasCarryOp = TLI.isOperationLegalOrCustom(
+      CarryOp, TLI.getTypeToExpandTo(*DAG.getContext(), LHS.getValueType()));
+
+  if (HasCarryOp) {
     // Expand the subcomponents.
     SDValue LHSL, LHSH, RHSL, RHSH;
     GetExpandedInteger(LHS, LHSL, LHSH);
     GetExpandedInteger(RHS, RHSL, RHSH);
     SDVTList VTList = DAG.getVTList(LHSL.getValueType(), N->getValueType(1));
     SDValue LoOps[2] = { LHSL, RHSL };
     SDValue HiOps[3] = { LHSH, RHSH };
 
-    unsigned Opc = N->getOpcode() == ISD::UADDO ? ISD::ADDCARRY : ISD::SUBCARRY;
     Lo = DAG.getNode(N->getOpcode(), dl, VTList, LoOps);
     HiOps[2] = Lo.getValue(1);
-    Hi = DAG.getNode(Opc, dl, VTList, HiOps);
+    Hi = DAG.getNode(CarryOp, dl, VTList, HiOps);
 
     Ovf = Hi.getValue(1);
   } else {
     // Expand the result by simply replacing it with the equivalent
     // non-overflow-checking operation.
-    auto Opc = N->getOpcode() == ISD::UADDO ? ISD::ADD : ISD::SUB;
-    SDValue Sum = DAG.getNode(Opc, dl, LHS.getValueType(), LHS, RHS);
+    SDValue Sum = DAG.getNode(NoCarryOp, dl, LHS.getValueType(), LHS, RHS);
     SplitInteger(Sum, Lo, Hi);
 
     // Calculate the overflow: addition overflows iff a + b < a, and subtraction
     // overflows iff a - b > a.
-    auto Cond = N->getOpcode() == ISD::UADDO ? ISD::SETULT : ISD::SETUGT;
     Ovf = DAG.getSetCC(dl, N->getValueType(1), Sum, LHS, Cond);
   }
 
   // Legalized the flag result - switch anything that used the old flag to
   // use the new one.
   ReplaceValueWith(SDValue(N, 1), Ovf);
 }