[PATCH] D137245: [AArch64][SVE2] Add the SVE2.1 quadword variants of ld1w/ld1d/st1w/st1d

Mon Nov 7 00:15:23 PST 2022

david-arm marked an inline comment as done.
david-arm added inline comments.

================
Comment at: llvm/lib/Target/AArch64/SVEInstrFormats.td:9213-9215
+class sve_mem_128b_cld_si<bits<2> dtype, string mnemonic>
+    : I<(outs Z_q:$Zt), (ins PPR3bAny:$Pg, GPR64sp:$Rn, simm4s1:$imm4),
+        mnemonic, "\t$Zt, $Pg/z, [$Rn, $imm4, mul vl]",
----------------
david-arm wrote:
> paulwalker-arm wrote:
> > Given they're structurally identical what about extending `sve_mem_cst_si` to include an extra `q` bit.  This will also mean the new instructions have a matching set of InstAliases. This also matches the path you've been able to take regarding the stores.
> I assume you mean sve_mem_cld_si_base, not sve_mem_cst_si since they are stores? If you're referring to sve_mem_cld_si_base then they're not quite structurally the same since the `dtype` field is only bits 24-23 for quadwords. By bringing them together it does become a bit confusing because treating the quadword encoding group as having a 24-21 bit dtype field leads to exactly the same dtypes we have for LD1SB (1100) and LD1SH (1000).
> 
> Reusing the classes reduces code for sure, but it might make it more confusing. I'll give it a try anyway and see how it looks.
Hmm, the problem with reusing the classes is that it requires inverting the meaning of the nf bit. In sve_mem_cld_si_base the nf is bit 20, which is set to 0b0 for normal non-quadword loads, i.e. LD1W_IMM, however for sve_mem_128b_cld_si bit 20 is 0b1! It seems a bit odd to say that a faulting quadword load has nf=1. If you don't mind I'll keep the existing class as it is?

================
Comment at: llvm/lib/Target/AArch64/SVEInstrFormats.td:9242
+// SVE contiguous load (quadwords, scalar plus scalar)
+class sve_mem_128b_cld_ss<bits<2> dtype, string mnemonic, RegisterOperand gprsh_ty>
+    : I<(outs Z_q:$Zt), (ins PPR3bAny:$Pg, GPR64sp:$Rn, gprsh_ty:$Rm),
----------------
paulwalker-arm wrote:
> Perhaps just extend `sve_mem_cld_ss_base`? I'd be tempted to extend `sve_mem_cld_ss` also, but that's up to you.
Again, I think this doesn't really help because the quadword loads (sve_mem_128b_cld_ss) and other loads (sve_mem_cld_ss_base) have different bits 15-14:

sve_mem_128b_cld_ss: 0b10
sve_mem_cld_ss_base: 0b01

It seems to me like the two encoding groups are not close enough to be reused easily.

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

https://reviews.llvm.org/D137245