[PATCH] D123779: [AArch64] Add `foldOverflowCheck` DAG combine

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:15427-15430
+static bool isCMP(SDValue Op) {
+  return Op.getOpcode() == AArch64ISD::SUBS &&
+         !Op.getNode()->hasAnyUseOfValue(0);
+}
----------------
What's the value provided here? because I only see a single use.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:15435
+static Optional<AArch64CC::CondCode> getCSETCondCode(SDValue Op) {
+  if (Op.getOpcode() == AArch64ISD::CSEL) {
+    auto CC = static_cast<AArch64CC::CondCode>(Op.getConstantOperandVal(2));
----------------
LLVM coding standard prefers early exits, so this should be
```
if (Op.getOpcode() == AArch64ISD::CSEL)
  return None;


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:15441
+    SDValue Rhs = Op.getOperand(1);
+    if (isOneConstant(Lhs) && isNullConstant(Rhs)) {
+      return CC;
----------------
Unnecessary {} for single line block. Also applies to the next if block.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:15478
+
+  SDValue OpLhs = Op->getOperand(0);
+  SDValue OpRhs = Op->getOperand(1);
----------------
I guess Dave will say `OpLHS` here? Likewise in `getCSETCondCode`.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:15486-15487
+  auto CC = getCSETCondCode(CmpOp.getOperand(0));
+  if (!(IsAdd ? ((CC == AArch64CC::HS) || (CC == AArch64CC::HI))
+              : ((CC == AArch64CC::LO) || (CC == AArch64CC::LS))))
+    return SDValue();
----------------
Perhaps a silly question but this suggests
```
ADC l r (CMP (CSET HS carry) 1)) == ADC l r (CMP (CSET HI carry) 1))
```
which at first glance doesn't look correct. Looking at the test changes I only see instances of `HS` and `LO`. Sure `HS` and `HI` both set the carry flag but for the `HI` case that's not enough for `CSET` to return `1`. Am I missing something?



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:15493
+  SDValue CsetOp = CmpOp.getOperand(0);
+  SDValue CsetCarry = CsetOp.getOperand(CsetOp->getNumOperands() - 1);
+
----------------
This seems weird.  You either know `CsetOp` is a `CSET` or you don't.  If you do then you know exactly which operand represents the carry?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:15499
+
+  return DAG.getNode(Opcode, SDLoc(Op), VTs, OpLhs, OpRhs, CsetCarry);
+}
----------------
Does just `Op->getVTList()` work here?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:18710-18715
+  case AArch64ISD::ADDS:
+  case AArch64ISD::SUBS:
+  case AArch64ISD::ADC:
+  case AArch64ISD::ADCS:
+  case AArch64ISD::SBC:
+  case AArch64ISD::SBCS:
----------------
How safe is this? i.e. do all the transformations relevant to `ADD` and `SUB` also apply to these nodes.  My cursory glance suggests not.  Even for the cases where the opcode is checked it seems inefficient to call a bunch of `try...` functions we know ahead of time are useless.  There's an argument `performAddSubCombine` should be sliced in two given there's not much overlap between the ADD and SUB cases.

But that's not your problem so I guess my question is why not just have your new opcodes call directly into `foldOverflowCheck`? Also the `ADDS` and `SUBS` cases look redundant?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123779