On Thu, Nov 13, 2025 at 01:12:08PM +0000, Alice Carlotti wrote:
> On Wed, Nov 12, 2025 at 09:54:15AM +0000, Tamar Christina wrote:
> > > -----Original Message-----
> > > From: Tamar Christina <[email protected]>
> > > Sent: 12 November 2025 08:49
> > > To: Alice Carlotti <[email protected]>
> > > Cc: [email protected]; Richard Earnshaw
> > > <[email protected]>; Kyrylo Tkachov <[email protected]>; Alex
> > > Coplan <[email protected]>; Andrew Pinski
> > > <[email protected]>; Wilco Dijkstra
> > > <[email protected]>; Alfie Richards <[email protected]>
> > > Subject: RE: [PATCH] aarch64: Extend syntax for cpuinfo feature string
> > > checks
> > >
> > > > -----Original Message-----
> > > > From: Alice Carlotti <[email protected]>
> > > > Sent: 11 November 2025 19:08
> > > > To: Tamar Christina <[email protected]>
> > > > Cc: [email protected]; Richard Earnshaw
> > > > <[email protected]>; Kyrylo Tkachov <[email protected]>;
> > > Alex
> > > > Coplan <[email protected]>; Andrew Pinski
> > > > <[email protected]>; Wilco Dijkstra
> > > > <[email protected]>; Alfie Richards <[email protected]>
> > > > Subject: Re: [PATCH] aarch64: Extend syntax for cpuinfo feature string
> > > > checks
> > > >
> > > > On Tue, Nov 11, 2025 at 02:04:07PM +0000, Tamar Christina wrote:
> > > > > > -----Original Message-----
> > > > > > From: Alice Carlotti <[email protected]>
> > > > > > Sent: 11 November 2025 02:05
> > > > > > To: [email protected]
> > > > > > Cc: Richard Earnshaw <[email protected]>; Tamar Christina
> > > > > > <[email protected]>; Kyrylo Tkachov <[email protected]>;
> > > > Alex
> > > > > > Coplan <[email protected]>; Andrew Pinski
> > > > > > <[email protected]>; Wilco Dijkstra
> > > > > > <[email protected]>; Alfie Richards <[email protected]>
> > > > > > Subject: [PATCH] aarch64: Extend syntax for cpuinfo feature string
> > > > > > checks
> > > > > >
> > > > > > Some SVE features in the toolchain need to be enabled when either of
> > > two
> > > > > > different kernel HWCAPS (and corresponding cpuinfo strings) are
> > > > > > enabled
> > > > > > (one for non-streaming mode and one for streaming mode).
> > > > > >
> > > > > > Add support for using "|" to separate alternative lists of required
> > > > > > features.
> > > > > >
> > > > > >
> > > > > > Bootstrapped and regression tested, and tested further by changing
> > > > > > two
> > > of
> > > > > > the
> > > > > > feature string definitions to "sve2 | blah" and "testing123 | fphp
> > > asimdhp"
> > > > > > and
> > > > > > rerunning the aarch64-cpunative.exp tests.
> > > > > >
> > > > > > This change is required for Alfie's new feature flags patch series.
> > > > > > Ok for master?
> > > > > >
> > > > > >
> > > > > > diff --git a/gcc/config/aarch64/driver-aarch64.cc
> > > > > > b/gcc/config/aarch64/driver-aarch64.cc
> > > > > > index
> > > > > >
> > > >
> > > 0333746ee00422e47a8349fad1127a1abface877..be98c5b5d1bcb33b4492
> > > > > > 7e6e365a3a39e46ee203 100644
> > > > > > --- a/gcc/config/aarch64/driver-aarch64.cc
> > > > > > +++ b/gcc/config/aarch64/driver-aarch64.cc
> > > > > > @@ -368,18 +368,30 @@ host_detect_local_cpu (int argc, const char
> > > > **argv)
> > > > > > continue;
> > > > > > }
> > > > > >
> > > > > > + /* This may be a multi-token feature string. We need to
> > > > > > match
> > > > > > + all parts in one of the "|" separated sublists. */
> > > > > > bool enabled = true;
> > > > > > -
> > > > > > - /* This may be a multi-token feature string. We need
> > > > > > - to match all parts, which could be in any order. */
> > > > > > - std::set<std::string> tokens;
> > > > > > - split_words (val, tokens);
> > > > > > - std::set<std::string>::iterator it;
> > > > > > -
> > > > > > - /* Iterate till the first feature isn't found or all of
> > > > > > them
> > > > > > - are found. */
> > > > > > - for (it = tokens.begin (); enabled && it != tokens.end
> > > > > > (); ++it)
> > > > > > - enabled = enabled && features.count (*it);
> > > > > > + size_t cur = 0;
> > > > > > + while (cur < val.length ())
> > > > > > + {
> > > > > > + size_t end = val.find_first_of (" ", cur);
> > > > > > + if (end == std::string::npos)
> > > > > > + end = val.length ();
> > > > > > + std::string word = val.substr (cur, end - cur);
> > > > > > + cur = end + 1;
> > > > > > +
> > > > > > + if (word == "|")
> > > > > > + {
> > > > > > + /* If we've matched everything in the current
> > > > > > sublist, we
> > > > > > + can stop now. */
> > > > > > + if (enabled)
> > > > > > + break;
> > > > > > + /* Otherwise, start again with the next sublist.
> > > > > > */
> > > > > > + enabled = true;
> > > > > > + continue;
> > > > > > + }
> > > > > > + enabled = enabled && features.count (word);
> > > > > > + }
> > > > >
> > > > > Overall I think this is fine, but you've now lost the short
> > > > > circuiting that was
> > > > being
> > > > > done before where the for loop before would stop as soon as enabled ==
> > > > false.
> > > > >
> > > > > Perhaps make this a nested loop instead? Where the outer loop checks
> > > > > for
> > > |
> > > > and
> > > > > the inner loop scans for the whitespace? HWCAPS strings are getting
> > > > > long
> > > so
> > > > > would be good to keep the short circuit.
> > > > >
> > > > > Thanks,
> > > > > Tamar
> > > >
> > > > We still have some short-circuiting that bypasses the set lookup when
> > > enabled
> > > > is false. I don't think it's worth the added complexity of scanning
> > > > ahead for
> > > > "|" just to avoid constructing another short string. The total impact
> > > > would
> > > be
> > > > constructing at most eight extra short strings in the entire driver
> > > > invocation,
> > > > and all of them are present on most modern systems anyway.
> > > >
> > >
> > > Sure, you have the shortcut on | but now lose it on " " which is the
> > > majority of
> > > systems running today. I don't think it's that hard to have keep the old
> > > " "
> > > shortcircuiting
> > > so don't quite see a reason to drop it.
> > >
> >
> > Btw, Patch is OK, but I would like to restore the shortcuit if possible.
> >
> > Just wanted to clarify.
>
> Thanks, I've pushed this so Alfie needn't wait longer.
I hadn't noticed my push was rejected due to a missing Changelog. It's now
actually pushed, with:
gcc/ChangeLog:
* config/aarch64/driver-aarch64.cc
(host_detect_local_cpu): Extend feature string syntax.
> I still don't
> understand what extra short-circuiting you're looking for. The
> short-circuiting of the && in
>
> + enabled = enabled && features.count (word);
>
> means that already we don't compare against the content of the cpuinfo string
> after we've missed another HWCAP feature, and the rest of the work is
> negligible.
>
> Alice
>
> >
> > Thanks,
> > Tamar
> >
> > > Thanks,
> > > Tamar
> > >
> > > > Alice
> > > >
> > > > >
> > > > > >
> > > > > > if (enabled)
> > > > > > extension_flags |= aarch64_extensions[i].flag;
