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

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm accepted this revision.
paulwalker-arm added inline comments.
This revision is now accepted and ready to land.


================
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:
> 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?
I think you're putting a bit too much stock into what these things are called but sure, I can see how there's a bit more going on here than normal so if this is your preference then yes this works for me. Thanks for investigating.


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

https://reviews.llvm.org/D137245