On Wed, Apr 10, 2024 at 05:42:05PM +0100, Richard Sandiford wrote:
> Andrew Carlotti <andrew.carlo...@arm.com> writes:
> > On Tue, Apr 09, 2024 at 04:43:16PM +0100, Richard Sandiford wrote:
> >> Andrew Carlotti <andrew.carlo...@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
