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

Eli Friedman via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri May 26 11:16:56 PDT 2017


efriedma 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:
> 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.
> and validate that the flag is of the form 0/1 ot that a (and X, 1) is present along the way.

I don't see any code in the current version to do this?


https://reviews.llvm.org/D32925





More information about the llvm-commits mailing list