[0/5] aarch64: FMV feature list fixes

  The first three patches are trivial changes to the feature list to reflect
recent changes in the ACLE.  Patch 4 removes most of the FMV multiversioning
features that don't work at the moment, and should be entirely uncontroversial.

Patch 5 handles the remaining cases, where there's an inconsistency in how
features are named in the current FMV specification compared to the existing
command line options.  It might be better to instead preserve the "memtag2",
"ssbs2" and "ls64_accdata" names for now; I'd be happy to commit either
version.

Bootstrapped and regression tested on aarch64. Ok for master?
  

Andrew Carlotti <andrew.carlotti@arm.com> writes:
> The first three patches are trivial changes to the feature list to reflect
> recent changes in the ACLE.  Patch 4 removes most of the FMV multiversioning
> features that don't work at the moment, and should be entirely uncontroversial.
>
> Patch 5 handles the remaining cases, where there's an inconsistency in how
> features are named in the current FMV specification compared to the existing
> command line options.  It might be better to instead preserve the "memtag2",
> "ssbs2" and "ls64_accdata" names for now; I'd be happy to commit either
> version.

Yeah, I suppose patch 5 leaves things in a somewhat awkward state,
since e.g.:

-AARCH64_OPT_FMV_EXTENSION("memtag", MEMTAG, (), (), (), "")
+AARCH64_OPT_EXTENSION("memtag", MEMTAG, (), (), (), "")
 
-AARCH64_FMV_FEATURE("memtag2", MEMTAG2, (MEMTAG))
+AARCH64_FMV_FEATURE("memtag", MEMTAG2, (MEMTAG))

seems to drop "memtag2" and FEAT_MEMTAG, but keep "memtag" and
FEAT_MEMTAG2.  Is that right?

Apart from that and the comment on patch 2, the series looks good to me.

While rechecking aarch64-option-extensions.def against the ACLE list:
it seems that the .def doesn't treat mops as an FMV feature.  Is that
deliberate?

Thanks,
Richard
  

On Tue, Apr 09, 2024 at 04:43:16PM +0100, Richard Sandiford wrote:
> Andrew Carlotti <andrew.carlotti@arm.com> writes:
> > The first three patches are trivial changes to the feature list to reflect
> > recent changes in the ACLE.  Patch 4 removes most of the FMV multiversioning
> > features that don't work at the moment, and should be entirely uncontroversial.
> >
> > Patch 5 handles the remaining cases, where there's an inconsistency in how
> > features are named in the current FMV specification compared to the existing
> > command line options.  It might be better to instead preserve the "memtag2",
> > "ssbs2" and "ls64_accdata" names for now; I'd be happy to commit either
> > version.
> 
> Yeah, I suppose patch 5 leaves things in a somewhat awkward state,
> since e.g.:
> 
> -AARCH64_OPT_FMV_EXTENSION("memtag", MEMTAG, (), (), (), "")
> +AARCH64_OPT_EXTENSION("memtag", MEMTAG, (), (), (), "")
>  
> -AARCH64_FMV_FEATURE("memtag2", MEMTAG2, (MEMTAG))
> +AARCH64_FMV_FEATURE("memtag", MEMTAG2, (MEMTAG))
> 
> seems to drop "memtag2" and FEAT_MEMTAG, but keep "memtag" and
> FEAT_MEMTAG2.  Is that right?

That's deliberate. The FEAT_MEMTAG bit in __aarch64_cpu_features is defined to
match the definition of FEAT_MTE in the architecture, and likewise for
FEAT_MEMTAG2/FEAT_MTE2.  However, in Binutils the "+memtag" extension enables
both FEAT_MTE and FEAT_MTE2 instructions (although none of the FEAT_MTE2
instructions can be generated from GCC without inline assembly).  The FMV
specification in the ACLE currently uses names "memtag" and "memtag2" that
match the architecture names, but arguably don't match the command line
extension names.  I'm advocating for that to change to match the extension
names in command line options.

The LS64 example is definitely an inconsistency, since GCC uses "+ls64" to
enable intrinsics for all of the FEAT_LS64/FEAT_LS64_V/FEAT_LS64_ACCDATA
intrinsics.

There were similar issues with "sha1", "pmull" and "sve2-pmull128", but in
these cases their presence architecturally is implied by the presence of the
features checked for "sha2", "aes" and "sve2-aes" so it's fine to just delete
the ones without command line flags.

> Apart from that and the comment on patch 2, the series looks good to me.
> 
> While rechecking aarch64-option-extensions.def against the ACLE list:
> it seems that the .def doesn't treat mops as an FMV feature.  Is that
> deliberate?

"mops" was added to the ACLE list later, and libgcc doesn't yet support
detecting it.  I didn't think it was sensible to add new FMV feature support at
this stage.

> Thanks,
> Richard
  

Andrew Carlotti <andrew.carlotti@arm.com> writes:
> On Tue, Apr 09, 2024 at 04:43:16PM +0100, Richard Sandiford wrote:
>> Andrew Carlotti <andrew.carlotti@arm.com> writes:
>> > The first three patches are trivial changes to the feature list to reflect
>> > recent changes in the ACLE.  Patch 4 removes most of the FMV multiversioning
>> > features that don't work at the moment, and should be entirely uncontroversial.
>> >
>> > Patch 5 handles the remaining cases, where there's an inconsistency in how
>> > features are named in the current FMV specification compared to the existing
>> > command line options.  It might be better to instead preserve the "memtag2",
>> > "ssbs2" and "ls64_accdata" names for now; I'd be happy to commit either
>> > version.
>> 
>> Yeah, I suppose patch 5 leaves things in a somewhat awkward state,
>> since e.g.:
>> 
>> -AARCH64_OPT_FMV_EXTENSION("memtag", MEMTAG, (), (), (), "")
>> +AARCH64_OPT_EXTENSION("memtag", MEMTAG, (), (), (), "")
>>  
>> -AARCH64_FMV_FEATURE("memtag2", MEMTAG2, (MEMTAG))
>> +AARCH64_FMV_FEATURE("memtag", MEMTAG2, (MEMTAG))
>> 
>> seems to drop "memtag2" and FEAT_MEMTAG, but keep "memtag" and
>> FEAT_MEMTAG2.  Is that right?
>
> That's deliberate. The FEAT_MEMTAG bit in __aarch64_cpu_features is defined to
> match the definition of FEAT_MTE in the architecture, and likewise for
> FEAT_MEMTAG2/FEAT_MTE2.  However, in Binutils the "+memtag" extension enables
> both FEAT_MTE and FEAT_MTE2 instructions (although none of the FEAT_MTE2
> instructions can be generated from GCC without inline assembly).  The FMV
> specification in the ACLE currently uses names "memtag" and "memtag2" that
> match the architecture names, but arguably don't match the command line
> extension names.  I'm advocating for that to change to match the extension
> names in command line options.

Hmm, ok.  I agree it makes sense for the user-visible FMV namnes to match
the command line.  But shouldn't __aarch64_cpu_features either (a) use exactly
the same names as the architecture or (b) use exactly the same names as the
command-line (mangled where necessary)?  It seems that we're instead
using a third convention that doesn't exactly match the other two.

That is, I can see the rationale for "memtag" => FEAT_MTE2 and
"memtag" => FEAT_MEMTAG.  It just seems odd to have "memtag" => FEAT_MEMTAG2
(where MEMTAG2 is an alias of MTE2).

How much leeway do we have to change the __aarch64_cpu_features names?
Is it supposed to be a public API (as opposed to ABI)?

> The LS64 example is definitely an inconsistency, since GCC uses "+ls64" to
> enable intrinsics for all of the FEAT_LS64/FEAT_LS64_V/FEAT_LS64_ACCDATA
> intrinsics.

Ok, thanks.  If we go for option (a) above then I agree that the ls64
change is correct.  If we go for option (b) then I suppose it should
stay as LS64.

> There were similar issues with "sha1", "pmull" and "sve2-pmull128", but in
> these cases their presence architecturally is implied by the presence of the
> features checked for "sha2", "aes" and "sve2-aes" so it's fine to just delete
> the ones without command line flags.
>
>> Apart from that and the comment on patch 2, the series looks good to me.
>> 
>> While rechecking aarch64-option-extensions.def against the ACLE list:
>> it seems that the .def doesn't treat mops as an FMV feature.  Is that
>> deliberate?
>
> "mops" was added to the ACLE list later, and libgcc doesn't yet support
> detecting it.  I didn't think it was sensible to add new FMV feature support at
> this stage.

Ah, ok, makes sense.

Richard
  

On Wed, Apr 10, 2024 at 05:42:05PM +0100, Richard Sandiford wrote:
> Andrew Carlotti <andrew.carlotti@arm.com> writes:
> > On Tue, Apr 09, 2024 at 04:43:16PM +0100, Richard Sandiford wrote:
> >> Andrew Carlotti <andrew.carlotti@arm.com> writes:
> >> > The first three patches are trivial changes to the feature list to reflect
> >> > recent changes in the ACLE.  Patch 4 removes most of the FMV multiversioning
> >> > features that don't work at the moment, and should be entirely uncontroversial.
> >> >
> >> > Patch 5 handles the remaining cases, where there's an inconsistency in how
> >> > features are named in the current FMV specification compared to the existing
> >> > command line options.  It might be better to instead preserve the "memtag2",
> >> > "ssbs2" and "ls64_accdata" names for now; I'd be happy to commit either
> >> > version.
> >> 
> >> Yeah, I suppose patch 5 leaves things in a somewhat awkward state,
> >> since e.g.:
> >> 
> >> -AARCH64_OPT_FMV_EXTENSION("memtag", MEMTAG, (), (), (), "")
> >> +AARCH64_OPT_EXTENSION("memtag", MEMTAG, (), (), (), "")
> >>  
> >> -AARCH64_FMV_FEATURE("memtag2", MEMTAG2, (MEMTAG))
> >> +AARCH64_FMV_FEATURE("memtag", MEMTAG2, (MEMTAG))
> >> 
> >> seems to drop "memtag2" and FEAT_MEMTAG, but keep "memtag" and
> >> FEAT_MEMTAG2.  Is that right?
> >
> > That's deliberate. The FEAT_MEMTAG bit in __aarch64_cpu_features is defined to
> > match the definition of FEAT_MTE in the architecture, and likewise for
> > FEAT_MEMTAG2/FEAT_MTE2.  However, in Binutils the "+memtag" extension enables
> > both FEAT_MTE and FEAT_MTE2 instructions (although none of the FEAT_MTE2
> > instructions can be generated from GCC without inline assembly).  The FMV
> > specification in the ACLE currently uses names "memtag" and "memtag2" that
> > match the architecture names, but arguably don't match the command line
> > extension names.  I'm advocating for that to change to match the extension
> > names in command line options.
> 
> Hmm, ok.  I agree it makes sense for the user-visible FMV namnes to match
> the command line.  But shouldn't __aarch64_cpu_features either (a) use exactly
> the same names as the architecture or (b) use exactly the same names as the
> command-line (mangled where necessary)?  It seems that we're instead
> using a third convention that doesn't exactly match the other two.

I agree that the name isn't one I would choose now, but I don't think it matters much that it's inconsistent.

> That is, I can see the rationale for "memtag" => FEAT_MTE2 and
> "memtag" => FEAT_MEMTAG.  It just seems odd to have "memtag" => FEAT_MEMTAG2
> (where MEMTAG2 is an alias of MTE2).
> 
> How much leeway do we have to change the __aarch64_cpu_features names?
> Is it supposed to be a public API (as opposed to ABI)?

I think we're designing it to be capable of being a public API, but we haven't
yet made it one.  That's partly why I've kept the enum value names the same as
in LLVM so far.

> > The LS64 example is definitely an inconsistency, since GCC uses "+ls64" to
> > enable intrinsics for all of the FEAT_LS64/FEAT_LS64_V/FEAT_LS64_ACCDATA
> > intrinsics.
> 
> Ok, thanks.  If we go for option (a) above then I agree that the ls64
> change is correct.  If we go for option (b) then I suppose it should
> stay as LS64.
> 
> > There were similar issues with "sha1", "pmull" and "sve2-pmull128", but in
> > these cases their presence architecturally is implied by the presence of the
> > features checked for "sha2", "aes" and "sve2-aes" so it's fine to just delete
> > the ones without command line flags.
> >
> >> Apart from that and the comment on patch 2, the series looks good to me.
> >> 
> >> While rechecking aarch64-option-extensions.def against the ACLE list:
> >> it seems that the .def doesn't treat mops as an FMV feature.  Is that
> >> deliberate?
> >
> > "mops" was added to the ACLE list later, and libgcc doesn't yet support
> > detecting it.  I didn't think it was sensible to add new FMV feature support at
> > this stage.
> 
> Ah, ok, makes sense.
> 
> Richard
  

Andrew Carlotti <andrew.carlotti@arm.com> writes:
> On Wed, Apr 10, 2024 at 05:42:05PM +0100, Richard Sandiford wrote:
>> Andrew Carlotti <andrew.carlotti@arm.com> writes:
>> > On Tue, Apr 09, 2024 at 04:43:16PM +0100, Richard Sandiford wrote:
>> >> Andrew Carlotti <andrew.carlotti@arm.com> writes:
>> >> > The first three patches are trivial changes to the feature list to reflect
>> >> > recent changes in the ACLE.  Patch 4 removes most of the FMV multiversioning
>> >> > features that don't work at the moment, and should be entirely uncontroversial.
>> >> >
>> >> > Patch 5 handles the remaining cases, where there's an inconsistency in how
>> >> > features are named in the current FMV specification compared to the existing
>> >> > command line options.  It might be better to instead preserve the "memtag2",
>> >> > "ssbs2" and "ls64_accdata" names for now; I'd be happy to commit either
>> >> > version.
>> >> 
>> >> Yeah, I suppose patch 5 leaves things in a somewhat awkward state,
>> >> since e.g.:
>> >> 
>> >> -AARCH64_OPT_FMV_EXTENSION("memtag", MEMTAG, (), (), (), "")
>> >> +AARCH64_OPT_EXTENSION("memtag", MEMTAG, (), (), (), "")
>> >>  
>> >> -AARCH64_FMV_FEATURE("memtag2", MEMTAG2, (MEMTAG))
>> >> +AARCH64_FMV_FEATURE("memtag", MEMTAG2, (MEMTAG))
>> >> 
>> >> seems to drop "memtag2" and FEAT_MEMTAG, but keep "memtag" and
>> >> FEAT_MEMTAG2.  Is that right?
>> >
>> > That's deliberate. The FEAT_MEMTAG bit in __aarch64_cpu_features is defined to
>> > match the definition of FEAT_MTE in the architecture, and likewise for
>> > FEAT_MEMTAG2/FEAT_MTE2.  However, in Binutils the "+memtag" extension enables
>> > both FEAT_MTE and FEAT_MTE2 instructions (although none of the FEAT_MTE2
>> > instructions can be generated from GCC without inline assembly).  The FMV
>> > specification in the ACLE currently uses names "memtag" and "memtag2" that
>> > match the architecture names, but arguably don't match the command line
>> > extension names.  I'm advocating for that to change to match the extension
>> > names in command line options.
>> 
>> Hmm, ok.  I agree it makes sense for the user-visible FMV namnes to match
>> the command line.  But shouldn't __aarch64_cpu_features either (a) use exactly
>> the same names as the architecture or (b) use exactly the same names as the
>> command-line (mangled where necessary)?  It seems that we're instead
>> using a third convention that doesn't exactly match the other two.
>
> I agree that the name isn't one I would choose now, but I don't think it matters much that it's inconsistent.

I kind-of think it does though.  Given...

>> That is, I can see the rationale for "memtag" => FEAT_MTE2 and
>> "memtag" => FEAT_MEMTAG.  It just seems odd to have "memtag" => FEAT_MEMTAG2
>> (where MEMTAG2 is an alias of MTE2).
>> 
>> How much leeway do we have to change the __aarch64_cpu_features names?
>> Is it supposed to be a public API (as opposed to ABI)?
>
> I think we're designing it to be capable of being a public API, but we haven't
> yet made it one.  That's partly why I've kept the enum value names the same as
> in LLVM so far.

...this, I don't want to sleep-walk into a situation where we have
one naming convention for the architecture, one for the attributes,
and a third one for the API.  If we're not in a position to commit
to a consistent naming scheme for the API by GCC 14 then it might be
better to remove the FMV features in 5/5 for GCC 14 and revisit in GCC 15.

A patch to do that is pre-approved if you agree (but please say
if you don't).

Thanks,
Richard
  

On Wed, Apr 10, 2024 at 07:51:44PM +0100, Richard Sandiford wrote:
> Andrew Carlotti <andrew.carlotti@arm.com> writes:
> > On Wed, Apr 10, 2024 at 05:42:05PM +0100, Richard Sandiford wrote:
> >> Andrew Carlotti <andrew.carlotti@arm.com> writes:
> >> > On Tue, Apr 09, 2024 at 04:43:16PM +0100, Richard Sandiford wrote:
> >> >> Andrew Carlotti <andrew.carlotti@arm.com> writes:
> >> >> > The first three patches are trivial changes to the feature list to reflect
> >> >> > recent changes in the ACLE.  Patch 4 removes most of the FMV multiversioning
> >> >> > features that don't work at the moment, and should be entirely uncontroversial.
> >> >> >
> >> >> > Patch 5 handles the remaining cases, where there's an inconsistency in how
> >> >> > features are named in the current FMV specification compared to the existing
> >> >> > command line options.  It might be better to instead preserve the "memtag2",
> >> >> > "ssbs2" and "ls64_accdata" names for now; I'd be happy to commit either
> >> >> > version.
> >> >> 
> >> >> Yeah, I suppose patch 5 leaves things in a somewhat awkward state,
> >> >> since e.g.:
> >> >> 
> >> >> -AARCH64_OPT_FMV_EXTENSION("memtag", MEMTAG, (), (), (), "")
> >> >> +AARCH64_OPT_EXTENSION("memtag", MEMTAG, (), (), (), "")
> >> >>  
> >> >> -AARCH64_FMV_FEATURE("memtag2", MEMTAG2, (MEMTAG))
> >> >> +AARCH64_FMV_FEATURE("memtag", MEMTAG2, (MEMTAG))
> >> >> 
> >> >> seems to drop "memtag2" and FEAT_MEMTAG, but keep "memtag" and
> >> >> FEAT_MEMTAG2.  Is that right?
> >> >
> >> > That's deliberate. The FEAT_MEMTAG bit in __aarch64_cpu_features is defined to
> >> > match the definition of FEAT_MTE in the architecture, and likewise for
> >> > FEAT_MEMTAG2/FEAT_MTE2.  However, in Binutils the "+memtag" extension enables
> >> > both FEAT_MTE and FEAT_MTE2 instructions (although none of the FEAT_MTE2
> >> > instructions can be generated from GCC without inline assembly).  The FMV
> >> > specification in the ACLE currently uses names "memtag" and "memtag2" that
> >> > match the architecture names, but arguably don't match the command line
> >> > extension names.  I'm advocating for that to change to match the extension
> >> > names in command line options.
> >> 
> >> Hmm, ok.  I agree it makes sense for the user-visible FMV namnes to match
> >> the command line.  But shouldn't __aarch64_cpu_features either (a) use exactly
> >> the same names as the architecture or (b) use exactly the same names as the
> >> command-line (mangled where necessary)?  It seems that we're instead
> >> using a third convention that doesn't exactly match the other two.
> >
> > I agree that the name isn't one I would choose now, but I don't think it matters much that it's inconsistent.
> 
> I kind-of think it does though.  Given...
> 
> >> That is, I can see the rationale for "memtag" => FEAT_MTE2 and
> >> "memtag" => FEAT_MEMTAG.  It just seems odd to have "memtag" => FEAT_MEMTAG2
> >> (where MEMTAG2 is an alias of MTE2).
> >> 
> >> How much leeway do we have to change the __aarch64_cpu_features names?
> >> Is it supposed to be a public API (as opposed to ABI)?
> >
> > I think we're designing it to be capable of being a public API, but we haven't
> > yet made it one.  That's partly why I've kept the enum value names the same as
> > in LLVM so far.
> 
> ...this, I don't want to sleep-walk into a situation where we have
> one naming convention for the architecture, one for the attributes,
> and a third one for the API.  If we're not in a position to commit
> to a consistent naming scheme for the API by GCC 14 then it might be
> better to remove the FMV features in 5/5 for GCC 14 and revisit in GCC 15.
> 
> A patch to do that is pre-approved if you agree (but please say
> if you don't).

I'm happy to remove those features for GCC 14 (pending agreement on the
attribute names in particular), but I don't think that does anything to solve
the enum names issue.  I'll remove the names from my FMV documentation patch as
well.

> Thanks,
> Richard