[PATCH 0/6] aarch64: sme-lutv2 fixes and cleanup

[Andrew Carlotti]

On Thu, Jan 16, 2025 at 09:36:35AM +0000, Richard Sandiford wrote:
> Andrew Carlotti <andrew.carlotti@arm.com> writes:
> > Patch 1 fixes incorrect target feature gating - ok for master and backport to
> > the branch?
> >
> > The remaining patches are some minor improvements that I noticed while
> > reviewing the rest of the original patch - ok for master?
> 
> Nice series!  OK as above.  But a comment about:
> 
> -  LUTv2_SME2_INSN ("luti4", 0xc08b0000, 0xffffcc23, sme_size_12_b, OP3 (SME_Zdnx4, SME_ZT0, SME_Znx2_BIT_INDEX), OP_SVE_VUU_B, F_STRICT | 0),
> -  LUTv2_SME2p1_INSN ("luti4", 0xc09b0000, 0xffffcc2c, sme_size_12_b, OP3 (SME_Zdnx4_STRIDED, SME_ZT0, SME_Znx2_BIT_INDEX), OP_SVE_VUU_B, F_STRICT | 0),
> +  LUTv2_SME2_INSN ("luti4", 0xc08b0000, 0xffffcc23, sme_size_12_b, OP3 (SME_Zdnx4, SME_ZT0, SME_Znx2_BIT_INDEX), OP_SVE_BUU, F_STRICT | 0),
> +  LUTv2_SME2p1_INSN ("luti4", 0xc09b0000, 0xffffcc2c, sme_size_12_b, OP3 (SME_Ztx4_STRIDED, SME_ZT0, SME_Znx2_BIT_INDEX), OP_SVE_BUU, F_STRICT | 0),
> 
> in patch 2.  Having OP_SVE_xxx_yy, with Vs in the xxx and with variants
> of the V in the yy, is the usual convention for cases in which the choice
> of qualifiers is determined by the aarch64_insn_class (here sme_size_12_b).
> When there is only one valid set of qualifiers, I agree it makes sense
> to use the OP_SVE_xxx form instead, with the elmeent sizes in the xxx.
> But then I think we should also drop sme_size_12_b and hardcode the size
> to 0 in the mask.
> 
> I realise that isn't how the instruction is specified in the ISA
> documentation, but I think it makes sense in binutils.  If any other
> values of "size" are supported in future, they would be conditional on
> a different set of features, and so would be a different table entry.
> 
> So the series is also ok with the removal of sme_size_12_b, if you're
> ok with that.  But it's also ok as-is, in which case I might try removing
> sme_size_12_b as a follow-up if I have time before the release.
> 
> Thanks,
> Richard

I'd only been viewing the OP_SVE_VUU_B macro name as a way of specifying the
qualifier list, without making any explicit link to how the qualifiers are
selected for that particular instruction.

I think specifying the size in this way can help a little with clarity - I
actually spent several minutes thinking one of the sme2p1 luti4 was incorrectly
specified because I hadn't noticed that only one size was permitted.  But the
extra cases elsewhere in the code arising are a minor disadvantage.

I've also contemplated refactoring the size determination code to include a
check for whether the computed size corresponds to a valid qualifier list
entry, which would eliminate all of these extra cases.  However, that's not
something to be changing mere days before the release branches.  I'm hoping we
can sort out other more fundamental issues with the way we use qualifiers
during the next release cycle, so it probably makes sense to look at
deduplication while doing that.