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

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td:1438
+  let Predicates = [HasSVE2p1_or_HasSME2p1] in {
+  defm ST2Q_IMM : sve_mem_128b_est_si<0b01, ZZ_q,    "st2q", simm4s2>;
+  defm ST3Q_IMM : sve_mem_128b_est_si<0b10, ZZZ_q,   "st3q", simm4s3>;
----------------
david-arm wrote:
> It's difficult to reuse the existing sve_mem_est_si and sve_mem_est_ss classes because the sz/nregs fields are shifted, i.e.
> 
> sve_mem_est_si:
>   let Inst{24-23} = sz;
>   let Inst{22-21} = nregs;
> 
> sve_mem_128b_est_si:
>   let Inst{23-22} = nregs;
>   let Inst{21-20} = 0b00;
Agreed.  The new instructions sit within a different part of the encoding tables anyway and so having new instructions classes is the correct play.


================
Comment at: llvm/lib/Target/AArch64/SVEInstrFormats.td:5813
 
+
+class sve_mem_128b_est_si<bits<2> nregs, RegisterOperand VecList,
----------------
Please add a comment for the encoding group.


================
Comment at: llvm/lib/Target/AArch64/SVEInstrFormats.td:5866
 
+
+class sve_mem_128b_est_ss<bits<2> nregs, RegisterOperand VecList,
----------------
Please add a comment for the encoding group.


================
Comment at: llvm/lib/Target/AArch64/SVEInstrFormats.td:5919
 
+
 class sve_mem_cstnt_si<bits<2> msz, string asm, RegisterOperand VecList>
----------------
typo?


================
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;
----------------
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.


================
Comment at: llvm/lib/Target/AArch64/SVEInstrFormats.td:7162-7163
   let Inst{31-25} = 0b1010010;
-  let Inst{24-23} = sz;
-  let Inst{22-21} = nregs;
+  let Inst{24-23} = !if(q, nregs, sz);
+  let Inst{22-21} = !if(q, 0b01, nregs);
   let Inst{20-16} = Rm;
----------------
As above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137554