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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:1985
+         (V.getOpcode() == ISD::AND && isOneConstant(V.getOperand(1))))
+      V = V.getOperand(0);
+
----------------
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.


https://reviews.llvm.org/D32925