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

[David Zarzycki via Phabricator via llvm-commits]

davezarzycki marked 5 inline comments as done.
davezarzycki added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2861-2864
+  if (CarryIn.getOpcode() != ISD::ZERO_EXTEND)
+    return SDValue();
+  if (CarryIn.getOperand(0).getValueType() != MVT::i1)
+    return SDValue();
----------------
lebedev.ri wrote:
> ```
>   if (CarryIn.getOpcode() != ISD::ZERO_EXTEND)
>     return SDValue();
>   CarryIn = CarryIn.getOperand(0);
>   if (CarryIn.getValueType() != MVT::i1)
>     return SDValue();
> ```
> It's better than seeing `CarryIn.getOperand(0)` later and going "wait what why we use first operand from CarryIn instruction?"
Sure. I'll change that before committing.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2815
+//            |   /    |
+//     (uaddo *, *)    \________
+//       |  \                   \
----------------
lebedev.ri wrote:
> Since you are drawing `add`, you could swap it's operands, and thus CarryIn wouldn't have to intercept PartialCarryOutX. Just a thought, not sure it matters.
I'd really rather not. The current diagram matches the UADDO/USUBO generated by CodeGenPrepare from plain IR. It also matches the UADDO/USUBO that clang emits as a part of its builtin addc/subc intrinsics. Also, if one substitutes usubo for uaddo, the diagram still holds true, whereas if the add operands were swapped, the diagram would be wrong for usubo.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70079