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

[Paweł Bylica via Phabricator via llvm-commits]

chfast added a reviewer: chfast.
chfast added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2848
+  // Verify that the carry/borrow in is plausibly a carry/borrow bit.
+  if (Carry1.getOperand(1).getOpcode() != ISD::ZERO_EXTEND)
+    return SDValue();
----------------
Have you seen the `getAsCarry()` helper? It might help simplifying matching here.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2854
+  SDLoc DL(N);
+  SDValue Merged = DAG.getNode(NewOp, DL, Carry1->getVTList(),
+                               Carry0.getOperand(0), Carry0.getOperand(1),
----------------
The `ADDCARRY` rather replaces `UADDO` so you should use the debug location of the `Carry1` node.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2873
+    return Merged.getValue(1);
+  DAG.ReplaceAllUsesOfValueWith(Carry1.getValue(0), Merged.getValue(0));
+  return DAG.getConstant(0, DL, MVT::i1);
----------------
davezarzycki wrote:
> lebedev.ri wrote:
> > I'm not following this bit, why do we do this only for `ISD::AND`?
> > 
> As the comment above tries to explain, we can prove in this diamond UADDO/USUBO scenario that it is impossible for both UADDO/USUBO nodes to overflow at the same time. Either one does, or the other, therefore using AND to merge the carry flags will always return zero.
I think you should use `CombineTo()` here to bump stats counters, produce some debug logs, updated worklist and delete replaced nodes.

It can be something like:
```
CombineTo(Carry1.getNode(), Merged.getValue(0), undef);
return CombineTo(N, Merged.getValue(1));
```
Both `Carry1` and `N` should be deleted by `CombineTo()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70079