| Message ID | 20200930073145.GG15011@bubble.grove.modra.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RS6000] -mno-minimal-toc vs. power10 pcrelative | expand |
Hi! On Wed, Sep 30, 2020 at 05:01:45PM +0930, Alan Modra wrote: > * config/rs6000/linux64.h (SUBSUBTARGET_OVERRIDE_OPTIONS): Don't > set -mcmodel=small for -mno-minimal-toc when pcrel. > - SET_CMODEL (CMODEL_SMALL); \ > + if (TARGET_MINIMAL_TOC \ > + || !(TARGET_PCREL \ > + || (PCREL_SUPPORTED_BY_OS \ > + && (rs6000_isa_flags_explicit \ > + & OPTION_MASK_PCREL) == 0))) \ > + SET_CMODEL (CMODEL_SMALL); \ Please write this in a more readable way? With some "else" statements, perhaps. It is also fine to SET_CMODEL twice if that makes for simpler code. The rest looks fine, fwiw. Segher
On Wed, Sep 30, 2020 at 03:56:32PM -0500, Segher Boessenkool wrote: > On Wed, Sep 30, 2020 at 05:01:45PM +0930, Alan Modra wrote: > > * config/rs6000/linux64.h (SUBSUBTARGET_OVERRIDE_OPTIONS): Don't > > set -mcmodel=small for -mno-minimal-toc when pcrel. > > > - SET_CMODEL (CMODEL_SMALL); \ > > + if (TARGET_MINIMAL_TOC \ > > + || !(TARGET_PCREL \ > > + || (PCREL_SUPPORTED_BY_OS \ > > + && (rs6000_isa_flags_explicit \ > > + & OPTION_MASK_PCREL) == 0))) \ > > + SET_CMODEL (CMODEL_SMALL); \ > > Please write this in a more readable way? With some "else" statements, > perhaps. > > It is also fine to SET_CMODEL twice if that makes for simpler code. Committed as per your suggestion. I was looking at it again today with the aim of converting this ugly macro to a function, and spotted the duplication in freebsd64.h. Which has some bit-rot. Do you like the following? rs6000_linux64_override_options is functionally the same as what was in linux64.h. I built various configurations to test the change, powerpc64-linux, powerpc64le-linux without any 32-bit targets enabled, powerpc64-freebsd12.0. * config/rs6000/freebsd64.h (SUBSUBTARGET_OVERRIDE_OPTIONS): Use rs6000_linux64_override_options. * config/rs6000/linux64.h (SUBSUBTARGET_OVERRIDE_OPTIONS): Break out to.. * config/rs6000/rs6000.c (rs6000_linux64_override_options): ..this, new function. Tweak non-biarch test and clearing of profile_kernel to work with freebsd64.h. diff --git a/gcc/config/rs6000/freebsd64.h b/gcc/config/rs6000/freebsd64.h index c9913638ffb..6984ca5a107 100644 --- a/gcc/config/rs6000/freebsd64.h +++ b/gcc/config/rs6000/freebsd64.h @@ -78,65 +78,7 @@ extern int dot_symbols; #undef SUBSUBTARGET_OVERRIDE_OPTIONS #define SUBSUBTARGET_OVERRIDE_OPTIONS \ - do \ - { \ - if (!global_options_set.x_rs6000_alignment_flags) \ - rs6000_alignment_flags = MASK_ALIGN_NATURAL; \ - if (TARGET_64BIT) \ - { \ - if (DEFAULT_ABI != ABI_AIX) \ - { \ - rs6000_current_abi = ABI_AIX; \ - error (INVALID_64BIT, "call"); \ - } \ - dot_symbols = !strcmp (rs6000_abi_name, "aixdesc"); \ - if (rs6000_isa_flags & OPTION_MASK_RELOCATABLE) \ - { \ - rs6000_isa_flags &= ~OPTION_MASK_RELOCATABLE; \ - error (INVALID_64BIT, "relocatable"); \ - } \ - if (ELFv2_ABI_CHECK) \ - { \ - rs6000_current_abi = ABI_ELFv2; \ - if (dot_symbols) \ - error ("%<-mcall-aixdesc%> incompatible with %<-mabi=elfv2%>"); \ - } \ - if (rs6000_isa_flags & OPTION_MASK_EABI) \ - { \ - rs6000_isa_flags &= ~OPTION_MASK_EABI; \ - error (INVALID_64BIT, "eabi"); \ - } \ - if (TARGET_PROTOTYPE) \ - { \ - target_prototype = 0; \ - error (INVALID_64BIT, "prototype"); \ - } \ - if ((rs6000_isa_flags & OPTION_MASK_POWERPC64) == 0) \ - { \ - rs6000_isa_flags |= OPTION_MASK_POWERPC64; \ - error ("%<-m64%> requires a PowerPC64 cpu"); \ - } \ - if ((rs6000_isa_flags_explicit \ - & OPTION_MASK_MINIMAL_TOC) != 0) \ - { \ - if (global_options_set.x_rs6000_current_cmodel \ - && rs6000_current_cmodel != CMODEL_SMALL) \ - error ("%<-mcmodel%> incompatible with other toc options"); \ - SET_CMODEL (CMODEL_SMALL); \ - } \ - else \ - { \ - if (!global_options_set.x_rs6000_current_cmodel) \ - SET_CMODEL (CMODEL_MEDIUM); \ - if (rs6000_current_cmodel != CMODEL_SMALL) \ - { \ - TARGET_NO_FP_IN_TOC = 0; \ - TARGET_NO_SUM_IN_TOC = 0; \ - } \ - } \ - } \ - } \ - while (0) + do rs6000_linux64_override_options (); while (0) #undef ASM_SPEC #undef LINK_OS_FREEBSD_SPEC diff --git a/gcc/config/rs6000/linux64.h b/gcc/config/rs6000/linux64.h index 5c9f8e3d7af..73b6c01874c 100644 --- a/gcc/config/rs6000/linux64.h +++ b/gcc/config/rs6000/linux64.h @@ -96,99 +96,7 @@ extern int dot_symbols; #undef SUBSUBTARGET_OVERRIDE_OPTIONS #define SUBSUBTARGET_OVERRIDE_OPTIONS \ - do \ - { \ - if (!global_options_set.x_rs6000_alignment_flags) \ - rs6000_alignment_flags = MASK_ALIGN_NATURAL; \ - if (rs6000_isa_flags & OPTION_MASK_64BIT) \ - { \ - if (DEFAULT_ABI != ABI_AIX) \ - { \ - rs6000_current_abi = ABI_AIX; \ - error (INVALID_64BIT, "call"); \ - } \ - dot_symbols = !strcmp (rs6000_abi_name, "aixdesc"); \ - if (ELFv2_ABI_CHECK) \ - { \ - rs6000_current_abi = ABI_ELFv2; \ - if (dot_symbols) \ - error ("%<-mcall-aixdesc%> incompatible with %<-mabi=elfv2%>"); \ - } \ - if (rs6000_isa_flags & OPTION_MASK_RELOCATABLE) \ - { \ - rs6000_isa_flags &= ~OPTION_MASK_RELOCATABLE; \ - error (INVALID_64BIT, "relocatable"); \ - } \ - if (rs6000_isa_flags & OPTION_MASK_EABI) \ - { \ - rs6000_isa_flags &= ~OPTION_MASK_EABI; \ - error (INVALID_64BIT, "eabi"); \ - } \ - if (TARGET_PROTOTYPE) \ - { \ - target_prototype = 0; \ - error (INVALID_64BIT, "prototype"); \ - } \ - if ((rs6000_isa_flags & OPTION_MASK_POWERPC64) == 0) \ - { \ - rs6000_isa_flags |= OPTION_MASK_POWERPC64; \ - error ("%<-m64%> requires a PowerPC64 cpu"); \ - } \ - if (!global_options_set.x_rs6000_current_cmodel) \ - SET_CMODEL (CMODEL_MEDIUM); \ - if ((rs6000_isa_flags_explicit \ - & OPTION_MASK_MINIMAL_TOC) != 0) \ - { \ - if (global_options_set.x_rs6000_current_cmodel \ - && rs6000_current_cmodel != CMODEL_SMALL) \ - error ("%<-mcmodel incompatible with other toc options%>"); \ - if (TARGET_MINIMAL_TOC) \ - SET_CMODEL (CMODEL_SMALL); \ - else if (TARGET_PCREL \ - || (PCREL_SUPPORTED_BY_OS \ - && (rs6000_isa_flags_explicit \ - & OPTION_MASK_PCREL) == 0)) \ - /* Ignore -mno-minimal-toc. */ \ - ; \ - else \ - SET_CMODEL (CMODEL_SMALL); \ - } \ - else \ - { \ - if (rs6000_current_cmodel != CMODEL_SMALL) \ - { \ - if (!global_options_set.x_TARGET_NO_FP_IN_TOC) \ - TARGET_NO_FP_IN_TOC \ - = rs6000_current_cmodel == CMODEL_MEDIUM; \ - if (!global_options_set.x_TARGET_NO_SUM_IN_TOC) \ - TARGET_NO_SUM_IN_TOC = 0; \ - } \ - } \ - if (TARGET_PLTSEQ && DEFAULT_ABI != ABI_ELFv2) \ - { \ - if (global_options_set.x_rs6000_pltseq) \ - warning (0, "%qs unsupported for this ABI", \ - "-mpltseq"); \ - rs6000_pltseq = false; \ - } \ - } \ - else \ - { \ - if (!RS6000_BI_ARCH_P) \ - error (INVALID_32BIT, "32"); \ - if (TARGET_PROFILE_KERNEL) \ - { \ - TARGET_PROFILE_KERNEL = 0; \ - error (INVALID_32BIT, "profile-kernel"); \ - } \ - if (global_options_set.x_rs6000_current_cmodel) \ - { \ - SET_CMODEL (CMODEL_SMALL); \ - error (INVALID_32BIT, "cmodel"); \ - } \ - } \ - } \ - while (0) + do rs6000_linux64_override_options (); while (0) #undef ASM_SPEC #undef LINK_OS_LINUX_SPEC diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c index d0924d59a65..48f3cdec440 100644 --- a/gcc/config/rs6000/rs6000.c +++ b/gcc/config/rs6000/rs6000.c @@ -3451,6 +3451,102 @@ rs6000_override_options_after_change (void) flag_cunroll_grow_size = flag_peel_loops || optimize >= 3; } +#ifdef TARGET_USES_LINUX64_OPT +static void +rs6000_linux64_override_options () +{ + if (!global_options_set.x_rs6000_alignment_flags) + rs6000_alignment_flags = MASK_ALIGN_NATURAL; + if (rs6000_isa_flags & OPTION_MASK_64BIT) + { + if (DEFAULT_ABI != ABI_AIX) + { + rs6000_current_abi = ABI_AIX; + error (INVALID_64BIT, "call"); + } + dot_symbols = !strcmp (rs6000_abi_name, "aixdesc"); + if (ELFv2_ABI_CHECK) + { + rs6000_current_abi = ABI_ELFv2; + if (dot_symbols) + error ("%<-mcall-aixdesc%> incompatible with %<-mabi=elfv2%>"); + } + if (rs6000_isa_flags & OPTION_MASK_RELOCATABLE) + { + rs6000_isa_flags &= ~OPTION_MASK_RELOCATABLE; + error (INVALID_64BIT, "relocatable"); + } + if (rs6000_isa_flags & OPTION_MASK_EABI) + { + rs6000_isa_flags &= ~OPTION_MASK_EABI; + error (INVALID_64BIT, "eabi"); + } + if (TARGET_PROTOTYPE) + { + target_prototype = 0; + error (INVALID_64BIT, "prototype"); + } + if ((rs6000_isa_flags & OPTION_MASK_POWERPC64) == 0) + { + rs6000_isa_flags |= OPTION_MASK_POWERPC64; + error ("%<-m64%> requires a PowerPC64 cpu"); + } + if (!global_options_set.x_rs6000_current_cmodel) + SET_CMODEL (CMODEL_MEDIUM); + if ((rs6000_isa_flags_explicit + & OPTION_MASK_MINIMAL_TOC) != 0) + { + if (global_options_set.x_rs6000_current_cmodel + && rs6000_current_cmodel != CMODEL_SMALL) + error ("%<-mcmodel incompatible with other toc options%>"); + if (TARGET_MINIMAL_TOC) + SET_CMODEL (CMODEL_SMALL); + else if (TARGET_PCREL + || (PCREL_SUPPORTED_BY_OS + && (rs6000_isa_flags_explicit + & OPTION_MASK_PCREL) == 0)) + /* Ignore -mno-minimal-toc. */ + ; + else + SET_CMODEL (CMODEL_SMALL); + } + else + { + if (rs6000_current_cmodel != CMODEL_SMALL) + { + if (!global_options_set.x_TARGET_NO_FP_IN_TOC) + TARGET_NO_FP_IN_TOC + = rs6000_current_cmodel == CMODEL_MEDIUM; + if (!global_options_set.x_TARGET_NO_SUM_IN_TOC) + TARGET_NO_SUM_IN_TOC = 0; + } + } + if (TARGET_PLTSEQ && DEFAULT_ABI != ABI_ELFv2) + { + if (global_options_set.x_rs6000_pltseq) + warning (0, "%qs unsupported for this ABI", + "-mpltseq"); + rs6000_pltseq = false; + } + } + else if (TARGET_64BIT) + error (INVALID_32BIT, "32"); + else + { + if (TARGET_PROFILE_KERNEL) + { + profile_kernel = 0; + error (INVALID_32BIT, "profile-kernel"); + } + if (global_options_set.x_rs6000_current_cmodel) + { + SET_CMODEL (CMODEL_SMALL); + error (INVALID_32BIT, "cmodel"); + } + } +} +#endif + /* Override command line options. Combine build-specific configuration information with options
Hi! On Thu, Oct 01, 2020 at 10:57:48PM +0930, Alan Modra wrote: > On Wed, Sep 30, 2020 at 03:56:32PM -0500, Segher Boessenkool wrote: > > On Wed, Sep 30, 2020 at 05:01:45PM +0930, Alan Modra wrote: > > > * config/rs6000/linux64.h (SUBSUBTARGET_OVERRIDE_OPTIONS): Don't > > > set -mcmodel=small for -mno-minimal-toc when pcrel. > > > > > - SET_CMODEL (CMODEL_SMALL); \ > > > + if (TARGET_MINIMAL_TOC \ > > > + || !(TARGET_PCREL \ > > > + || (PCREL_SUPPORTED_BY_OS \ > > > + && (rs6000_isa_flags_explicit \ > > > + & OPTION_MASK_PCREL) == 0))) \ > > > + SET_CMODEL (CMODEL_SMALL); \ > > > > Please write this in a more readable way? With some "else" statements, > > perhaps. > > > > It is also fine to SET_CMODEL twice if that makes for simpler code. > > Committed as per your suggestion. Thanks. > I was looking at it again today > with the aim of converting this ugly macro to a function, and spotted > the duplication in freebsd64.h. Which has some bit-rot. > > Do you like the following? rs6000_linux64_override_options is > functionally the same as what was in linux64.h. I built various > configurations to test the change, powerpc64-linux, powerpc64le-linux > without any 32-bit targets enabled, powerpc64-freebsd12.0. Please do this as two patches? One the refactoring without any functional changes (which is pre-approved -- the name "linux64" isn't great if you use it in other OSes as well, but I cannot think of a better name either), and the other the actual change (which probably is fine as well, but it is hard to see with the patch like this). Thanks, Segher
Hi Segher, On Thu, Oct 01, 2020 at 01:22:07PM -0500, Segher Boessenkool wrote: > Hi! > > On Thu, Oct 01, 2020 at 10:57:48PM +0930, Alan Modra wrote: > > On Wed, Sep 30, 2020 at 03:56:32PM -0500, Segher Boessenkool wrote: > > > On Wed, Sep 30, 2020 at 05:01:45PM +0930, Alan Modra wrote: > > > > * config/rs6000/linux64.h (SUBSUBTARGET_OVERRIDE_OPTIONS): Don't > > > > set -mcmodel=small for -mno-minimal-toc when pcrel. > > > > > > > - SET_CMODEL (CMODEL_SMALL); \ > > > > + if (TARGET_MINIMAL_TOC \ > > > > + || !(TARGET_PCREL \ > > > > + || (PCREL_SUPPORTED_BY_OS \ > > > > + && (rs6000_isa_flags_explicit \ > > > > + & OPTION_MASK_PCREL) == 0))) \ > > > > + SET_CMODEL (CMODEL_SMALL); \ > > > > > > Please write this in a more readable way? With some "else" statements, > > > perhaps. > > > > > > It is also fine to SET_CMODEL twice if that makes for simpler code. > > > > Committed as per your suggestion. > > Thanks. > > > I was looking at it again today > > with the aim of converting this ugly macro to a function, and spotted > > the duplication in freebsd64.h. Which has some bit-rot. > > > > Do you like the following? rs6000_linux64_override_options is > > functionally the same as what was in linux64.h. I built various > > configurations to test the change, powerpc64-linux, powerpc64le-linux > > without any 32-bit targets enabled, powerpc64-freebsd12.0. > > Please do this as two patches? One the refactoring without any > functional changes (which is pre-approved -- the name "linux64" isn't > great if you use it in other OSes as well, but I cannot think of a > better name either), The patch as posted has no functional changes. I even avoided formatting changes as much as possible. The only changes were those necessary to use the code from linux64.h in a powerpc64-freebsd compiler, where #define TARGET_PROFILE_KERNEL 0 .. TARGET_PROFILE_KERNEL = 0; doesn't work, nor does if (!RS6000_BI_ARCH_P) error (INVALID_32BIT, "32"); when RS6000_BI_ARCH_P is undefined. > and the other the actual change (which probably is > fine as well, but it is hard to see with the patch like this). I do have a followup patch.. Commit c6be439b37 wrongly left a block of code inside and "else" block, which changed the default for power10 TARGET_NO_FP_IN_TOC accidentally. We don't want FP constants in the TOC when -mcmodel=medium can address them just as efficiently outside the TOC. * config/rs6000/rs6000.c (rs6000_linux64_override_options): Formatting. Correct setting of TARGET_NO_FP_IN_TOC and TARGET_NO_SUM_IN_TOC. diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c index 48f3cdec440..a1651551ff2 100644 --- a/gcc/config/rs6000/rs6000.c +++ b/gcc/config/rs6000/rs6000.c @@ -3493,8 +3493,7 @@ rs6000_linux64_override_options () } if (!global_options_set.x_rs6000_current_cmodel) SET_CMODEL (CMODEL_MEDIUM); - if ((rs6000_isa_flags_explicit - & OPTION_MASK_MINIMAL_TOC) != 0) + if ((rs6000_isa_flags_explicit & OPTION_MASK_MINIMAL_TOC) != 0) { if (global_options_set.x_rs6000_current_cmodel && rs6000_current_cmodel != CMODEL_SMALL) @@ -3503,23 +3502,18 @@ rs6000_linux64_override_options () SET_CMODEL (CMODEL_SMALL); else if (TARGET_PCREL || (PCREL_SUPPORTED_BY_OS - && (rs6000_isa_flags_explicit - & OPTION_MASK_PCREL) == 0)) + && (rs6000_isa_flags_explicit & OPTION_MASK_PCREL) == 0)) /* Ignore -mno-minimal-toc. */ ; else SET_CMODEL (CMODEL_SMALL); } - else + if (rs6000_current_cmodel != CMODEL_SMALL) { - if (rs6000_current_cmodel != CMODEL_SMALL) - { - if (!global_options_set.x_TARGET_NO_FP_IN_TOC) - TARGET_NO_FP_IN_TOC - = rs6000_current_cmodel == CMODEL_MEDIUM; - if (!global_options_set.x_TARGET_NO_SUM_IN_TOC) - TARGET_NO_SUM_IN_TOC = 0; - } + if (!global_options_set.x_TARGET_NO_FP_IN_TOC) + TARGET_NO_FP_IN_TOC = rs6000_current_cmodel == CMODEL_MEDIUM; + if (!global_options_set.x_TARGET_NO_SUM_IN_TOC) + TARGET_NO_SUM_IN_TOC = 0; } if (TARGET_PLTSEQ && DEFAULT_ABI != ABI_ELFv2) {
Hi Alan, On Fri, Oct 02, 2020 at 07:06:46AM +0930, Alan Modra wrote: > > > I was looking at it again today > > > with the aim of converting this ugly macro to a function, and spotted > > > the duplication in freebsd64.h. Which has some bit-rot. > > > > > > Do you like the following? rs6000_linux64_override_options is > > > functionally the same as what was in linux64.h. I built various > > > configurations to test the change, powerpc64-linux, powerpc64le-linux > > > without any 32-bit targets enabled, powerpc64-freebsd12.0. > > > > Please do this as two patches? One the refactoring without any > > functional changes (which is pre-approved -- the name "linux64" isn't > > great if you use it in other OSes as well, but I cannot think of a > > better name either), > > The patch as posted has no functional changes. Ah. You said the freebsd one had some bitrot, and I couldn't spot that easily. But in the actual patch you are just throwing away all of the freebsd stuff here, and the new, shared implementation is exactly what the linux one was? That is fine (I hope :-) ). > I do have a followup patch.. Commit c6be439b37 wrongly left a block > of code inside and "else" block, which changed the default for power10 > TARGET_NO_FP_IN_TOC accidentally. We don't want FP constants in the > TOC when -mcmodel=medium can address them just as efficiently outside > the TOC. > > * config/rs6000/rs6000.c (rs6000_linux64_override_options): > Formatting. Correct setting of TARGET_NO_FP_IN_TOC and > TARGET_NO_SUM_IN_TOC. Okay for trunk. Thanks! Segher
diff --git a/gcc/config/rs6000/linux64.h b/gcc/config/rs6000/linux64.h index 2ded3301282..2de1097d3ec 100644 --- a/gcc/config/rs6000/linux64.h +++ b/gcc/config/rs6000/linux64.h @@ -132,20 +132,25 @@ extern int dot_symbols; if ((rs6000_isa_flags & OPTION_MASK_POWERPC64) == 0) \ { \ rs6000_isa_flags |= OPTION_MASK_POWERPC64; \ - error ("%<-m64%> requires a PowerPC64 cpu"); \ + error ("%<-m64%> requires a PowerPC64 cpu"); \ } \ + if (!global_options_set.x_rs6000_current_cmodel) \ + SET_CMODEL (CMODEL_MEDIUM); \ if ((rs6000_isa_flags_explicit \ & OPTION_MASK_MINIMAL_TOC) != 0) \ { \ if (global_options_set.x_rs6000_current_cmodel \ && rs6000_current_cmodel != CMODEL_SMALL) \ error ("%<-mcmodel incompatible with other toc options%>"); \ - SET_CMODEL (CMODEL_SMALL); \ + if (TARGET_MINIMAL_TOC \ + || !(TARGET_PCREL \ + || (PCREL_SUPPORTED_BY_OS \ + && (rs6000_isa_flags_explicit \ + & OPTION_MASK_PCREL) == 0))) \ + SET_CMODEL (CMODEL_SMALL); \ } \ else \ { \ - if (!global_options_set.x_rs6000_current_cmodel) \ - SET_CMODEL (CMODEL_MEDIUM); \ if (rs6000_current_cmodel != CMODEL_SMALL) \ { \ if (!global_options_set.x_TARGET_NO_FP_IN_TOC) \ diff --git a/libgcc/config/rs6000/t-linux b/libgcc/config/rs6000/t-linux index 4f6d4c4a4d2..ed821947b66 100644 --- a/libgcc/config/rs6000/t-linux +++ b/libgcc/config/rs6000/t-linux @@ -1,3 +1,8 @@ SHLIB_MAPFILES += $(srcdir)/config/rs6000/libgcc-glibc.ver -HOST_LIBGCC2_CFLAGS += -mlong-double-128 -mno-minimal-toc +HOST_LIBGCC2_CFLAGS += -mlong-double-128 + +# This is a way of selecting -mcmodel=small for ppc64, which gives +# smaller and faster libgcc code. Directly specifying -mcmodel=small +# would need to take into account targets for which -mcmodel is invalid. +HOST_LIBGCC2_CFLAGS += -mno-minimal-toc