[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:2842-2847
+  if (Carry0.getOperand(0) != Carry1.getValue(0)) {
+    if (Carry1.getOperand(0) != Carry0.getValue(0))
+      return SDValue();
+    // Canonicalize the carry in as Carry0 and the addition as Carry1.
+    std::swap(Carry0, Carry1);
+  }
----------------
davezarzycki wrote:
> lebedev.ri wrote:
> > I find this confusing.
> > Which one of Carry0 and Carry1 is the first one?
> > Can this be simplified to something like
> > ```
> > // ???
> > if (Carry1.getOperand(0) != Carry0.getValue(0))
> >   std::swap(Carry0, Carry1);
> > if (Carry1.getOperand(0) != Carry0.getValue(0))
> >   return SDValue();
> > ```
> > This would be more idiomatic..
> Sure. I can refactor that. I guess the comment wasn't clear enough. The code wants the UADDO (or USUBO) that does the primary arithmetic to be Carry0. I.e. the node at the top of the ASCII art. The UADDO (or USUBO) that does the carry/borrow in is Carry1. I.e. the node in the middle of the ASCII art.
Please mark done comments as done.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2819-2820
+// Our goal is to identify A, B, and In and produce ADDCARRY/SUBCARRY with a
+// single path for carry/borrow out propagation.
+//
+// Please note that because we have proven that the result of the UADDO/USUBO
----------------
Since you've drawn the original graph, also showing the result might be good?


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2846
+
+  // Canonicalize the carry in as Carry0 and the add/sub of A and B as Carry1.
+  // I.e. the middle and top nodes respectively in the above ASCII art.
----------------
Ah, so the numbering is from bottom to top, i expected the other way around.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2853-2855
+  unsigned ISDOp = Opcode == ISD::UADDO ? ISD::ADDCARRY : ISD::SUBCARRY;
+  if (!TLI.isOperationLegalOrCustom(ISDOp, Carry1.getValue(0).getValueType()))
+    return SDValue();
----------------
s/ISDOp/NewOp/


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2861-2865
+  // Verify that the carry/borrow in is plausibly a carry/borrow bit.
+  if (Carry0.getOperand(1).getOpcode() != ISD::ZERO_EXTEND)
+    return SDValue();
+  if (Carry0.getOperand(1).getOperand(0).getValueType() != MVT::i1)
+    return SDValue();
----------------
There's `getAsCarry()` should it be used here, or are we okay with some other `i1`?
(can we have some other i1 by now?)


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2871-2874
+  if (N->getOpcode() != ISD::AND)
+    return Merged.getValue(1);
+  DAG.ReplaceAllUsesOfValueWith(Carry0.getValue(0), Merged.getValue(0));
+  return DAG.getConstant(0, DL, MVT::i1);
----------------
This could use a few comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70079