[PATCH] D70079: [SelectionDAG] Combine U{ADD,SUB}O diamonds into {ADD,SUB}CARRY

[David Zarzycki via Phabricator via llvm-commits]

davezarzycki marked 10 inline comments as done.
davezarzycki added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2812
+//            |          \
+//            |    (uaddo *, In)
+//            |       /
----------------
lebedev.ri wrote:
> Wait, In is some third carry-in bit, right? This diagram doesn't really make that obvious.
No. There is one carry in bit in this graph: `In`. There are two carry out bits being merged in a way that implies an addcarry or subcarry is possible. I can try to make this more obvious.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2820
+//
+//    (addcarry A, B, In):Out
+//
----------------
lebedev.ri wrote:
> Out = (addcarry A, B, In).carry
Sure. In the next patch.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2837-2840
+  if (Carry1.getOperand(0) != Carry0.getValue(0))
+    std::swap(Carry0, Carry1);
+  if (Carry1.getOperand(0) != Carry0.getValue(0))
+    return SDValue();
----------------
lebedev.ri wrote:
> Why can `Carry0.getValue(0)` only be in `Carry1.getOperand(0)`, but not in `Carry1.getOperand(1)`?
> I think there is no such restriction at least for `add`.
Sure. I'll make it more robust in the next patch. For whatever it may be worth, this is the order output by CodeGenPrepare when creates implicit UADDO/USUBO nodes. It's also happens to be the order that clang generates via its addc/subc builtins.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2846-2848
+  // Verify that the add/sub has only one use of its respective results.
+  if (!Carry0->hasNUsesOfValue(1, 0) || !Carry0->hasNUsesOfValue(1, 1))
+    return SDValue();
----------------
lebedev.ri wrote:
> We are replacing `and`/`or`/`xor` node, and we only create a single replacement node,
> so i don't think we need any one-use checks here?
I was trying to be conservative. After some thought, I think dropping the one-use tests is harmless. I'll remove them from the next patch.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2849
+    return SDValue();
+
+  // Verify that the carry/borrow in is plausibly a carry/borrow bit.
----------------
lebedev.ri wrote:
> I think it will be helpful to
> ```
> SDValue CarryBit = Carry1.getOperand(1);
> ```
Sure. In the next patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70079/new/

https://reviews.llvm.org/D70079