[PATCH] D123322: [AArch64] Add lowerings for {ADD,SUB}CARRY and S{ADD,SUB}O_CARRY

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added a comment.

One nit plus a suggestion but otherwise the patch looks good to me.  There are several regressions though, presumably caused by my request to split the original patch up.  I'm not qualified to say how important these test cases are from a performance point of view but given the number of regressions I think it's worth creating a patch series (via Depends On) so the patches can be reviewed in isolation but pushed together. Sorry if this is messing you around.



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:3365
+  // Let legalize expand this if it isn't a legal type yet.
+  if (!DAG.getTargetLoweringInfo().isTypeLegal(VT0)) {
+    return SDValue();
----------------
Unnecessary {} for single line body.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:3369-3390
+  bool IsSigned;
+  unsigned Opcode;
+  switch (Op->getOpcode()) {
+  default:
+    return SDValue();
+  case ISD::ADDCARRY:
+    IsSigned = false;
----------------
Up to you but I think these are better passed in a parameters so you end up with:
```
case ISD::ADDCARRY:
  return lowerADDSUBCARRY(Op, DAG, AArch64ISD::ADCS, false /*unsigned*/);
case ISD::SUBCARRY:
  return lowerADDSUBCARRY(Op, DAG, AArch64ISD::SBCS, false /*unsigned*/);
case ISD::SADDO_CARRY:
  return lowerADDSUBCARRY(Op, DAG, AArch64ISD::ADCS, true /*signed*/);
case ISD::SSUBO_CARRY:
  return lowerADDSUBCARRY(Op, DAG, AArch64ISD::SBCS, true /*signed*/);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123322