[PATCH] D137554: [AArch64][SVE2] Add the SVE2.1 quadword structured load/store instructions

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/SVEInstrFormats.td:7129-7131
+  let Inst{24-23} = !if(q, nregs, sz);
+  let Inst{22-21} = !if(q, 0b00, nregs);
+  let Inst{20}    = q;
----------------
paulwalker-arm wrote:
> You're being overly literal regarding the name of these parameters.  They were likely picked to match the original encoding groups but looking at "SVE load multiple structures (scalar plus immediate)" they now just call `{22-21}` opc.
> 
> It's better for the variables to always be placed in the same part of the instruction. It doesn't really matter if the names don't exactly match. In this instance I think it'll be better to replace `nregs` and `q` with a 3-bit `opc`. You may as well keep `sz` even though it means something different for the quadword variants, because it matters not. However, if you don't like keeping `sz` then just make `opc` a 5-bit opcode instead.
The main reason I chose to write it like this is because otherwise the list of instructions in AArch64SVEInstrInfo.td just looks weird/inconsistent, i.e.

  defm LD2D_IMM : sve_mem_eld_si<0b11, 0b001, ZZ_d,   "ld2d", simm4s2>;
  defm LD3D_IMM : sve_mem_eld_si<0b11, 0b010, ZZZ_d,  "ld3d", simm4s3>;
  defm LD4D_IMM : sve_mem_eld_si<0b11, 0b011, ZZZZ_d, "ld4d", simm4s4>;
  let Predicates = [HasSVE2p1_or_HasSME2p1] in {
  defm LD2Q_IMM : sve_mem_eld_si<0b01, 0b100, ZZ_q,   "ld2q", simm4s2>;
  defm LD3Q_IMM : sve_mem_eld_si<0b10, 0b100, ZZZ_q,  "ld3q", simm4s3>;
  defm LD4Q_IMM : sve_mem_eld_si<0b11, 0b100, ZZZZ_q, "ld4q", simm4s4>;
  }

because the quadword variants don't follow the pattern of the ones above. I guess it's a choice between having a potentially confusing instruction class format, or a confusing list of instructions. I don't really mind though or feel strongly about it - happy to go with the latter!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137554