Re: [PATCH v2 1/1] aarch64: Add SME2.1 structured sparsity outer product intrinsics

On 18/08/2026 12:19, Alex Coplan wrote:
> On 17/08/2026 19:12, Claudio Bantaloukas wrote:
>> Hellos!
  (snip)>>> You'll just need a mode attribute mapping ZA modes to the 
number of
>>> immediate bits needed to address them.  So the predicates become
>>> "aarch64_imm<za_imm_bits>" or so in the patterns.
>>>
>> I was thinking about following your initial suggestion but then opted for
>> aarch64_za<elem_bits>_imm for a few reasons.
>> One is because elem_bits is already there whereas za_imm_bits isn't.
> 
> Yes, but given you're going to have to add new preidcates if you don't
> add za_imm_bits, this seems a bit moot.
> 
>> One that I feel strongly about is that the za in the predicate shows intent.
> 
> I think "aarch64_imm<za_imm_bits>" shows the intent equally well.
> 
Absolutely!
	   (match_operand:DI 0 "aarch64_za<elem_bits>_imm")
vs
	   (match_operand:DI 0 "aarch64_imm<za_imm_bits>")
are equally readable and convey what arg 0 is very clearly.

But I find
	   (match_operand:DI 0 "aarch64_za32_imm")
	   (match_operand:<SVE_FULL_HF:VDOUBLE> 1 "aligned_register_operand" "Uw2")
	   (match_operand:SVE_FULL_HF 2 "register_operand" "w")
	   (match_operand:VNx16QI 3 "register_operand" "Uwo")
	   (match_operand:DI 4 "aarch64_imm2")

slightly better to
	   (match_operand:DI 0 "aarch64_imm2")
	   (match_operand:<SVE_FULL_HF:VDOUBLE> 1 "aligned_register_operand" "Uw2")
	   (match_operand:SVE_FULL_HF 2 "register_operand" "w")
	   (match_operand:VNx16QI 3 "register_operand" "Uwo")
	   (match_operand:DI 4 "aarch64_imm2")

Nevertheless, I'll go ahead with the change set out below unless told 
otherwise.

>> It makes it a little bit easier to see at a glance which opearand denotes
>> the tile.
>> And the one that I think we should discuss more widely is what to do with
>> aarch64_imm3. It's the wrong predicate to use for this case because it
>> limits the values to 4 or less whereas we would need something like
>> aarch64_lane_imm3 which correctly constrains us to 7 or less.
> 
> That's a good point, I hadn't realised that aarch64_imm3 doesn't actually
> follow the pattern here.  However, I don't think it presents an immediate
> problem for the purposes of this patch.
> 
> We have the following situation:
> 
> ZA element size | num ZA tiles | immediate bits
> ================+==============+====================
>                8 | 1            | 0
>               16 | 2            | 1
>               32 | 4            | 2
>               64 | 8            | 3
> 
> and none of the TMOP instructions have 64-bit ZA tile destintions (only
> 16- or 32-bit).
> 
> I agree, however, that using this approach presents a potential footgun
> for future SME patches (which might start using aarch64_imm3), so either
> as a pre-patch or follow-up patch, we should probably do the renaming
> you suggest below, making the aarch64_immN family of predicates
> self-consistent.
> 
> FWIW, I think aarch64_shift_imm3 is fine as a replacement name for the
> current aarch64_imm3.
> 
>>
>> If you feel strongly about adding za_imm_bits, would you consider it
>> complete if it was limited to all the modes in elem_bits belonging to 8(0),
>> 16(1), 32(2), 64(3), 128(4) bit za modes or would you rather I keep 0 out?
> 
> I don't think it would need to be implemented for all the modes in
> elem_bits.  In general, iterators and mode attributes only need
> implement what is actually practically needed.  In this case I think
> only { VNx8HI, VNx4SI, VNx2DI } and corresponding F modes are needed.
> If you wanted to be hyper-pragmatic, I think only VNx8HF and VNx4SF are
> needed by your patch as it stands, but we might as well implement the
> complete set that is likely to be used in practice.
> 
>>
>> Fields using aarch64_imm3 are described as "Is the left shift amount to be
>> applied after extension in the range 0 to 4". I guess we could call it
>> aarch64_shift_amount_imm3 and replace it in the extended register versions
>> of add, adds etc. Then we could rename aarch64_lane_imm3 to aarch64_imm3 but
>> if you look at places where aarch64_lane_imm3 is used, you'd spot the slight
>> loss in context this extended name brings.
>>
>> Let me know where you lean on this and I'll go that way.
> 
> As said above, I think we should use aarch64_imm<za_imm_bits>, and do
> this renaming (either as a pre-patch or follow-up patch), provided there
> are no objections from other reviewers/maintainers.
> 
> Thanks,
> Alex
> 
>>
>>>> +
>>>>    ;; An immediate that fits into 24 bits, but needs splitting.
>>>>    (define_predicate "aarch64_split_imm24"
>>>>      (match_code "const_int")
>>> <snip>
>>>> diff --git a/gcc/testsuite/gcc.target/aarch64/sme2/acle-asm/tmopa_lane_za16_bf16_bf16.c b/gcc/testsuite/gcc.target/aarch64/sme2/acle-asm/tmopa_lane_za16_bf16_bf16.c
>>>> new file mode 100644
>>>> index 00000000000..381ec046f45
>>>> --- /dev/null
>>>> +++ b/gcc/testsuite/gcc.target/aarch64/sme2/acle-asm/tmopa_lane_za16_bf16_bf16.c
>>>> @@ -0,0 +1,76 @@
>>>> +/* { dg-do assemble { target { aarch64_asm_sme-b16b16_ok && aarch64_asm_sme-tmop_ok}  } } */
>>>> +/* { dg-do compile { target { ! { aarch64_asm_sme-b16b16_ok && aarch64_asm_sme-tmop_ok } } } } */
>>>> +/* { dg-final { check-function-bodies "**" "" "-DCHECK_ASM" } } */
>>>> +
>>>> +#include "test_sme2_acle.h"
>>>> +
>>>> +#pragma GCC target "+sme-tmop+sme-b16b16"
>>>> +
>>>> +/*
>>>> +** tmopa_lane_za16_bf16_bf16_0_z0_z4_z20_0:
>>>> +**	bftmopa	za0\.h, {z0\.h - z1\.h}, z4\.h, z20\[0\]
>>>> +**	ret
>>>> +*/
>>>> +TEST_ZA_TMOP (tmopa_lane_za16_bf16_bf16_0_z0_z4_z20_0, svbfloat16x2_t, svbfloat16_t,
>>>> +		 svtmopa_lane_za16_bf16_bf16 (0, z0, z4, z20, 0),
>>>> +		 svtmopa_lane_za16 (0, z0, z4, z20, 0))
>>>> +
>>>> +/* ZA slice and offset with different values.
>>>> +** tmopa_lane_za16_bf16_bf16_1_z2_z4_z20_3:
>>>> +**	bftmopa	za1\.h, {z2\.h - z3\.h}, z4\.h, z20\[3\]
>>>> +**	ret
>>>> +*/
>>>> +TEST_ZA_TMOP (tmopa_lane_za16_bf16_bf16_1_z2_z4_z20_3, svbfloat16x2_t, svbfloat16_t,
>>>> +		 svtmopa_lane_za16_bf16_bf16 (1, z2, z4, z20, 3),
>>>> +		 svtmopa_lane_za16 (1, z2, z4, z20, 3))
>>>> +
>>>> +/* The first register on the second argument must be even.
>>>> +** tmopa_lane_za16_bf16_bf16_0_z1_z4_z20_0:
>>>> +**	mov	(z\d+)\.d, z1\.d
>>>> +**	mov	(z\d+)\.d, z2\.d
>>>> +**	bftmopa	za0\.h, {\1\.h - \2\.h}, z4\.h, z20\[0\]
>>>> +**	ret
>>>> +*/
>>>> +TEST_ZA_TMOP (tmopa_lane_za16_bf16_bf16_0_z1_z4_z20_0, svbfloat16x2_t, svbfloat16_t,
>>>> +		 svtmopa_lane_za16_bf16_bf16 (0, z1, z4, z20, 0),
>>>> +		 svtmopa_lane_za16 (0, z1, z4, z20, 0))
>>>> +
>>>> +/* zk register must be one of Z20-Z23 or Z28-z31.
>>>> +** tmopa_lane_za16_bf16_bf16_0_z0_z4_z19_0:
>>>> +**	mov	(z2[0-3]|z2[89]|z3[01]).d, z19.d
>>>
>>> Very minor nit, but you could factor the z out of the alternation, such
>>> that you're only matching the register number (adding it back in again
>>> below when you have the backreference).  It would make the regex a bit
>>> tidier / more readable.  Same for the other occurrences.
>>>
>> ack, will fix :)
>>
>>
>>> OK with those changes, thanks.
>>>
>>> Alex
>>>
>>>> +**	bftmopa	za0\.h, {z0\.h - z1\.h}, z4\.h, \1\[0\]
>>>> +**	ret
>>>> +*/
>>>> +TEST_ZA_TMOP (tmopa_lane_za16_bf16_bf16_0_z0_z4_z19_0, svbfloat16x2_t, svbfloat16_t,
>>>> +		 svtmopa_lane_za16_bf16_bf16 (0, z0, z4, z19, 0),
>>>> +		 svtmopa_lane_za16 (0, z0, z4, z19, 0))
>>>> +
>>>> +/* zk register must be one of Z20-Z23 or Z28-z31.
>>>> +** tmopa_lane_za16_bf16_bf16_0_z0_z4_z24_0:
>>>> +**	mov	(z2[0-3]|z2[89]|z3[01]).d, z24.d
>>>> +**	bftmopa	za0\.h, {z0\.h - z1\.h}, z4\.h, \1\[0\]
>>>> +**	ret
>>>> +*/
>>>> +TEST_ZA_TMOP (tmopa_lane_za16_bf16_bf16_0_z0_z4_z24_0, svbfloat16x2_t, svbfloat16_t,
>>>> +		 svtmopa_lane_za16_bf16_bf16 (0, z0, z4, z24, 0),
>>>> +		 svtmopa_lane_za16 (0, z0, z4, z24, 0))
>>>> +
>>>> +/* zk register must be one of Z20-Z23 or Z28-z31.
>>>> +** tmopa_lane_za16_bf16_bf16_0_z0_z4_z27_0:
>>>> +**	mov	(z2[0-3]|z2[89]|z3[01]).d, z27.d
>>>> +**	bftmopa	za0\.h, {z0\.h - z1\.h}, z4\.h, \1\[0\]
>>>> +**	ret
>>>> +*/
>>>> +TEST_ZA_TMOP (tmopa_lane_za16_bf16_bf16_0_z0_z4_z27_0, svbfloat16x2_t, svbfloat16_t,
>>>> +		 svtmopa_lane_za16_bf16_bf16 (0, z0, z4, z27, 0),
>>>> +		 svtmopa_lane_za16 (0, z0, z4, z27, 0))
>>>> +
>>>> +/* zk register must be one of Z20-Z23 or Z28-z31.
>>>> +** tmopa_lane_za16_bf16_bf16_0_z0_z4_z28_0:
>>>> +**	bftmopa	za0\.h, {z0\.h - z1\.h}, z4\.h, z28\[0\]
>>>> +**	ret
>>>> +*/
>>>> +TEST_ZA_TMOP (tmopa_lane_za16_bf16_bf16_0_z0_z4_z28_0, svbfloat16x2_t, svbfloat16_t,
>>>> +		 svtmopa_lane_za16_bf16_bf16 (0, z0, z4, z28, 0),
>>>> +		 svtmopa_lane_za16 (0, z0, z4, z28, 0))
>>>> +
>>> <snip>
>>