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

[David Zarzycki via Phabricator via llvm-commits]

davezarzycki marked 4 inline comments as done.
davezarzycki 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();
----------------
chfast wrote:
> Have you seen the `getAsCarry()` helper? It might help simplifying matching here.
As mentioned elsewhere in this thread, `getAsCarry()` would need to be updated in order to be used here. In particular, it would need become aware of this proposed optimization because the carry flag might not have been merged yet.


================
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),
----------------
chfast wrote:
> The `ADDCARRY` rather replaces `UADDO` so you should use the debug location of the `Carry1` node.
The ADDCARRY replaces three operations. Why is one node better than another for the debug location?


================
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);
----------------
chfast wrote:
> 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()`.
Okay. I'll look into that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70079