[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: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:
> > 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?
Ping. @RKSimon @spatel @craig.topper – As the top three committers to DAGCombiner.cpp in the last two years, do you have any preference about `DAG.ReplaceAllUsesOfValueWith(…)` versus `Combiner.CombineTo(…)`? Otherwise I believe that all feedback has been addressed to date. What's blocking approval? Thanks! :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70079