[PATCH] D32925: [DAGCombine] (add/uaddo X, Carry) -> (addcarry X, 0, Carry)

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a subscriber: efriedma.
spatel added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:1985
+         (V.getOpcode() == ISD::AND && isOneConstant(V.getOperand(1))))
+      V = V.getOperand(0);
+
----------------
deadalnix wrote:
> RKSimon wrote:
> > As was discussed on D32916, the carry is a boolean that will respect TargetLowering::BooleanContent so we can't just accept any extension - it has to match the bool type.
> This just follow the chain to see if there is a carry generating ops at the end. This isn't generating anything, so I'm not sure what the problem is.
In this case, we're taking a boolean carry value and using it as an operand for a new ADDCARRY node that has the same boolean content requirement, so I think this part is ok. 

Given that it's not clear from reading the code, please add a comment to explain the logic.

cc'ing @efriedma in case he sees a hole in that reasoning.




================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:1982
+
+  // If this is a carry, return.
+  if (V.getResNo() != 1)
----------------
Shouldn't this be: "If this is *not* a use of a carry bit, return." ?


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2042
 
+  // (add X, Carry) -> (addcarry X, 0, Carry)
+  if (auto Carry = getAsCarry(N1))
----------------
Just looking at that transform formula, it's not clear to me why it makes sense. We're turning a simple op into a more complicated op and creating a new constant operand.

Is it because there will always be a trunc/extend/mask op sitting between the carry and the add? Should we check or assert that?

In the first test case, we're going to eventually fold the addcarry with 0 operand with a uaddo. Would it make sense to have that fold directly? 

I don't have a sense of how these folds work together, so if we do need this more general fold, we should have a comment to explain why it is generally good.


https://reviews.llvm.org/D32925