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

[David Zarzycki via Phabricator via llvm-commits]

davezarzycki marked an inline comment as done.
davezarzycki added inline comments.


================
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);
----------------
lebedev.ri wrote:
> 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.
There is a test for the zero case. See `bogus_add_U320_without_i128_and` in addcarry.ll.

In any case, I'll switch to `CombineTo`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70079