[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:
> 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"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70079