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

[Amaury SECHET via Phabricator via llvm-commits]

deadalnix added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:1985
+         (V.getOpcode() == ISD::AND && isOneConstant(V.getOperand(1))))
+      V = V.getOperand(0);
+
----------------
spatel wrote:
> efriedma wrote:
> > spatel wrote:
> > > 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.
> > > 
> > > 
> > "X + (sext Carry)" clearly produces a different result from "X + (zext carry)"; I can't see how it makes sense to ignore the difference.
> Ah, right. So yes, Simon's observation was correct. We need to match whatever this node is to the boolean content type. 
> 
> So I think that's more reason to make specific folds for the actual patterns that are causing problems for this codegen.
It has to match the bool type and this one is correct. But if the bool type isn't 1/0 the end result is different. We need to avoid matching SEXT and ANY_EXT here and validate that the flag is of the form 0/1 ot that a (and X, 1) is present along the way.


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


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2042
 
+  // (add X, Carry) -> (addcarry X, 0, Carry)
+  if (auto Carry = getAsCarry(N1))
----------------
spatel wrote:
> 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.
In practice this is better. When taking the carry as an argument, it is necessary to materialize it, which takes a register and an instruction to copy the carry from the flag register to a general purpose register. On the other hand, when passing the carry down as carry, there are usually some predicated instruction that can be used.

In addition, there are various combine you can do on top of that form, for instance if X is (add A, B) you end up with (addcarry A, B, Carry) which is preferable.


https://reviews.llvm.org/D32925