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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri accepted this revision.
lebedev.ri added a comment.
This revision is now accepted and ready to land.

With latest update i think this is basically LG, last few non-functional comments.
But please wait a bit for @deadalnix / @craig.topper / @RKSimon.

In D70079#1753410 <https://reviews.llvm.org/D70079#1753410>, @davezarzycki wrote:

> This patch fixes a bug introduced during the last round of feedback. By making the patch robust against the optimizer swapping operands to UADDO, the patch started tolerating the carry in bit being on the lefthand side of USUBO. That is wrong, and it's now fixed.


Thank you! I was just figuring out if that bug *wasn't* there.



================
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();
----------------
```
  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?"


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2869
+                               Carry0.getOperand(0), Carry0.getOperand(1),
+                               CarryIn.getOperand(0));
+
----------------
CarryIn);


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2815
+//            |   /    |
+//     (uaddo *, *)    \________
+//       |  \                   \
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70079