Hi Alfie, Thanks for taking the comments on board, a few more notes below.
On 16/01/2026 12:11, Alfie Richards wrote: > Hi All, > > Some changes here to make the changes requested by Alex Copland. > (thank you for the review, I think it is a definite improvement) > > I also mirrored the requested changes onto the comment in the > builtins files as they were equally applicable there. Sorry, I didn't notice the changes to the builtins files in the previous review, comments on those inline below. > > Okay for master? > > Thanks, > Alfie > > > -- >8 -- > > gcc/ChangeLog: > > * config/aarch64/aarch64.md: Update comment. > * config/aarch64/aarch64-simd.md: Change comment to refer to > aarch64.md. > * config/aarch64/aarch64-sme.md: Likewise. > * config/aarch64/aarch64-sve.md: Likewise. > * config/aarch64/aarch64-sve2.md: Likewise. > * config/aarch64/aarch64-sve-builtins.def: Update comment. > * config/aarch64/aarch64-sve-builtins-base.def: Update to refer > to aarch64-sve-builtins.def. > * config/aarch64/aarch64-sve-builtins-sme.def: Likewise. > * config/aarch64/aarch64-sve-builtins-sve2.def: Likewise. > --- > gcc/config/aarch64/aarch64-simd.md | 18 +---------------- > gcc/config/aarch64/aarch64-sme.md | 18 +---------------- > .../aarch64/aarch64-sve-builtins-base.def | 17 +--------------- > .../aarch64/aarch64-sve-builtins-sme.def | 17 +--------------- > .../aarch64/aarch64-sve-builtins-sve2.def | 17 +--------------- > gcc/config/aarch64/aarch64-sve-builtins.def | 13 ++++++------ > gcc/config/aarch64/aarch64-sve.md | 18 +---------------- > gcc/config/aarch64/aarch64-sve2.md | 20 ++----------------- > gcc/config/aarch64/aarch64.md | 10 +++++----- > 9 files changed, 19 insertions(+), 129 deletions(-) > > diff --git a/gcc/config/aarch64/aarch64-simd.md > b/gcc/config/aarch64/aarch64-simd.md > index 7a38310efce..44d90cab575 100644 > --- a/gcc/config/aarch64/aarch64-simd.md > +++ b/gcc/config/aarch64/aarch64-simd.md > @@ -18,23 +18,7 @@ > ;; along with GCC; see the file COPYING3. If not see > ;; <http://www.gnu.org/licenses/>. > > -;; Code organisation: > -; > -;; The lines of is an instruction is aarch64, simd, sve, sve2, or sme are > -;; a little blurry. > -;; > -;; Therefore code is organised by the following rough principles: > -;; > -;; - aarch64.md: For shared parts of the architecture (such as defining > -;; registers and constants) and for instructions that operate on non-SIMD > -;; registers. > -;; - aarch64-simd.md: For instructions that operate on non-scaling SIMD > -;; registers. > -;; - aarch64-sve.md for SVE instructions that are pre SVE2. > -;; - aarch64-sme.md for any scalable SIMD instruction that is incompatible > with > -;; non-streaming mode. This usually means it uses the ZA or ZT register. > -;; - aarch64-sve2.md for any scalable SIMD instruction that either is > -;; streaming compatible, or theoretically could be later. > +;; Code organisation: See comment in aarch64.md. GCC coding style is to use American spelling, so this needs to be "organization". Perhaps we should be a bit more specific, say: "See block comment at the top of aarch64.md." otherwise the reader might not know where to look. Same applies for the other occurrences in the rest of the patch, of course. > > ;; The following define_subst rules are used to produce patterns representing > ;; the implicit zeroing effect of 64-bit Advanced SIMD operations, in effect > diff --git a/gcc/config/aarch64/aarch64-sme.md > b/gcc/config/aarch64/aarch64-sme.md > index a2faa4b2bb7..871b93ad0af 100644 > --- a/gcc/config/aarch64/aarch64-sme.md > +++ b/gcc/config/aarch64/aarch64-sme.md > @@ -17,23 +17,7 @@ > ;; along with GCC; see the file COPYING3. If not see > ;; <http://www.gnu.org/licenses/>. > > -;; Code organisation: > -; > -;; The lines of is an instruction is aarch64, simd, sve, sve2, or sme are > -;; a little blurry. > -;; > -;; Therefore code is organised by the following rough principles: > -;; > -;; - aarch64.md: For shared parts of the architecture (such as defining > -;; registers and constants) and for instructions that operate on non-SIMD > -;; registers. > -;; - aarch64-simd.md: For instructions that operate on non-scaling SIMD > -;; registers. > -;; - aarch64-sve.md for SVE instructions that are pre SVE2. > -;; - aarch64-sme.md for any scalable SIMD instruction that is incompatible > with > -;; non-streaming mode. This usually means it uses the ZA or ZT register. > -;; - aarch64-sve2.md for any scalable SIMD instruction that either is > -;; streaming compatible, or theoretically could be later. > +;; Code organisation: See comment in aarch64.md. > > ;; The file is organised into the following sections (search for the full > ;; line): > diff --git a/gcc/config/aarch64/aarch64-sve-builtins-base.def > b/gcc/config/aarch64/aarch64-sve-builtins-base.def > index 0a3a7a0e144..4c1e32e8703 100644 > --- a/gcc/config/aarch64/aarch64-sve-builtins-base.def > +++ b/gcc/config/aarch64/aarch64-sve-builtins-base.def > @@ -17,22 +17,7 @@ > along with GCC; see the file COPYING3. If not see > <http://www.gnu.org/licenses/>. */ > > -/* Code organisation: > - > - The lines defining if an intrinsic is for sve, sve2, sme, and sme2 can get > - a little blurry. > - > - Therefore code is organised by the following rough principles: > - > - - aarch64-sve-builtins-sme.def for any intrinsic that is fundamentally > - incompatible with non-streaming mode. This usually means it uses the ZA > - or ZT register. > - - aarch64-sve-builtins-sve2.def for any intrinsic that that is a > streaming mode > - intrinsic, but either is non-streaming compatible, or theoretically > could > - be later. > - - aarch64-sve-builtins-base.def for SVE intrinsics that are pre SVE2. > - - aarch64-sve-builtins.def for common data types and group definitions > used > - across all files. */ > +/* Code organisation: see comment in aarch-sve-builtins.def. */ "organization" and "aarch64-sve-builtins.def". > > #define REQUIRED_EXTENSIONS ssve (0) > DEF_SVE_FUNCTION (svabd, binary_opt_n, all_arith, mxz) > diff --git a/gcc/config/aarch64/aarch64-sve-builtins-sme.def > b/gcc/config/aarch64/aarch64-sve-builtins-sme.def > index 9f126e8647a..a78a847b6b4 100644 > --- a/gcc/config/aarch64/aarch64-sve-builtins-sme.def > +++ b/gcc/config/aarch64/aarch64-sve-builtins-sme.def > @@ -17,22 +17,7 @@ > along with GCC; see the file COPYING3. If not see > <http://www.gnu.org/licenses/>. */ > > -/* Code organisation: > - > - The lines defining if an intrinsic is for sve, sve2, sme, and sme2 can get > - a little blurry. > - > - Therefore code is organised by the following rough principles: > - > - - aarch64-sve-builtins-sme.def for any intrinsic that is fundamentally > - incompatible with non-streaming mode. This usually means it uses the ZA > - or ZT register. > - - aarch64-sve-builtins-sve2.def for any intrinsic that that is a > streaming mode > - intrinsic, but either is non-streaming compatible, or theoretically > could > - be later. > - - aarch64-sve-builtins-base.def for SVE intrinsics that are pre SVE2. > - - aarch64-sve-builtins.def for common data types and group definitions > used > - across all files. */ > +/* Code organisation: see comment in aarch-sve-builtins.def. */ Likewise. > > #ifndef DEF_SME_FUNCTION_GS > #define DEF_SME_FUNCTION_GS(NAME, SHAPE, TYPES, GROUPS, PREDS) \ > diff --git a/gcc/config/aarch64/aarch64-sve-builtins-sve2.def > b/gcc/config/aarch64/aarch64-sve-builtins-sve2.def > index 8765021d951..c526df96f24 100644 > --- a/gcc/config/aarch64/aarch64-sve-builtins-sve2.def > +++ b/gcc/config/aarch64/aarch64-sve-builtins-sve2.def > @@ -17,22 +17,7 @@ > along with GCC; see the file COPYING3. If not see > <http://www.gnu.org/licenses/>. */ > > -/* Code organisation: > - > - The lines defining if an intrinsic is for sve, sve2, sme, and sme2 can get > - a little blurry. > - > - Therefore code is organised by the following rough principles: > - > - - aarch64-sve-builtins-sme.def for any intrinsic that is fundamentally > - incompatible with non-streaming mode. This usually means it uses the ZA > - or ZT register. > - - aarch64-sve-builtins-sve2.def for any intrinsic that that is a > streaming mode > - intrinsic, but either is non-streaming compatible, or theoretically > could > - be later. > - - aarch64-sve-builtins-base.def for SVE intrinsics that are pre SVE2. > - - aarch64-sve-builtins.def for common data types and group definitions > used > - across all files. */ > +/* Code organisation: see comment in aarch-sve-builtins.def. */ Likewise. > > #define REQUIRED_EXTENSIONS sve_and_sme (AARCH64_FL_SVE2, 0) > DEF_SVE_FUNCTION (svaba, ternary_opt_n, all_integer, none) > diff --git a/gcc/config/aarch64/aarch64-sve-builtins.def > b/gcc/config/aarch64/aarch64-sve-builtins.def > index ecd434bda35..93e12b7b750 100644 > --- a/gcc/config/aarch64/aarch64-sve-builtins.def > +++ b/gcc/config/aarch64/aarch64-sve-builtins.def > @@ -19,20 +19,19 @@ > > /* Code organisation: "organization" > > - The lines defining if an intrinsic is for sve, sve2, sme, and sme2 can get > + The taxinomic lines dividing intrinsics into sve, sve2, sme, and sme2 are "taxonomic" > a little blurry. I think "a little blurry" is perhpas a bit unfair here, to what is some pretty well-organized code. How about "perhaps a little non-obvious" instead? > > Therefore code is organised by the following rough principles: "organized" > > + - aarch64-sve-builtins.def for common data types and group definitions > used > + across all files. How about "common data types, groups, and other supporting definitions used ..." > - aarch64-sve-builtins-sme.def for any intrinsic that is fundamentally > incompatible with non-streaming mode. This usually means it uses the ZA Instead of "fundamentally incompatible with non-streaming mode", how about "only useable in streaming mode"? Either way is fine by me. > or ZT register. > - - aarch64-sve-builtins-sve2.def for any intrinsic that that is a > streaming mode > - intrinsic, but either is non-streaming compatible, or theoretically > could > - be later. > - - aarch64-sve-builtins-base.def for SVE intrinsics that are pre SVE2. > - - aarch64-sve-builtins.def for common data types and group definitions > used > - across all files. */ > + - aarch64-sve-builtins-sve2.def for any scalable SIMD intrinsic that is > + enabled by an SVE2+ extension. > + - aarch64-sve-builtins-base.def for SVE intrinsics that are pre SVE2. */ Sorry for not spotting this earlier, but can we sort this list chronologically by when the relevant extension was introduced? So after aarch64-sve-builtins.def, we'd have {base,sve,sve2,sme}. > > #ifndef DEF_SVE_MODE > #define DEF_SVE_MODE(A, B, C, D) > diff --git a/gcc/config/aarch64/aarch64-sve.md > b/gcc/config/aarch64/aarch64-sve.md > index b77a04cd2d2..2b16eb90976 100644 > --- a/gcc/config/aarch64/aarch64-sve.md > +++ b/gcc/config/aarch64/aarch64-sve.md > @@ -18,23 +18,7 @@ > ;; along with GCC; see the file COPYING3. If not see > ;; <http://www.gnu.org/licenses/>. > > -;; Code organisation: > -; > -;; The lines of is an instruction is aarch64, simd, sve, sve2, or sme are > -;; a little blurry. > -;; > -;; Therefore code is organised by the following rough principles: > -;; > -;; - aarch64.md: For shared parts of the architecture (such as defining > -;; registers and constants) and for instructions that operate on non-SIMD > -;; registers. > -;; - aarch64-simd.md: For instructions that operate on non-scaling SIMD > -;; registers. > -;; - aarch64-sve.md for SVE instructions that are pre SVE2. > -;; - aarch64-sme.md for any scalable SIMD instruction that is incompatible > with > -;; non-streaming mode. This usually means it uses the ZA or ZT register. > -;; - aarch64-sve2.md for any scalable SIMD instruction that either is > -;; streaming compatible, or theoretically could be later. > +;; Code organisation: See comment in aarch64.md. > > ;; The file is organised into the following sections (search for the full > ;; line): > diff --git a/gcc/config/aarch64/aarch64-sve2.md > b/gcc/config/aarch64/aarch64-sve2.md > index f959837eca0..cea919b54cf 100644 > --- a/gcc/config/aarch64/aarch64-sve2.md > +++ b/gcc/config/aarch64/aarch64-sve2.md > @@ -17,24 +17,8 @@ > ;; You should have received a copy of the GNU General Public License > ;; along with GCC; see the file COPYING3. If not see > ;; <http://www.gnu.org/licenses/>. > -; > -;; Code organisation: > -; > -;; The lines of is an instruction is aarch64, simd, sve, sve2, or sme are > -;; a little blurry. > -;; > -;; Therefore code is organised by the following rough principles: > -;; > -;; - aarch64.md: For shared parts of the architecture (such as defining > -;; registers and constants) and for instructions that operate on non-SIMD > -;; registers. > -;; - aarch64-simd.md: For instructions that operate on non-scaling SIMD > -;; registers. > -;; - aarch64-sve.md for SVE instructions that are pre SVE2. > -;; - aarch64-sme.md for any scalable SIMD instruction that is incompatible > with > -;; non-streaming mode. This usually means it uses the ZA or ZT register. > -;; - aarch64-sve2.md for any scalable SIMD instruction that either is > -;; streaming compatible, or theoretically could be later. > + > +;; Code organisation: See comment in aarch64.md. > > ;; The file is organised into the following sections (search for the full > ;; line): > diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md > index 58e57fd65a7..f0841c0adf3 100644 > --- a/gcc/config/aarch64/aarch64.md > +++ b/gcc/config/aarch64/aarch64.md > @@ -20,21 +20,21 @@ > > ;; Code organisation: > ; > -;; The lines of is an instruction is aarch64, simd, sve, sve2, or sme are > -;; a little blurry. > +;; The tavonomic lines dividing instrucitons into aarch64, simd, sve, sve2, "taxonomic" > +;; and sme are a little blurry. > ;; > ;; Therefore code is organised by the following rough principles: "organized" > ;; > ;; - aarch64.md: For shared parts of the architecture (such as defining > ;; registers and constants) and for instructions that operate on non-SIMD > ;; registers. I think "general-purpose registers" might read better than non-SIMD registers, if you agree? Sorry for not mentioning that last time round. > -;; - aarch64-simd.md: For instructions that operate on non-scaling SIMD > +;; - aarch64-simd.md: For instructions that operate on Advanced SIMD > ;; registers. > ;; - aarch64-sve.md for SVE instructions that are pre SVE2. > ;; - aarch64-sme.md for any scalable SIMD instruction that is incompatible > with > ;; non-streaming mode. This usually means it uses the ZA or ZT register. > -;; - aarch64-sve2.md for any scalable SIMD instruction that either is > -;; streaming compatible, or theoretically could be later. > +;; - aarch64-sve2.md for any scalable SIMD instruction that is enabled by an > +;; SVE2+ extension. Please can we swap the sve2 and sme entries so that the list is sorted chronologically by when the extension was introduced? We should have aarch64.md followed by aarch64-{simd,sve,sve2,sme}.md. Hope this is constructive. Thanks, Alex > > ;; Register numbers > (define_constants > -- > 2.34.1 >
