[PATCH] D116468: [AArch64] Combine ADD/SUB instructions when they contain a 24-bit immediate.

[Dave Green via Phabricator via llvm-commits]

dmgreen added a reviewer: benshi001.
dmgreen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:4795
+    // over ADDI-ADDI
+    if (!(Imm & ~0x00ffffff) && (Imm & 0x00ff0000) && (Imm & 0x00000fff)) {
+      Patterns.push_back(Pat);
----------------
Can we generalize this to "Any MOV that will be > 1 instruction"? It may be possible to use expandMOVImm for that, and check the number of instructions.

The rules for what makes a single instruction are difficult to represent simply, and can be more than a 16bit imm. As in https://godbolt.org/z/KjKhMjb7v.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:4838
+      return false;
+    unsigned Imm =
+        MRI.getUniqueVRegDef(AddSubOprd.getReg())->getOperand(1).getImm();
----------------
Should this be a uint64_t?


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:5560
+/// \endcode
+/// \param MF Containing MachineFunction
+/// \param MRI Register information
----------------
LLVM tends not to go too heavy on the doxygen comments, especially for obvious parameters like MF/MRI/TII.


================
Comment at: llvm/test/CodeGen/AArch64/aarch64-combine-addsub-24bit-imm.mir:2
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -O0 -run-pass=machine-combiner -o - -mtriple=aarch64-unknown-linux -verify-machineinstrs %s | FileCheck %s
+
----------------
I don't think this needs -O0


================
Comment at: llvm/test/CodeGen/AArch64/aarch64-combine-addsub-24bit-imm.mir:343
+    ; CHECK-NEXT: [[ADDXri:%[0-9]+]]:gpr64common = ADDXri [[COPY]], 273, 12
+    ; CHECK-NEXT: [[ADDXri1:%[0-9]+]]:gpr64common = ADDXri killed [[ADDXri]], 3549, 0
+    ; CHECK-NEXT: $x0 = COPY [[ADDXri1]]
----------------
I think this might be negative of the correct result.

(But equally I don't think that SUBXrr will be present at this point in the pipeline. It will be speculatively emitted as a SUBSXrr as that may be used as a compare).


================
Comment at: llvm/test/CodeGen/AArch64/aarch64-combine-addsub-imm-reject-loop.mir:2
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -O0 -run-pass=machine-combiner -o - -mtriple=aarch64-unknown-linux -verify-machineinstrs %s | FileCheck %s
+
----------------
This one might be better as a .ll file. That way we test the entire backend, and can see obvious regressions when the instruction count increases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116468