* [AArch64] PR target/105157 Increase number of cores TARGET_CPU_DEFAULT can encode
@ 2022-04-07 17:23 Andre Vieira (lists)
2022-04-08 7:04 ` Richard Sandiford
0 siblings, 1 reply; 4+ messages in thread
From: Andre Vieira (lists) @ 2022-04-07 17:23 UTC (permalink / raw)
To: gcc-patches; +Cc: Richard Sandiford, Kyrylo Tkachov
[-- Attachment #1: Type: text/plain, Size: 1072 bytes --]
Hi,
This addresses the compile-time increase seen in the PR target/105157.
This was being caused by selecting the wrong core tuning, as when we
added the latest AArch64 the TARGET_CPU_generic tuning was pushed beyond
the 0x3f mask we used to encode both target cpu and attributes into
TARGET_CPU_DEFAULT.
This is a quick fix to increase the number of allowed cores for now late
in stage 4, but for next GCC we plan to fix this in a more suitable and
maintainable manner.
gcc/ChangeLog:
PR target/105157
* config.gcc: Shift ext_mask by TARGET_CPU_NBITS.
* config/aarch64/aarch64.h (TARGET_CPU_NBITS): New macro.
(TARGET_CPU_MASK): Likewise.
(TARGET_CPU_DEFAULT): Use TARGET_CPU_NBITS.
* config/aarch64/aarch64.cc (aarch64_get_tune_cpu): Use
TARGET_CPU_MASK.
(aarch64_get_arch): Likewise.
(aarch64_override_options): Use TARGET_CPU_NBITS.
(aarch64_run_selftests): Add test to guarantee TARGET_CPU_NBITS
is big enough.
[-- Attachment #2: PR105157.patch --]
[-- Type: text/plain, Size: 3421 bytes --]
diff --git a/gcc/config.gcc b/gcc/config.gcc
index 2cc5aeec9e46aacc46e4e509d01f57f9f6585169..8946984f5e93cd6fe5364b44e74c64e714ba1d6a 100644
--- a/gcc/config.gcc
+++ b/gcc/config.gcc
@@ -4239,7 +4239,7 @@ case "${target}" in
ext_val=`echo $ext_val | sed -e 's/[a-z0-9]\+//'`
done
- ext_mask="(("$ext_mask") << 6)"
+ ext_mask="(("$ext_mask") << TARGET_CPU_NBITS)"
if [ x"$base_id" != x ]; then
target_cpu_cname="TARGET_CPU_$base_id | $ext_mask"
fi
diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
index efa46ac0b8799b5849b609d591186e26e5cb37ff..97b2d38dac9f13717e90f4f6996ab94814922950 100644
--- a/gcc/config/aarch64/aarch64.h
+++ b/gcc/config/aarch64/aarch64.h
@@ -813,10 +813,17 @@ enum target_cpus
TARGET_CPU_generic
};
+
+/* Define how many bits are used to represent the CPU in TARGET_CPU_DEFAULT.
+ 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_FLAGS. */
+#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 << 6))
+ (TARGET_CPU_generic | (AARCH64_CPU_DEFAULT_FLAGS << TARGET_CPU_NBITS))
#endif
/* 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 d66626748e5edc46e46edf10e243114a9f74be97..9d83e007d7a31455c167bf21124cf9fbc0d31788 100644
--- a/gcc/config/aarch64/aarch64.cc
+++ b/gcc/config/aarch64/aarch64.cc
@@ -18062,12 +18062,12 @@ aarch64_get_tune_cpu (enum aarch64_processor cpu)
if (cpu != aarch64_none)
return &all_cores[cpu];
- /* The & 0x3f is to extract the bottom 6 bits that encode the
- default cpu as selected by the --with-cpu GCC configure option
+ /* The & TARGET_CPU_MASK is to extract the bottom TARGET_CPU_NBITS bits 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 & 0x3f];
+ return &all_cores[TARGET_CPU_DEFAULT & TARGET_CPU_MASK];
}
/* Return the architecture corresponding to the enum ARCH.
@@ -18079,7 +18079,8 @@ aarch64_get_arch (enum aarch64_arch arch)
if (arch != aarch64_no_arch)
return &all_architectures[arch];
- const struct processor *cpu = &all_cores[TARGET_CPU_DEFAULT & 0x3f];
+ const struct processor *cpu
+ = &all_cores[TARGET_CPU_DEFAULT & TARGET_CPU_MASK];
return &all_architectures[cpu->arch];
}
@@ -18166,7 +18167,7 @@ aarch64_override_options (void)
{
/* Get default configure-time CPU. */
selected_cpu = aarch64_get_tune_cpu (aarch64_none);
- aarch64_isa_flags = TARGET_CPU_DEFAULT >> 6;
+ aarch64_isa_flags = TARGET_CPU_DEFAULT >> TARGET_CPU_NBITS;
}
if (selected_tune)
@@ -27260,6 +27261,9 @@ aarch64_run_selftests (void)
{
aarch64_test_loading_full_dump ();
aarch64_test_fractional_cost ();
+ /* Check whether the TARGET_CPU_MASK is big enough to fit all of the definded
+ CPUs. */
+ ASSERT_TRUE (TARGET_CPU_generic < TARGET_CPU_MASK);
}
} // namespace selftest
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [AArch64] PR target/105157 Increase number of cores TARGET_CPU_DEFAULT can encode
2022-04-07 17:23 [AArch64] PR target/105157 Increase number of cores TARGET_CPU_DEFAULT can encode Andre Vieira (lists)
@ 2022-04-08 7:04 ` Richard Sandiford
2022-04-08 9:07 ` Andre Vieira (lists)
0 siblings, 1 reply; 4+ messages in thread
From: Richard Sandiford @ 2022-04-08 7:04 UTC (permalink / raw)
To: Andre Vieira (lists); +Cc: gcc-patches, Kyrylo Tkachov
"Andre Vieira (lists)" <andre.simoesdiasvieira@arm.com> writes:
> Hi,
>
> This addresses the compile-time increase seen in the PR target/105157.
> This was being caused by selecting the wrong core tuning, as when we
> added the latest AArch64 the TARGET_CPU_generic tuning was pushed beyond
> the 0x3f mask we used to encode both target cpu and attributes into
> TARGET_CPU_DEFAULT.
>
> This is a quick fix to increase the number of allowed cores for now late
> in stage 4, but for next GCC we plan to fix this in a more suitable and
> maintainable manner.
>
> gcc/ChangeLog:
>
> PR target/105157
> * config.gcc: Shift ext_mask by TARGET_CPU_NBITS.
> * config/aarch64/aarch64.h (TARGET_CPU_NBITS): New macro.
> (TARGET_CPU_MASK): Likewise.
> (TARGET_CPU_DEFAULT): Use TARGET_CPU_NBITS.
> * config/aarch64/aarch64.cc (aarch64_get_tune_cpu): Use
> TARGET_CPU_MASK.
> (aarch64_get_arch): Likewise.
> (aarch64_override_options): Use TARGET_CPU_NBITS.
> (aarch64_run_selftests): Add test to guarantee TARGET_CPU_NBITS
> is big enough.
Looks good, just a minor nit:
>
> diff --git a/gcc/config.gcc b/gcc/config.gcc
> index 2cc5aeec9e46aacc46e4e509d01f57f9f6585169..8946984f5e93cd6fe5364b44e74c64e714ba1d6a 100644
> --- a/gcc/config.gcc
> +++ b/gcc/config.gcc
> @@ -4239,7 +4239,7 @@ case "${target}" in
> ext_val=`echo $ext_val | sed -e 's/[a-z0-9]\+//'`
> done
>
> - ext_mask="(("$ext_mask") << 6)"
> + ext_mask="(("$ext_mask") << TARGET_CPU_NBITS)"
> if [ x"$base_id" != x ]; then
> target_cpu_cname="TARGET_CPU_$base_id | $ext_mask"
> fi
> diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
> index efa46ac0b8799b5849b609d591186e26e5cb37ff..97b2d38dac9f13717e90f4f6996ab94814922950 100644
> --- a/gcc/config/aarch64/aarch64.h
> +++ b/gcc/config/aarch64/aarch64.h
> @@ -813,10 +813,17 @@ enum target_cpus
> TARGET_CPU_generic
> };
>
> +
Nit: excess extra line.
> +/* Define how many bits are used to represent the CPU in TARGET_CPU_DEFAULT.
> + 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_FLAGS. */
> +#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 << 6))
> + (TARGET_CPU_generic | (AARCH64_CPU_DEFAULT_FLAGS << TARGET_CPU_NBITS))
> #endif
>
> /* 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 d66626748e5edc46e46edf10e243114a9f74be97..9d83e007d7a31455c167bf21124cf9fbc0d31788 100644
> --- a/gcc/config/aarch64/aarch64.cc
> +++ b/gcc/config/aarch64/aarch64.cc
> @@ -18062,12 +18062,12 @@ aarch64_get_tune_cpu (enum aarch64_processor cpu)
> if (cpu != aarch64_none)
> return &all_cores[cpu];
>
> - /* The & 0x3f is to extract the bottom 6 bits that encode the
> - default cpu as selected by the --with-cpu GCC configure option
> + /* The & TARGET_CPU_MASK is to extract the bottom TARGET_CPU_NBITS bits 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 & 0x3f];
> + return &all_cores[TARGET_CPU_DEFAULT & TARGET_CPU_MASK];
> }
>
> /* Return the architecture corresponding to the enum ARCH.
> @@ -18079,7 +18079,8 @@ aarch64_get_arch (enum aarch64_arch arch)
> if (arch != aarch64_no_arch)
> return &all_architectures[arch];
>
> - const struct processor *cpu = &all_cores[TARGET_CPU_DEFAULT & 0x3f];
> + const struct processor *cpu
> + = &all_cores[TARGET_CPU_DEFAULT & TARGET_CPU_MASK];
>
> return &all_architectures[cpu->arch];
> }
> @@ -18166,7 +18167,7 @@ aarch64_override_options (void)
> {
> /* Get default configure-time CPU. */
> selected_cpu = aarch64_get_tune_cpu (aarch64_none);
> - aarch64_isa_flags = TARGET_CPU_DEFAULT >> 6;
> + aarch64_isa_flags = TARGET_CPU_DEFAULT >> TARGET_CPU_NBITS;
> }
>
> if (selected_tune)
> @@ -27260,6 +27261,9 @@ aarch64_run_selftests (void)
> {
> aarch64_test_loading_full_dump ();
> aarch64_test_fractional_cost ();
> + /* Check whether the TARGET_CPU_MASK is big enough to fit all of the definded
> + CPUs. */
> + ASSERT_TRUE (TARGET_CPU_generic < TARGET_CPU_MASK);
I think this would be better as a static assert at the top level:
static_assert (TARGET_CPU_generic < TARGET_CPU_MASK,
"TARGET_CPU_NBITS is big enough");
OK with that change, thanks.
Richard
> }
>
> } // namespace selftest
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [AArch64] PR target/105157 Increase number of cores TARGET_CPU_DEFAULT can encode
2022-04-08 7:04 ` Richard Sandiford
@ 2022-04-08 9:07 ` Andre Vieira (lists)
2022-04-08 10:18 ` Richard Sandiford
0 siblings, 1 reply; 4+ messages in thread
From: Andre Vieira (lists) @ 2022-04-08 9:07 UTC (permalink / raw)
To: gcc-patches, Kyrylo Tkachov, richard.sandiford
On 08/04/2022 08:04, Richard Sandiford wrote:
> I think this would be better as a static assert at the top level:
>
> static_assert (TARGET_CPU_generic < TARGET_CPU_MASK,
> "TARGET_CPU_NBITS is big enough");
The motivation being that you want this to be checked regardless of
whether we are using CHECKING_P?
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [AArch64] PR target/105157 Increase number of cores TARGET_CPU_DEFAULT can encode
2022-04-08 9:07 ` Andre Vieira (lists)
@ 2022-04-08 10:18 ` Richard Sandiford
0 siblings, 0 replies; 4+ messages in thread
From: Richard Sandiford @ 2022-04-08 10:18 UTC (permalink / raw)
To: Andre Vieira (lists); +Cc: gcc-patches, Kyrylo Tkachov
"Andre Vieira (lists)" <andre.simoesdiasvieira@arm.com> writes:
> On 08/04/2022 08:04, Richard Sandiford wrote:
>> I think this would be better as a static assert at the top level:
>>
>> static_assert (TARGET_CPU_generic < TARGET_CPU_MASK,
>> "TARGET_CPU_NBITS is big enough");
> The motivation being that you want this to be checked regardless of
> whether we are using CHECKING_P?
Yeah, that's one reason. Others are that static_asserts are caught at
the earliest possible moment (as a build failure) and that they don't
require any object code.
Thanks,
Richard
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-04-08 10:19 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-07 17:23 [AArch64] PR target/105157 Increase number of cores TARGET_CPU_DEFAULT can encode Andre Vieira (lists)
2022-04-08 7:04 ` Richard Sandiford
2022-04-08 9:07 ` Andre Vieira (lists)
2022-04-08 10:18 ` Richard Sandiford
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).