| Message ID | 33371799-7353-cd99-3f78-9abe31ad24ec@e124511.cambridge.arm.com |
|---|---|
| Headers | show |
| Series | aarch64: FMV feature list fixes | expand |
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