| Message ID | ZVT7EuX7I1X0+xfV@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/6] AArch64: Refactor costs models to different files. | expand |
On 15/11/2023 17:08, Tamar Christina wrote: > Hi All, > > At the moment we emit a warning whenever you specify both -march and -mcpu > and the architecture of them differ. The idea originally was that the user may > not be aware of this change. > > However this has a few problems: > > 1. Architecture revisions is not an observable part of the architecture, > extensions are. Starting with GCC 14 we have therefore relaxed the rule that > all extensions can be enabled at any architecture level. Therefore it's > incorrect, or at least not useful to keep the check on architecture. > > 2. It's problematic in Makefiles and other build systems, where you want to > for certain files enable CPU specific builds. i.e. you may be by default > building for -march=armv8-a but for some file for -mcpu=neoverse-n1. Since > there's no easy way to remove the earlier options we end up warning and > there's no way to disable just this warning. Build systems compiling with > -Werror face an issue in this case that compiling with GCC is needlessly > hard. > > 3. It doesn't actually warn for cases that may lead to issues, so e.g. > -march=armv8.2-a+sve -mcpu=neoverse-n1 does not give a warning that SVE would > be disabled. > > For this reason I have one of two proposals: > > 1. Just remove this warning all together. > > 2. Rework the warning based on extensions and only warn when features would be > disabled by the presence of the -mcpu. This is the approach this patch has > taken. There's a third option here, which is what I plan to add for the Arm port: 3. Add -mcpu=unset and -march=unset support in the driver, which has the effect of suppressing any earlier option that sets that flag. [BTW: patch 5 seems to be missing so I'm holding off on approving this now.] R. > > As examples: > >> aarch64-none-linux-gnu-gcc -march=armv8.2-a+sve -mcpu=neoverse-n1 > cc1: warning: switch ‘-mcpu=neoverse-n1’ conflicts with ‘-march=armv8.2-a+sve’ switch and resulted in options +crc+sve+norcpc+nodotprod being added .arch armv8.2-a+crc+sve > >> aarch64-none-linux-gnu-gcc -march=armv8.2-a -mcpu=neoverse-n1 >> aarch64-none-linux-gnu-gcc -march=armv8.2-a+dotprod -mcpu=neoverse-n1 >> aarch64-none-linux-gnu-gcc -march=armv8.2-a+dotprod -mcpu=neoverse-n2 > <no warning> > > The one remaining issue here is that if both -march and -mcpu are specified we > pick the -march. This is not particularly obvious and for the use case to be > more useful I think it makes sense to pick the CPU's arch? > > I did not make that change in the patch as it changes semantics. > > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. > > Note that I can't write a test for this because dg-warning expects warnings to > be at a particular line and doesn't support warnings at the "global" level. > > Ok for master? > > Thanks, > Tamar > > gcc/ChangeLog: > > * config/aarch64/aarch64.cc (aarch64_override_options): Rework warnings. > > --- inline copy of patch -- > diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc > index caf80d66b3a744cc93899645aa5f9374983cd3db..3afd222ad3bdcfb922cc010dcc0b138db29caf7f 100644 > --- a/gcc/config/aarch64/aarch64.cc > +++ b/gcc/config/aarch64/aarch64.cc > @@ -16388,12 +16388,22 @@ aarch64_override_options (void) > if (cpu && arch) > { > /* If both -mcpu and -march are specified, warn if they are not > - architecturally compatible and prefer the -march ISA flags. */ > - if (arch->arch != cpu->arch) > - { > - warning (0, "switch %<-mcpu=%s%> conflicts with %<-march=%s%> switch", > + feature compatible. feature compatible means that the inclusion of the > + cpu features would end up disabling an achitecture feature. In > + otherwords the cpu features need to be a strict superset of the arch > + features and if so prefer the -march ISA flags. */ > + auto full_arch_flags = arch->flags | arch_isa; > + auto full_cpu_flags = cpu->flags | cpu_isa; > + if (~full_cpu_flags & full_arch_flags) > + { > + std::string ext_diff > + = aarch64_get_extension_string_for_isa_flags (full_arch_flags, > + full_cpu_flags); > + warning (0, "switch %<-mcpu=%s%> conflicts with %<-march=%s%> switch " > + "and resulted in options %s being added", > aarch64_cpu_string, > - aarch64_arch_string); > + aarch64_arch_string, > + ext_diff.c_str ()); > } > > selected_arch = arch->arch; > > > >
> -----Original Message----- > From: Richard Earnshaw <Richard.Earnshaw@foss.arm.com> > Sent: Thursday, November 16, 2023 9:27 AM > To: Tamar Christina <Tamar.Christina@arm.com>; gcc-patches@gcc.gnu.org > Cc: nd <nd@arm.com>; Richard Earnshaw <Richard.Earnshaw@arm.com>; > Marcus Shawcroft <Marcus.Shawcroft@arm.com>; Kyrylo Tkachov > <Kyrylo.Tkachov@arm.com>; Richard Sandiford > <Richard.Sandiford@arm.com> > Subject: Re: [PATCH 6/6]AArch64: only emit mismatch error when features > would be disabled. > > > > On 15/11/2023 17:08, Tamar Christina wrote: > > Hi All, > > > > At the moment we emit a warning whenever you specify both -march and > > -mcpu and the architecture of them differ. The idea originally was > > that the user may not be aware of this change. > > > > However this has a few problems: > > > > 1. Architecture revisions is not an observable part of the architecture, > > extensions are. Starting with GCC 14 we have therefore relaxed the rule > that > > all extensions can be enabled at any architecture level. Therefore it's > > incorrect, or at least not useful to keep the check on architecture. > > > > 2. It's problematic in Makefiles and other build systems, where you want to > > for certain files enable CPU specific builds. i.e. you may be by default > > building for -march=armv8-a but for some file for -mcpu=neoverse-n1. > Since > > there's no easy way to remove the earlier options we end up warning and > > there's no way to disable just this warning. Build systems compiling with > > -Werror face an issue in this case that compiling with GCC is needlessly > > hard. > > > > 3. It doesn't actually warn for cases that may lead to issues, so e.g. > > -march=armv8.2-a+sve -mcpu=neoverse-n1 does not give a warning that > SVE would > > be disabled. > > > > For this reason I have one of two proposals: > > > > 1. Just remove this warning all together. > > > > 2. Rework the warning based on extensions and only warn when features > would be > > disabled by the presence of the -mcpu. This is the approach this patch has > > taken. > > There's a third option here, which is what I plan to add for the Arm port: > > 3. Add -mcpu=unset and -march=unset support in the driver, which has the > effect of suppressing any earlier option that sets that flag. > > [BTW: patch 5 seems to be missing so I'm holding off on approving this now.] > Ah sorry, I should have re-numbered this series. Patch 5 was sent earlier to unblock an internal team. It was https://gcc.gnu.org/pipermail/gcc-patches/2023-October/632802.html Thanks, Tamar > R. > > > > > As examples: > > > >> aarch64-none-linux-gnu-gcc -march=armv8.2-a+sve -mcpu=neoverse-n1 > > cc1: warning: switch ‘-mcpu=neoverse-n1’ conflicts with ‘-march=armv8.2- > a+sve’ switch and resulted in options +crc+sve+norcpc+nodotprod being > added > .arch armv8.2-a+crc+sve > > > >> aarch64-none-linux-gnu-gcc -march=armv8.2-a -mcpu=neoverse-n1 > >> aarch64-none-linux-gnu-gcc -march=armv8.2-a+dotprod -mcpu=neoverse- > n1 > >> aarch64-none-linux-gnu-gcc -march=armv8.2-a+dotprod -mcpu=neoverse- > n2 > > <no warning> > > > > The one remaining issue here is that if both -march and -mcpu are > > specified we pick the -march. This is not particularly obvious and > > for the use case to be more useful I think it makes sense to pick the CPU's > arch? > > > > I did not make that change in the patch as it changes semantics. > > > > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. > > > > Note that I can't write a test for this because dg-warning expects > > warnings to be at a particular line and doesn't support warnings at the > "global" level. > > > > Ok for master? > > > > Thanks, > > Tamar > > > > gcc/ChangeLog: > > > > * config/aarch64/aarch64.cc (aarch64_override_options): Rework > warnings. > > > > --- inline copy of patch -- > > diff --git a/gcc/config/aarch64/aarch64.cc > > b/gcc/config/aarch64/aarch64.cc index > > > caf80d66b3a744cc93899645aa5f9374983cd3db..3afd222ad3bdcfb922cc01 > 0dcc0b > > 138db29caf7f 100644 > > --- a/gcc/config/aarch64/aarch64.cc > > +++ b/gcc/config/aarch64/aarch64.cc > > @@ -16388,12 +16388,22 @@ aarch64_override_options (void) > > if (cpu && arch) > > { > > /* If both -mcpu and -march are specified, warn if they are not > > - architecturally compatible and prefer the -march ISA flags. */ > > - if (arch->arch != cpu->arch) > > - { > > - warning (0, "switch %<-mcpu=%s%> conflicts with %<-march=%s%> > switch", > > + feature compatible. feature compatible means that the inclusion of > the > > + cpu features would end up disabling an achitecture feature. In > > + otherwords the cpu features need to be a strict superset of the arch > > + features and if so prefer the -march ISA flags. */ > > + auto full_arch_flags = arch->flags | arch_isa; > > + auto full_cpu_flags = cpu->flags | cpu_isa; > > + if (~full_cpu_flags & full_arch_flags) > > + { > > + std::string ext_diff > > + = aarch64_get_extension_string_for_isa_flags (full_arch_flags, > > + full_cpu_flags); > > + warning (0, "switch %<-mcpu=%s%> conflicts with %<-march=%s%> > switch " > > + "and resulted in options %s being added", > > aarch64_cpu_string, > > - aarch64_arch_string); > > + aarch64_arch_string, > > + ext_diff.c_str ()); > > } > > > > selected_arch = arch->arch; > > > > > > > >
On 16/11/2023 09:33, Tamar Christina wrote: >> -----Original Message----- >> From: Richard Earnshaw <Richard.Earnshaw@foss.arm.com> >> Sent: Thursday, November 16, 2023 9:27 AM >> To: Tamar Christina <Tamar.Christina@arm.com>; gcc-patches@gcc.gnu.org >> Cc: nd <nd@arm.com>; Richard Earnshaw <Richard.Earnshaw@arm.com>; >> Marcus Shawcroft <Marcus.Shawcroft@arm.com>; Kyrylo Tkachov >> <Kyrylo.Tkachov@arm.com>; Richard Sandiford >> <Richard.Sandiford@arm.com> >> Subject: Re: [PATCH 6/6]AArch64: only emit mismatch error when features >> would be disabled. >> >> >> >> On 15/11/2023 17:08, Tamar Christina wrote: >>> Hi All, >>> >>> At the moment we emit a warning whenever you specify both -march and >>> -mcpu and the architecture of them differ. The idea originally was >>> that the user may not be aware of this change. >>> >>> However this has a few problems: >>> >>> 1. Architecture revisions is not an observable part of the architecture, >>> extensions are. Starting with GCC 14 we have therefore relaxed the rule >> that >>> all extensions can be enabled at any architecture level. Therefore it's >>> incorrect, or at least not useful to keep the check on architecture. >>> >>> 2. It's problematic in Makefiles and other build systems, where you want to >>> for certain files enable CPU specific builds. i.e. you may be by default >>> building for -march=armv8-a but for some file for -mcpu=neoverse-n1. >> Since >>> there's no easy way to remove the earlier options we end up warning and >>> there's no way to disable just this warning. Build systems compiling with >>> -Werror face an issue in this case that compiling with GCC is needlessly >>> hard. >>> >>> 3. It doesn't actually warn for cases that may lead to issues, so e.g. >>> -march=armv8.2-a+sve -mcpu=neoverse-n1 does not give a warning that >> SVE would >>> be disabled. >>> >>> For this reason I have one of two proposals: >>> >>> 1. Just remove this warning all together. >>> >>> 2. Rework the warning based on extensions and only warn when features >> would be >>> disabled by the presence of the -mcpu. This is the approach this patch has >>> taken. >> >> There's a third option here, which is what I plan to add for the Arm port: >> >> 3. Add -mcpu=unset and -march=unset support in the driver, which has the >> effect of suppressing any earlier option that sets that flag. >> >> [BTW: patch 5 seems to be missing so I'm holding off on approving this now.] >> > > Ah sorry, I should have re-numbered this series. Patch 5 was sent earlier to unblock > an internal team. It was https://gcc.gnu.org/pipermail/gcc-patches/2023-October/632802.html Ah, OK. So going back to your option 2. What should happen if the user specified -mcpu=cortex-r82 and then specifies an extension that doesn't exist in the R profile? R. > > Thanks, > Tamar >> R. >> >>> >>> As examples: >>> >>>> aarch64-none-linux-gnu-gcc -march=armv8.2-a+sve -mcpu=neoverse-n1 >>> cc1: warning: switch ‘-mcpu=neoverse-n1’ conflicts with ‘-march=armv8.2- >> a+sve’ switch and resulted in options +crc+sve+norcpc+nodotprod being >> added >> .arch armv8.2-a+crc+sve >>> >>>> aarch64-none-linux-gnu-gcc -march=armv8.2-a -mcpu=neoverse-n1 >>>> aarch64-none-linux-gnu-gcc -march=armv8.2-a+dotprod -mcpu=neoverse- >> n1 >>>> aarch64-none-linux-gnu-gcc -march=armv8.2-a+dotprod -mcpu=neoverse- >> n2 >>> <no warning> >>> >>> The one remaining issue here is that if both -march and -mcpu are >>> specified we pick the -march. This is not particularly obvious and >>> for the use case to be more useful I think it makes sense to pick the CPU's >> arch? >>> >>> I did not make that change in the patch as it changes semantics. >>> >>> Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. >>> >>> Note that I can't write a test for this because dg-warning expects >>> warnings to be at a particular line and doesn't support warnings at the >> "global" level. >>> >>> Ok for master? >>> >>> Thanks, >>> Tamar >>> >>> gcc/ChangeLog: >>> >>> * config/aarch64/aarch64.cc (aarch64_override_options): Rework >> warnings. >>> >>> --- inline copy of patch -- >>> diff --git a/gcc/config/aarch64/aarch64.cc >>> b/gcc/config/aarch64/aarch64.cc index >>> >> caf80d66b3a744cc93899645aa5f9374983cd3db..3afd222ad3bdcfb922cc01 >> 0dcc0b >>> 138db29caf7f 100644 >>> --- a/gcc/config/aarch64/aarch64.cc >>> +++ b/gcc/config/aarch64/aarch64.cc >>> @@ -16388,12 +16388,22 @@ aarch64_override_options (void) >>> if (cpu && arch) >>> { >>> /* If both -mcpu and -march are specified, warn if they are not >>> - architecturally compatible and prefer the -march ISA flags. */ >>> - if (arch->arch != cpu->arch) >>> - { >>> - warning (0, "switch %<-mcpu=%s%> conflicts with %<-march=%s%> >> switch", >>> + feature compatible. feature compatible means that the inclusion of >> the >>> + cpu features would end up disabling an achitecture feature. In >>> + otherwords the cpu features need to be a strict superset of the arch >>> + features and if so prefer the -march ISA flags. */ >>> + auto full_arch_flags = arch->flags | arch_isa; >>> + auto full_cpu_flags = cpu->flags | cpu_isa; >>> + if (~full_cpu_flags & full_arch_flags) >>> + { >>> + std::string ext_diff >>> + = aarch64_get_extension_string_for_isa_flags (full_arch_flags, >>> + full_cpu_flags); >>> + warning (0, "switch %<-mcpu=%s%> conflicts with %<-march=%s%> >> switch " >>> + "and resulted in options %s being added", >>> aarch64_cpu_string, >>> - aarch64_arch_string); >>> + aarch64_arch_string, >>> + ext_diff.c_str ()); >>> } >>> >>> selected_arch = arch->arch; >>> >>> >>> >>>
> -----Original Message----- > From: Richard Earnshaw <Richard.Earnshaw@foss.arm.com> > Sent: Thursday, November 16, 2023 9:42 AM > To: Tamar Christina <Tamar.Christina@arm.com>; gcc-patches@gcc.gnu.org > Cc: nd <nd@arm.com>; Richard Earnshaw <Richard.Earnshaw@arm.com>; > Marcus Shawcroft <Marcus.Shawcroft@arm.com>; Kyrylo Tkachov > <Kyrylo.Tkachov@arm.com>; Richard Sandiford > <Richard.Sandiford@arm.com> > Subject: Re: [PATCH 6/6]AArch64: only emit mismatch error when features > would be disabled. > > > > On 16/11/2023 09:33, Tamar Christina wrote: > >> -----Original Message----- > >> From: Richard Earnshaw <Richard.Earnshaw@foss.arm.com> > >> Sent: Thursday, November 16, 2023 9:27 AM > >> To: Tamar Christina <Tamar.Christina@arm.com>; > >> gcc-patches@gcc.gnu.org > >> Cc: nd <nd@arm.com>; Richard Earnshaw <Richard.Earnshaw@arm.com>; > >> Marcus Shawcroft <Marcus.Shawcroft@arm.com>; Kyrylo Tkachov > >> <Kyrylo.Tkachov@arm.com>; Richard Sandiford > >> <Richard.Sandiford@arm.com> > >> Subject: Re: [PATCH 6/6]AArch64: only emit mismatch error when > >> features would be disabled. > >> > >> > >> > >> On 15/11/2023 17:08, Tamar Christina wrote: > >>> Hi All, > >>> > >>> At the moment we emit a warning whenever you specify both -march and > >>> -mcpu and the architecture of them differ. The idea originally was > >>> that the user may not be aware of this change. > >>> > >>> However this has a few problems: > >>> > >>> 1. Architecture revisions is not an observable part of the architecture, > >>> extensions are. Starting with GCC 14 we have therefore > >>> relaxed the rule > >> that > >>> all extensions can be enabled at any architecture level. Therefore it's > >>> incorrect, or at least not useful to keep the check on architecture. > >>> > >>> 2. It's problematic in Makefiles and other build systems, where you want > to > >>> for certain files enable CPU specific builds. i.e. you may be by default > >>> building for -march=armv8-a but for some file for -mcpu=neoverse-n1. > >> Since > >>> there's no easy way to remove the earlier options we end up warning > and > >>> there's no way to disable just this warning. Build systems compiling > with > >>> -Werror face an issue in this case that compiling with GCC is needlessly > >>> hard. > >>> > >>> 3. It doesn't actually warn for cases that may lead to issues, so e.g. > >>> -march=armv8.2-a+sve -mcpu=neoverse-n1 does not give a warning > >>> that > >> SVE would > >>> be disabled. > >>> > >>> For this reason I have one of two proposals: > >>> > >>> 1. Just remove this warning all together. > >>> > >>> 2. Rework the warning based on extensions and only warn when > >>> features > >> would be > >>> disabled by the presence of the -mcpu. This is the approach this patch > has > >>> taken. > >> > >> There's a third option here, which is what I plan to add for the Arm port: > >> > >> 3. Add -mcpu=unset and -march=unset support in the driver, which has > >> the effect of suppressing any earlier option that sets that flag. > >> > >> [BTW: patch 5 seems to be missing so I'm holding off on approving > >> this now.] > >> > > > > Ah sorry, I should have re-numbered this series. Patch 5 was sent > > earlier to unblock an internal team. It was > > https://gcc.gnu.org/pipermail/gcc-patches/2023-October/632802.html > > Ah, OK. > > So going back to your option 2. What should happen if the user specified - > mcpu=cortex-r82 and then specifies an extension that doesn't exist in the R > profile? > AArch64 in general does not validate extensions to architectures. So basically we would allow it. e.g. > aarch64-none-elf-gcc -O3 ./gcc/testsuite/gcc.dg/tree-ssa/slsr-20.c -S -o - -march=armv8.2-a+sve -mcpu=cortex-r82 cc1: warning: switch '-mcpu=cortex-r82' conflicts with '-march=armv8.2-a+sve' switch and would result in options +sve+norcpc+nodotprod+nofp16fml+noflagm+nopauth being added .arch armv8.2-a+crc+sve The new warning only tells you exactly what the compiler will be doing to your options, but doesn't change the behavior the compiler exhibits today since we always take -march over -mcpu. The difference is today we just say "there's a conflict" and don't specify what the conflict it. Regards, Tamar > R. > > > > > Thanks, > > Tamar > >> R. > >> > >>> > >>> As examples: > >>> > >>>> aarch64-none-linux-gnu-gcc -march=armv8.2-a+sve -mcpu=neoverse-n1 > >>> cc1: warning: switch ‘-mcpu=neoverse-n1’ conflicts with > >>> ‘-march=armv8.2- > >> a+sve’ switch and resulted in options +crc+sve+norcpc+nodotprod being > >> added > >> .arch armv8.2-a+crc+sve > >>> > >>>> aarch64-none-linux-gnu-gcc -march=armv8.2-a -mcpu=neoverse-n1 > >>>> aarch64-none-linux-gnu-gcc -march=armv8.2-a+dotprod - > mcpu=neoverse- > >> n1 > >>>> aarch64-none-linux-gnu-gcc -march=armv8.2-a+dotprod - > mcpu=neoverse- > >> n2 > >>> <no warning> > >>> > >>> The one remaining issue here is that if both -march and -mcpu are > >>> specified we pick the -march. This is not particularly obvious and > >>> for the use case to be more useful I think it makes sense to pick > >>> the CPU's > >> arch? > >>> > >>> I did not make that change in the patch as it changes semantics. > >>> > >>> Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. > >>> > >>> Note that I can't write a test for this because dg-warning expects > >>> warnings to be at a particular line and doesn't support warnings at > >>> the > >> "global" level. > >>> > >>> Ok for master? > >>> > >>> Thanks, > >>> Tamar > >>> > >>> gcc/ChangeLog: > >>> > >>> * config/aarch64/aarch64.cc (aarch64_override_options): Rework > >> warnings. > >>> > >>> --- inline copy of patch -- > >>> diff --git a/gcc/config/aarch64/aarch64.cc > >>> b/gcc/config/aarch64/aarch64.cc index > >>> > >> > caf80d66b3a744cc93899645aa5f9374983cd3db..3afd222ad3bdcfb922cc01 > >> 0dcc0b > >>> 138db29caf7f 100644 > >>> --- a/gcc/config/aarch64/aarch64.cc > >>> +++ b/gcc/config/aarch64/aarch64.cc > >>> @@ -16388,12 +16388,22 @@ aarch64_override_options (void) > >>> if (cpu && arch) > >>> { > >>> /* If both -mcpu and -march are specified, warn if they are not > >>> - architecturally compatible and prefer the -march ISA flags. */ > >>> - if (arch->arch != cpu->arch) > >>> - { > >>> - warning (0, "switch %<-mcpu=%s%> conflicts with %<-march=%s%> > >> switch", > >>> + feature compatible. feature compatible means that the inclusion > >>> +of > >> the > >>> + cpu features would end up disabling an achitecture feature. In > >>> + otherwords the cpu features need to be a strict superset of the arch > >>> + features and if so prefer the -march ISA flags. */ > >>> + auto full_arch_flags = arch->flags | arch_isa; > >>> + auto full_cpu_flags = cpu->flags | cpu_isa; > >>> + if (~full_cpu_flags & full_arch_flags) > >>> + { > >>> + std::string ext_diff > >>> + = aarch64_get_extension_string_for_isa_flags (full_arch_flags, > >>> + full_cpu_flags); > >>> + warning (0, "switch %<-mcpu=%s%> conflicts with %<-march=%s%> > >> switch " > >>> + "and resulted in options %s being added", > >>> aarch64_cpu_string, > >>> - aarch64_arch_string); > >>> + aarch64_arch_string, > >>> + ext_diff.c_str ()); > >>> } > >>> > >>> selected_arch = arch->arch; > >>> > >>> > >>> > >>>
On 15/11/2023 17:08, Tamar Christina wrote: > Hi All, > > At the moment we emit a warning whenever you specify both -march and -mcpu > and the architecture of them differ. The idea originally was that the user may > not be aware of this change. > > However this has a few problems: > > 1. Architecture revisions is not an observable part of the architecture, > extensions are. Starting with GCC 14 we have therefore relaxed the rule that > all extensions can be enabled at any architecture level. Therefore it's > incorrect, or at least not useful to keep the check on architecture. > > 2. It's problematic in Makefiles and other build systems, where you want to > for certain files enable CPU specific builds. i.e. you may be by default > building for -march=armv8-a but for some file for -mcpu=neoverse-n1. Since > there's no easy way to remove the earlier options we end up warning and > there's no way to disable just this warning. Build systems compiling with > -Werror face an issue in this case that compiling with GCC is needlessly > hard. > > 3. It doesn't actually warn for cases that may lead to issues, so e.g. > -march=armv8.2-a+sve -mcpu=neoverse-n1 does not give a warning that SVE would > be disabled. > > For this reason I have one of two proposals: > > 1. Just remove this warning all together. > > 2. Rework the warning based on extensions and only warn when features would be > disabled by the presence of the -mcpu. This is the approach this patch has > taken. > > As examples: > >> aarch64-none-linux-gnu-gcc -march=armv8.2-a+sve -mcpu=neoverse-n1 > cc1: warning: switch ‘-mcpu=neoverse-n1’ conflicts with ‘-march=armv8.2-a+sve’ switch and resulted in options +crc+sve+norcpc+nodotprod being added .arch armv8.2-a+crc+sve > >> aarch64-none-linux-gnu-gcc -march=armv8.2-a -mcpu=neoverse-n1 >> aarch64-none-linux-gnu-gcc -march=armv8.2-a+dotprod -mcpu=neoverse-n1 >> aarch64-none-linux-gnu-gcc -march=armv8.2-a+dotprod -mcpu=neoverse-n2 > <no warning> > > The one remaining issue here is that if both -march and -mcpu are specified we > pick the -march. This is not particularly obvious and for the use case to be > more useful I think it makes sense to pick the CPU's arch? The intent was always that users would either specify -march (with -mtune) or they would specify just -mcpu, not that they should mix both on the the command line. -mcpu=<cpu> is supposed to be the equivalent of -march=<arch-of(cpu)> -mtune=<cpu>. Both the Arm and AArch64 compilers implement the rule that -march dominates any -mcpu setting and this is (or at least used to be) documented in the manual. Part of the problem is that there's no clear way to recover positional information from the parameter list, so that it's not possible in the specs files to determine whether the user wrote -mcpu=x -march=y or -march=y -mcpu=x Now if a single source of rules for the cpu/arch conflates things in this way, that is pilot error, but we can't currently distinguish that case from the one where, say, the user adds -mcpu to CFLAGS in a makefile, but the make rules themselves need specific architecture features in order to build a specific file. This is where unset becomes useful as it will provide a clean(er) way to say ignore any conflict from an earlier option and use the following flags. Hence (-mcpu=x) (-mcpu=unset -march=y) (parenthesis indicate different sources of flags) will cause the compiler to forget any earlier -mcpu and just look at -march. Conversely, (-march=y) (-march=unset -mcpu=x) would clear any earlier -march flag and tell the compiler to just use -mcpu from now on. > > I did not make that change in the patch as it changes semantics. > > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. > > Note that I can't write a test for this because dg-warning expects warnings to > be at a particular line and doesn't support warnings at the "global" level. > > Ok for master? > > Thanks, > Tamar > > gcc/ChangeLog: > > * config/aarch64/aarch64.cc (aarch64_override_options): Rework warnings. > > --- inline copy of patch -- > diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc > index caf80d66b3a744cc93899645aa5f9374983cd3db..3afd222ad3bdcfb922cc010dcc0b138db29caf7f 100644 > --- a/gcc/config/aarch64/aarch64.cc > +++ b/gcc/config/aarch64/aarch64.cc > @@ -16388,12 +16388,22 @@ aarch64_override_options (void) > if (cpu && arch) > { > /* If both -mcpu and -march are specified, warn if they are not > - architecturally compatible and prefer the -march ISA flags. */ > - if (arch->arch != cpu->arch) > - { > - warning (0, "switch %<-mcpu=%s%> conflicts with %<-march=%s%> switch", > + feature compatible. feature compatible means that the inclusion of the > + cpu features would end up disabling an achitecture feature. In "CPU" and "architecture" > + otherwords the cpu features need to be a strict superset of the arch "other words" and "CPU". > + features and if so prefer the -march ISA flags. */ > + auto full_arch_flags = arch->flags | arch_isa; > + auto full_cpu_flags = cpu->flags | cpu_isa; > + if (~full_cpu_flags & full_arch_flags) > + { > + std::string ext_diff > + = aarch64_get_extension_string_for_isa_flags (full_arch_flags, > + full_cpu_flags); > + warning (0, "switch %<-mcpu=%s%> conflicts with %<-march=%s%> switch " > + "and resulted in options %s being added", Please check the convention here: should %s be surrounded in %<..%>? It is part of what the user effectively specified on the command line. > aarch64_cpu_string, > - aarch64_arch_string); > + aarch64_arch_string, > + ext_diff.c_str ()); > } > > selected_arch = arch->arch; > > Otherwise OK. R.
--- a/gcc/config/aarch64/aarch64.cc +++ b/gcc/config/aarch64/aarch64.cc @@ -16388,12 +16388,22 @@ aarch64_override_options (void) if (cpu && arch) { /* If both -mcpu and -march are specified, warn if they are not - architecturally compatible and prefer the -march ISA flags. */ - if (arch->arch != cpu->arch) - { - warning (0, "switch %<-mcpu=%s%> conflicts with %<-march=%s%> switch", + feature compatible. feature compatible means that the inclusion of the + cpu features would end up disabling an achitecture feature. In + otherwords the cpu features need to be a strict superset of the arch + features and if so prefer the -march ISA flags. */ + auto full_arch_flags = arch->flags | arch_isa; + auto full_cpu_flags = cpu->flags | cpu_isa; + if (~full_cpu_flags & full_arch_flags) + { + std::string ext_diff + = aarch64_get_extension_string_for_isa_flags (full_arch_flags, + full_cpu_flags); + warning (0, "switch %<-mcpu=%s%> conflicts with %<-march=%s%> switch " + "and resulted in options %s being added", aarch64_cpu_string, - aarch64_arch_string); + aarch64_arch_string, + ext_diff.c_str ()); } selected_arch = arch->arch;
Hi All, At the moment we emit a warning whenever you specify both -march and -mcpu and the architecture of them differ. The idea originally was that the user may not be aware of this change. However this has a few problems: 1. Architecture revisions is not an observable part of the architecture, extensions are. Starting with GCC 14 we have therefore relaxed the rule that all extensions can be enabled at any architecture level. Therefore it's incorrect, or at least not useful to keep the check on architecture. 2. It's problematic in Makefiles and other build systems, where you want to for certain files enable CPU specific builds. i.e. you may be by default building for -march=armv8-a but for some file for -mcpu=neoverse-n1. Since there's no easy way to remove the earlier options we end up warning and there's no way to disable just this warning. Build systems compiling with -Werror face an issue in this case that compiling with GCC is needlessly hard. 3. It doesn't actually warn for cases that may lead to issues, so e.g. -march=armv8.2-a+sve -mcpu=neoverse-n1 does not give a warning that SVE would be disabled. For this reason I have one of two proposals: 1. Just remove this warning all together. 2. Rework the warning based on extensions and only warn when features would be disabled by the presence of the -mcpu. This is the approach this patch has taken. As examples: > aarch64-none-linux-gnu-gcc -march=armv8.2-a+sve -mcpu=neoverse-n1 cc1: warning: switch ‘-mcpu=neoverse-n1’ conflicts with ‘-march=armv8.2-a+sve’ switch and resulted in options +crc+sve+norcpc+nodotprod being added .arch armv8.2-a+crc+sve > aarch64-none-linux-gnu-gcc -march=armv8.2-a -mcpu=neoverse-n1 > aarch64-none-linux-gnu-gcc -march=armv8.2-a+dotprod -mcpu=neoverse-n1 > aarch64-none-linux-gnu-gcc -march=armv8.2-a+dotprod -mcpu=neoverse-n2 <no warning> The one remaining issue here is that if both -march and -mcpu are specified we pick the -march. This is not particularly obvious and for the use case to be more useful I think it makes sense to pick the CPU's arch? I did not make that change in the patch as it changes semantics. Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. Note that I can't write a test for this because dg-warning expects warnings to be at a particular line and doesn't support warnings at the "global" level. Ok for master? Thanks, Tamar gcc/ChangeLog: * config/aarch64/aarch64.cc (aarch64_override_options): Rework warnings. --- inline copy of patch -- diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc index caf80d66b3a744cc93899645aa5f9374983cd3db..3afd222ad3bdcfb922cc010dcc0b138db29caf7f 100644 -- diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc index caf80d66b3a744cc93899645aa5f9374983cd3db..3afd222ad3bdcfb922cc010dcc0b138db29caf7f 100644 --- a/gcc/config/aarch64/aarch64.cc +++ b/gcc/config/aarch64/aarch64.cc @@ -16388,12 +16388,22 @@ aarch64_override_options (void) if (cpu && arch) { /* If both -mcpu and -march are specified, warn if they are not - architecturally compatible and prefer the -march ISA flags. */ - if (arch->arch != cpu->arch) - { - warning (0, "switch %<-mcpu=%s%> conflicts with %<-march=%s%> switch", + feature compatible. feature compatible means that the inclusion of the + cpu features would end up disabling an achitecture feature. In + otherwords the cpu features need to be a strict superset of the arch + features and if so prefer the -march ISA flags. */ + auto full_arch_flags = arch->flags | arch_isa; + auto full_cpu_flags = cpu->flags | cpu_isa; + if (~full_cpu_flags & full_arch_flags) + { + std::string ext_diff + = aarch64_get_extension_string_for_isa_flags (full_arch_flags, + full_cpu_flags); + warning (0, "switch %<-mcpu=%s%> conflicts with %<-march=%s%> switch " + "and resulted in options %s being added", aarch64_cpu_string, - aarch64_arch_string); + aarch64_arch_string, + ext_diff.c_str ()); } selected_arch = arch->arch;