[PATCH] D112311: [AArch64] Handle ST1iN instructions in isAArch64FrameOffsetLegal

Mon Oct 25 06:02:24 PDT 2021

danilaml added inline comments.

================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:4293
+  case AArch64::ST1i16:
+  case AArch64::ST1i32:
   case AArch64::IRG:
----------------
ostannard wrote:
> Should this also handle `ST1i64`, and the `LD1` opcodes?
Right, forgot about i64.
Logically, it seems like it should handle LD1 opcodes the same way but I just wasn't able to come up with the code to test them.
This switch case feels a bit ad-hoc and if something more general is required, I think it's better to use tablegen for that.

================
Comment at: llvm/test/CodeGen/AArch64/aarch64st1.ll:2
+; Check that it doesn't crash with unhandled opcode error, see pr52249
+; RUN: llc < %s -o -| FileCheck %s
+
----------------
ostannard wrote:
> This would be better as an MIR test, so it's not dependent on unrelated details of codegen. If I run this test with `-stop-before=localstackalloc` to generate a MIR file, then run that back through llc with `-run-pass=localstackalloc`, I get the same same assertion.
I feel like MIR tests can be "brittle" in other ways (and CHECKs ensure that the st1 instructions are actually generated), but alright.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112311