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

[David Zarzycki via Phabricator via llvm-commits]

davezarzycki 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);
+  }
----------------
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.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2852-2854
+  if (Carry0.getOperand(1).getOperand(0).getValueType() != MVT::i1)
+    return SDValue();
+
----------------
lebedev.ri wrote:
> Do we care what `Carry0.getOperand(1)` is, what produces it?
We want that operand to be a carry bit (or at least plausibly so), so that we don't create bogus addcarry/subcarry nodes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70079