On Wed, Apr 10, 2024 at 07:51:44PM +0100, Richard Sandiford wrote:
> Andrew Carlotti <andrew.carlo...@arm.com> writes:
> > 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.
>
> 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
