From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id D301F3857372 for ; Thu, 12 May 2022 08:02:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org D301F3857372 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 2B2B3106F; Thu, 12 May 2022 01:02:16 -0700 (PDT) Received: from localhost (e121540-lin.manchester.arm.com [10.32.98.37]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 7E84B3F73D; Thu, 12 May 2022 01:02:15 -0700 (PDT) From: Richard Sandiford To: Wilco Dijkstra via Gcc-patches Mail-Followup-To: Wilco Dijkstra via Gcc-patches , Wilco Dijkstra , richard.sandiford@arm.com Cc: Wilco Dijkstra Subject: Re: [PATCH] AArch64: Cleanup CPU option processing code References: Date: Thu, 12 May 2022 09:02:14 +0100 In-Reply-To: (Wilco Dijkstra via Gcc-patches's message of "Wed, 11 May 2022 14:44:17 +0000") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-10.7 required=5.0 tests=BAYES_00, BODY_8BITS, GIT_PATCH_0, KAM_DMARC_STATUS, KAM_LOTSOFHASH, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 12 May 2022 08:02:19 -0000 Wilco Dijkstra via Gcc-patches writes: > Hi Richard, > >> Although invoking ./cc1 directly only half-works with --with-arch, >> it half-works well-enough that I'd still like to keep it working. >> But I agree we should apply your change first, then I can follow up >> with a patch to make --with-* work with ./cc1 later.=C2=A0 (I have a ver= sion >> locally and the net result is much simpler than the status quo, as well >> as hopefully actually working properly.) > > That sounds good indeed. Is that changing TARGET_CPU_DEFAULT into > a string can could just be parsed like a -mcpu option? Yeah, it emulates the DRIVER_SELF_SPECS stuff using string macros. >> Do we still need both selected_arch and explicit_arch?=C2=A0 explicit_ar= ch >> seems a misnomer now, since it includes implicit as well as explicit >> choices.=C2=A0 Same for selected_tune and explicit_tune_core. > > At the moment we do since these are settings that must be saved/restored > since they can be overridden. Right, but I was wondering if we could save/restore them based on the selected_* values instead. However=E2=80=A6 > However it may be possible to do further cleanups to remove some of this. =E2=80=A6I agree that might as well be a separate clean-up. > I also wonder whether we can remove the internal override feature > (override_tune_string) since that further complicates the tunings, and > I'm not convinced that it is either useful or being used at all. It's a developer option rather than a user-facing thing. I found it really useful when doing the Neoverse V1 tuning, and there are some tests that rely on it. I'd prefer to keep it if possible. >> aarch64_option_restore has: >> >>=C2=A0 if (opts->x_explicit_tune_core =3D=3D aarch64_none >>=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 && opts->x_explicit_arch !=3D aarch64_no_a= rch) >>=C2=A0=C2=A0=C2=A0 selected_tune =3D &all_cores[selected_arch->ident]; >>=C2=A0 else >>=C2=A0=C2=A0=C2=A0 selected_tune =3D aarch64_get_tune_cpu (ptr->x_explici= t_tune_core); >> >> Is the =E2=80=9Cif=E2=80=9D condition ever true, or can we now restore t= he tune >> info unconditionally? > > Yes that was added a year or so after I created this patch, so this > is now redundant. I've removed it in v2: > > > The --with-cpu/--with-arch configure option processing not only checks va= lid arguments > but also sets TARGET_CPU_DEFAULT with a CPU and extension bitmask. This = isn't used > however since a --with-cpu is translated into a -mcpu option which is pro= cessed as if > written on the command-line (so TARGET_CPU_DEFAULT is never accessed). > > So remove all the complex processing and bitmask, and just validate the o= ption. > Fix a bug that always reports valid architecture extensions as invalid. = As a result > the CPU processing in aarch64.c can be simplified. > > Bootstrap OK, regress pass, OK for commit? > > ChangeLog: > 2022-04-19 Wilco Dijkstra > > * config.gcc (aarch64*-*-*): Simplify --with-cpu and --with-arch > processing. Add support for architectural extensions. > * config/aarch64/aarch64.h (TARGET_CPU_DEFAULT): Remove > AARCH64_CPU_DEFAULT_FLAGS. > (TARGET_CPU_NBITS): Remove. > (TARGET_CPU_MASK): Remove. > * config/aarch64/aarch64.cc (AARCH64_CPU_DEFAULT_FLAGS): Remove d= efine. > (get_tune_cpu): Assert CPU is always valid. > (get_arch): Assert architecture is always valid. > (aarch64_override_options): Cleanup CPU selection code and simpli= fy logic. > (aarch64_option_restore): Remove unnecessary checks on tune. Looks like you might have attached the old patch. The aarch64_option_resto= re change is mentioned in the changelog but doesn't appear in the patch itself. Thanks, Richard > > --- > > diff --git a/gcc/config.gcc b/gcc/config.gcc > index c5064dd376660c192d5573997b4fc86b6b3e3838..b48d5451e8027c93fb1f61481= 2589183d0a88c4b 100644 > --- a/gcc/config.gcc > +++ b/gcc/config.gcc > @@ -4178,8 +4178,6 @@ case "${target}" in > pattern=3DAARCH64_CORE > fi >=20=20 > - ext_mask=3DAARCH64_CPU_DEFAULT_FLAGS > - > # Find the base CPU or ARCH id in aarch64-cores.def or > # aarch64-arches.def > if [ x"$base_val" =3D x ] \ > @@ -4187,23 +4185,6 @@ case "${target}" in > ${srcdir}/config/aarch64/$def \ > > /dev/null; then >=20=20 > - if [ $which =3D arch ]; then > - base_id=3D`grep "^$pattern(\"$base_val\"," \ > - ${srcdir}/config/aarch64/$def | \ > - sed -e 's/^[^,]*,[ ]*//' | \ > - sed -e 's/,.*$//'` > - # Extract the architecture flags from aarch64-arches.def > - ext_mask=3D`grep "^$pattern(\"$base_val\"," \ > - ${srcdir}/config/aarch64/$def | \ > - sed -e 's/)$//' | \ > - sed -e 's/^.*,//'` > - else > - base_id=3D`grep "^$pattern(\"$base_val\"," \ > - ${srcdir}/config/aarch64/$def | \ > - sed -e 's/^[^,]*,[ ]*//' | \ > - sed -e 's/,.*$//'` > - fi > - > # Disallow extensions in --with-tune=3Dcortex-a53+crc. > if [ $which =3D tune ] && [ x"$ext_val" !=3D x ]; then > echo "Architecture extensions not supported in --with-$which=3D$v= al" 1>&2 > @@ -4234,25 +4215,7 @@ case "${target}" in > grep "^\"$base_ext\""` >=20=20 > if [ x"$base_ext" =3D x ] \ > - || [[ -n $opt_line ]]; then > - > - # These regexp extract the elements based on > - # their group match index in the regexp. > - ext_canon=3D`echo -e "$opt_line" | \ > - sed -e "s/$sed_patt/\2/"` > - ext_on=3D`echo -e "$opt_line" | \ > - sed -e "s/$sed_patt/\3/"` > - ext_off=3D`echo -e "$opt_line" | \ > - sed -e "s/$sed_patt/\4/"` > - > - if [ $ext =3D $base_ext ]; then > - # Adding extension > - ext_mask=3D"("$ext_mask") | ("$ext_on" | "$ext_canon")" > - else > - # Removing extension > - ext_mask=3D"("$ext_mask") & ~("$ext_off" | "$ext_canon")" > - fi > - > + || [ x"$opt_line" !=3D x ]; then > true > else > echo "Unknown extension used in --with-$which=3D$val" 1>&2 > @@ -4261,10 +4224,6 @@ case "${target}" in > ext_val=3D`echo $ext_val | sed -e 's/[a-z0-9]\+//'` > done >=20=20 > - ext_mask=3D"(("$ext_mask") << TARGET_CPU_NBITS)" > - if [ x"$base_id" !=3D x ]; then > - target_cpu_cname=3D"TARGET_CPU_$base_id | $ext_mask" > - fi > true > else > # Allow --with-$which=3Dnative. > diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h > index 359b6e8561faa38f53a806b8c114c83ae37e7e17..f835da33b72f36bbf25a0e132= 8135411bd8ab4f6 100644 > --- a/gcc/config/aarch64/aarch64.h > +++ b/gcc/config/aarch64/aarch64.h > @@ -813,16 +813,9 @@ enum target_cpus > TARGET_CPU_generic > }; >=20=20 > -/* Define how many bits are used to represent the CPU in TARGET_CPU_DEFA= ULT. > - This needs to be big enough to fit the value of TARGET_CPU_generic. > - All bits after this are used to represent the AARCH64_CPU_DEFAULT_FLA= GS. */ > -#define TARGET_CPU_NBITS 8 > -#define TARGET_CPU_MASK ((1 << TARGET_CPU_NBITS) - 1) > - > /* If there is no CPU defined at configure, use generic as default. */ > #ifndef TARGET_CPU_DEFAULT > -#define TARGET_CPU_DEFAULT \ > - (TARGET_CPU_generic | (AARCH64_CPU_DEFAULT_FLAGS << TARGET_CPU_NBITS)) > +# define TARGET_CPU_DEFAULT TARGET_CPU_generic > #endif >=20=20 > /* If inserting NOP before a mult-accumulate insn remember to adjust the > diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc > index f650abbc4ce49cf0947049931f86bad1130c3428..43d87d1b9c4ef1a85094e51f8= 1745f98f1ef27fb 100644 > --- a/gcc/config/aarch64/aarch64.cc > +++ b/gcc/config/aarch64/aarch64.cc > @@ -2760,8 +2760,6 @@ static const struct attribute_spec aarch64_attribut= e_table[] =3D > { NULL, 0, 0, false, false, false, false, NULL, NULL } > }; >=20=20 > -#define AARCH64_CPU_DEFAULT_FLAGS ((selected_cpu) ? selected_cpu->flags = : 0) > - > /* An ISA extension in the co-processor and main instruction set space. = */ > struct aarch64_option_extension > { > @@ -18057,39 +18055,24 @@ aarch64_validate_mtune (const char *str, const = struct processor **res) > return false; > } >=20=20 > -static_assert (TARGET_CPU_generic < TARGET_CPU_MASK, > - "TARGET_CPU_NBITS is big enough"); > - > -/* Return the CPU corresponding to the enum CPU. > - If it doesn't specify a cpu, return the default. */ > +/* Return the CPU corresponding to the enum CPU. */ >=20=20 > static const struct processor * > aarch64_get_tune_cpu (enum aarch64_processor cpu) > { > - if (cpu !=3D aarch64_none) > - return &all_cores[cpu]; > + gcc_assert (cpu !=3D aarch64_none); >=20=20 > - /* The & TARGET_CPU_MASK is to extract the bottom TARGET_CPU_NBITS bit= s that > - encode the default cpu as selected by the --with-cpu GCC configure = option > - in config.gcc. > - ???: The whole TARGET_CPU_DEFAULT and AARCH64_CPU_DEFAULT_FLAGS > - flags mechanism should be reworked to make it more sane. */ > - return &all_cores[TARGET_CPU_DEFAULT & TARGET_CPU_MASK]; > + return &all_cores[cpu]; > } >=20=20 > -/* Return the architecture corresponding to the enum ARCH. > - If it doesn't specify a valid architecture, return the default. */ > +/* Return the architecture corresponding to the enum ARCH. */ >=20=20 > static const struct processor * > aarch64_get_arch (enum aarch64_arch arch) > { > - if (arch !=3D aarch64_no_arch) > - return &all_architectures[arch]; > - > - const struct processor *cpu > - =3D &all_cores[TARGET_CPU_DEFAULT & TARGET_CPU_MASK]; > + gcc_assert (arch !=3D aarch64_no_arch); >=20=20 > - return &all_architectures[cpu->arch]; > + return &all_architectures[arch]; > } >=20=20 > /* Return the VG value associated with -msve-vector-bits=3D value VALUE.= */ > @@ -18127,10 +18110,6 @@ aarch64_override_options (void) > uint64_t arch_isa =3D 0; > aarch64_isa_flags =3D 0; >=20=20 > - bool valid_cpu =3D true; > - bool valid_tune =3D true; > - bool valid_arch =3D true; > - > selected_cpu =3D NULL; > selected_arch =3D NULL; > selected_tune =3D NULL; > @@ -18145,77 +18124,56 @@ aarch64_override_options (void) > If either of -march or -mtune is given, they override their > respective component of -mcpu. */ > if (aarch64_cpu_string) > - valid_cpu =3D aarch64_validate_mcpu (aarch64_cpu_string, &selected_c= pu, > - &cpu_isa); > + aarch64_validate_mcpu (aarch64_cpu_string, &selected_cpu, &cpu_isa); >=20=20 > if (aarch64_arch_string) > - valid_arch =3D aarch64_validate_march (aarch64_arch_string, &selecte= d_arch, > - &arch_isa); > + aarch64_validate_march (aarch64_arch_string, &selected_arch, &arch_i= sa); >=20=20 > if (aarch64_tune_string) > - valid_tune =3D aarch64_validate_mtune (aarch64_tune_string, &selecte= d_tune); > + aarch64_validate_mtune (aarch64_tune_string, &selected_tune); >=20=20 > #ifdef SUBTARGET_OVERRIDE_OPTIONS > SUBTARGET_OVERRIDE_OPTIONS; > #endif >=20=20 > - /* If the user did not specify a processor, choose the default > - one for them. This will be the CPU set during configuration using > - --with-cpu, otherwise it is "generic". */ > - if (!selected_cpu) > - { > - if (selected_arch) > - { > - selected_cpu =3D &all_cores[selected_arch->ident]; > - aarch64_isa_flags =3D arch_isa; > - explicit_arch =3D selected_arch->arch; > - } > - else > - { > - /* Get default configure-time CPU. */ > - selected_cpu =3D aarch64_get_tune_cpu (aarch64_none); > - aarch64_isa_flags =3D TARGET_CPU_DEFAULT >> TARGET_CPU_NBITS; > - } > - > - if (selected_tune) > - explicit_tune_core =3D selected_tune->ident; > - } > - /* If both -mcpu and -march are specified check that they are architec= turally > - compatible, warn if they're not and prefer the -march ISA flags. */ > - else if (selected_arch) > + if (selected_cpu && selected_arch) > { > + /* If both -mcpu and -march are specified, warn if they are not > + architecturally compatible and prefer the -march ISA flags. */ > if (selected_arch->arch !=3D selected_cpu->arch) > { > warning (0, "switch %<-mcpu=3D%s%> conflicts with %<-march=3D%s%> swi= tch", > aarch64_cpu_string, > aarch64_arch_string); > } > + > aarch64_isa_flags =3D arch_isa; > - explicit_arch =3D selected_arch->arch; > - explicit_tune_core =3D selected_tune ? selected_tune->ident > - : selected_cpu->ident; > } > - else > + else if (selected_cpu) > { > - /* -mcpu but no -march. */ > - aarch64_isa_flags =3D cpu_isa; > - explicit_tune_core =3D selected_tune ? selected_tune->ident > - : selected_cpu->ident; > - gcc_assert (selected_cpu); > selected_arch =3D &all_architectures[selected_cpu->arch]; > - explicit_arch =3D selected_arch->arch; > + aarch64_isa_flags =3D cpu_isa; > } > - > - /* Set the arch as well as we will need it when outputing > - the .arch directive in assembly. */ > - if (!selected_arch) > + else if (selected_arch) > { > - gcc_assert (selected_cpu); > + selected_cpu =3D &all_cores[selected_arch->ident]; > + aarch64_isa_flags =3D arch_isa; > + } > + else > + { > + /* No -mcpu or -march specified, so use the default CPU. */ > + selected_cpu =3D &all_cores[TARGET_CPU_DEFAULT]; > selected_arch =3D &all_architectures[selected_cpu->arch]; > + aarch64_isa_flags =3D selected_cpu->flags; > } >=20=20 > + explicit_arch =3D selected_arch->arch; > if (!selected_tune) > selected_tune =3D selected_cpu; > + explicit_tune_core =3D selected_tune->ident; > + > + gcc_assert (explicit_tune_core !=3D aarch64_none); > + gcc_assert (explicit_arch !=3D aarch64_no_arch); >=20=20 > if (aarch64_enable_bti =3D=3D 2) > { > @@ -18251,15 +18209,6 @@ aarch64_override_options (void) > if (aarch64_ra_sign_scope !=3D AARCH64_FUNCTION_NONE && TARGET_ILP32) > sorry ("return address signing is only supported for %<-mabi=3Dlp64%= >"); >=20=20 > - /* Make sure we properly set up the explicit options. */ > - if ((aarch64_cpu_string && valid_cpu) > - || (aarch64_tune_string && valid_tune)) > - gcc_assert (explicit_tune_core !=3D aarch64_none); > - > - if ((aarch64_cpu_string && valid_cpu) > - || (aarch64_arch_string && valid_arch)) > - gcc_assert (explicit_arch !=3D aarch64_no_arch); > - > /* The pass to insert speculation tracking runs before > shrink-wrapping and the latter does not know how to update the > tracking status. So disable it in this case. */