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 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;
