[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: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:
> > 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`
FWIW the previous version (no `CombineTo()`, only a singled RAUW call) looked more better to //me//,
but maybe i'm missing the point as to why that was wrong.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2874-2875
+    return Combiner.CombineTo(N, Merged.getValue(1));
+  Combiner.CombineTo(Carry1.getNode(), Merged.getValue(0),
+                     DAG.getUNDEF(Merged.getValue(1).getValueType()));
+  return Combiner.CombineTo(N, DAG.getConstant(0, DL, MVT::i1));
----------------
I'm sorry, i still don't understand why this is being done only in `ISD::AND` case.
Please explain that in a new comment in the code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70079