[PATCH] D94936: [AArch64][GlobalISel] Instruction selection for add/sub with carry-in

[Jessica Paquette via Phabricator via llvm-commits]

paquette added a comment.

Do you think you could split this into 3 patches, one for each commit?



================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp:2794
+    bool DefIsCarryOp = isCarryOp(*Def);
+    bool IsPredecessor = I.getPrevNode() == Def;
+    bool UsableCFlag = IsPredecessor && DefIsCarryOp &&
----------------
Could debug instructions mess this up?

If so, adding a `isPredecessorIgnoringDBG` helper might be useful here.


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp:2795
+    bool IsPredecessor = I.getPrevNode() == Def;
+    bool UsableCFlag = IsPredecessor && DefIsCarryOp &&
+                       condCodeForOverflowOp(Def->getOpcode()) == AArch64CC::HS;
----------------
Looks like you can fold the variables above into this one?


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp:169
+  getActionDefinitionsBuilder(
+      {G_SADDE, G_SSUBE, G_UADDE, G_USUBE, G_SADDO, G_SSUBO, G_UADDO, G_USUBO})
       .legalFor({{s32, s1}, {s64, s1}})
----------------
Can you add testcases for the `s32`/`s64` cases here, and for the `minScalar` case?



================
Comment at: llvm/test/CodeGen/AArch64/GlobalISel/legalize-arith-128.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+; RUN: llc -O3 --mtriple aarch64-unknown-unknown -stop-after=legalizer --global-isel=1 %s -o - -verify-machineinstrs | FileCheck %s
+
----------------
I think this should be a MIR test rather than a IR test.

This command line should get you 90% there:

```
llc -mtriple aarch64 -stop-before=legalizer -global-isel -simplify-mir -o -
```

You'll probably have to tidy it up a little bit after generating it though. (See: test/CodeGen/AArch64/legalize-uaddo.mir as an example)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94936