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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

Almost LG to me, but apparently i have more comments.



================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2812
+//            |          \
+//            |    (uaddo *, In)
+//            |       /
----------------
Wait, In is some third carry-in bit, right? This diagram doesn't really make that obvious.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2820
+//
+//    (addcarry A, B, In):Out
+//
----------------
Out = (addcarry A, B, In).carry


================
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();
----------------
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`.


================
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();
----------------
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?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70079