[PATCH] D32756: [DAGCombine] Refactor common addcarry pattern.

[Amaury SECHET via Phabricator via llvm-commits]

deadalnix added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2175
+   *                  /           \
+   * (addcarry 0, 0, *)  (addcarry *, 0, Z)
+   *                 \        /
----------------
spatel wrote:
> I like the ascii art!
> 
> I'm still not very familiar with the carry ops, so I would've kept the formulas from the previous rev in here too. This is complicated enough that I think it's justified to have both.
> 
> But I have a question about this node: (addcarry 0, 0, *)
> Can that be simplified to a cast instruction from bool to the VT of the addcarry?
> If yes, if we add that simplification, will it break this pattern matching?
I think you are correct. Let me do a patch with (addcarry 0, 0, X) to (extend/trunc X) and see where this goes.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2188
+      CarryIn.getOpcode() == ISD::ADDCARRY &&
+      isNullConstant(CarryIn.getOperand(1))) {
+    auto Y = CarryIn.getOperand(0);
----------------
filcab wrote:
> Shouldn't we allow both `(addcarry *, 0, Z)` and `(addcarry 0, *, Z)`?
> Same for the final one, we could have X+0(+C) or 0+X(+C), no?
The method is called with argument in both order already. See line 2146 onward.


https://reviews.llvm.org/D32756