[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: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.
----------------
davezarzycki wrote:
> lebedev.ri wrote:
> > Ah, so the numbering is from bottom to top, i expected the other way around.
> I've swapped the ordering to match your expectations.
(It's just that i think this ordering is more prevalent, i've practically not seen the opposite ordering in the codebase)


================
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();
----------------
davezarzycki wrote:
> lebedev.ri wrote:
> > There's `getAsCarry()` should it be used here, or are we okay with some other `i1`?
> > (can we have some other i1 by now?)
> That function is non-recursive, so it doesn't work. In other words, it doesn't know how to handle carry flags being merged. I can change that if people are okay with that. 
No real opinion from me here.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2873
+    return Merged.getValue(1);
+  DAG.ReplaceAllUsesOfValueWith(Carry1.getValue(0), Merged.getValue(0));
+  return DAG.getConstant(0, DL, MVT::i1);
----------------
I'm not following this bit, why do we do this only for `ISD::AND`?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70079