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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri marked an inline comment as done.
lebedev.ri added inline comments.


================
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));
----------------
davezarzycki wrote:
> lebedev.ri wrote:
> > lebedev.ri wrote:
> > > 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.
> > To be specific, i don't understand why this isn't:
> > ```
> > DAG.ReplaceAllUsesOfValueWith(Carry1.getValue(0), Merged.getValue(0)); // debatable, but done always
> > if (N->getOpcode() == ISD::AND) // Both carrys can't overflow at the same time
> >   return DAG.getConstant(0, DL, MVT::i1);
> > return Merged.getValue(1);
> > ```
> That also works. I'll switch to that.
> 
> Why in your mind is the first line of your suggestion "debatable"?
Because i do not understand why currently `Combiner.CombineTo()` is used,
maybe using `DAG.ReplaceAllUsesOfValueWith()` would be incorrect there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70079