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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
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),
----------------
davezarzycki wrote:
> 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?
I'd say the current debuginfo is correct.


================
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:
> 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.
> 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.

Yes, but that wasn't the question i was asking.
My question was - what happens with uses of `Carry1.getValue(0)` if we have non-AND?
Since you don't replace, they will still use old nodes, 
which means the old instruction chain will stay?
I think this may missing more tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70079