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

[David Zarzycki via Phabricator via llvm-commits]

davezarzycki marked 2 inline comments as done.
davezarzycki 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));
----------------
lebedev.ri wrote:
> 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.
I tried this:
```
-  Combiner.CombineTo(Carry1.getNode(), Merged.getValue(0),
-                     DAG.getUNDEF(Merged.getValue(1).getValueType()));
+  DAG.ReplaceAllUsesOfValueWith(Carry1.getValue(0), Merged.getValue(0));
   if (N->getOpcode() == ISD::AND)
-    return Combiner.CombineTo(N, DAG.getConstant(0, DL, MVT::i1));
-  return Combiner.CombineTo(N, Merged.getValue(1));
+    return DAG.getConstant(0, DL, MVT::i1);
+  return Merged.getValue(1);
```
And it also worked. Who is responsible for this code and decide which approach is preferable?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70079