[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);
----------------
chfast wrote:
> lebedev.ri wrote:
> > 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.
> To my understanding `CombineTo()` is a rich version of RAUW. It does RAUW, but also can add new nodes to the DAG Combiner worklist, updates DAG combiner stats, prints debug logs about which nodes has been combined into which, handles removed nodes.
I would think simply returning the new value would be enough,
and whatever code called us will do all the same anyway?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70079