Ping?
Best regards,
Thomas
On Mon, 15 Oct 2018 at 16:01, Thomas Preudhomme
<thomas.preudho...@linaro.org> wrote:
>
> Ping?
>
> Best regards,
>
> Thomas
> On Fri, 5 Oct 2018 at 17:50, Thomas Preudhomme
> <thomas.preudho...@linaro.org> wrote:
> >
> > Hi Ramana and Kyrill,
> >
> > I've reworked the patch to add some documentation of the option
> > conflict and reworked the -mword-relocation logic slightly to set the
> > variable explicitely in PIC mode rather than test for PIC and word
> > relocation everywhere.
> >
> > ChangeLog entries are now as follows:
> >
> > *** gcc/ChangeLog ***
> >
> > 2018-10-02 Thomas Preud'homme <thomas.preudho...@linaro.org>
> >
> > PR target/87374
> > * config/arm/arm.c (arm_option_check_internal): Disable the combined
> > use of -mslow-flash-data and -mword-relocations.
> > (arm_option_override): Enable -mword-relocations if -fpic or -fPIC.
> > * config/arm/arm.md (SYMBOL_REF MOVT splitter): Stop checking for
> > flag_pic.
> > * doc/invoke.texi (-mword-relocations): Mention conflict with
> > -mslow-flash-data.
> > (-mslow-flash-data): Reciprocally.
> >
> > *** gcc/testsuite/ChangeLog ***
> >
> > 2018-09-25 Thomas Preud'homme <thomas.preudho...@linaro.org>
> >
> > PR target/87374
> > * gcc.target/arm/movdi_movt.c: Skip if both -mslow-flash-data and
> > -mword-relocations would be passed when compiling the test.
> > * gcc.target/arm/movsi_movt.c: Likewise.
> > * gcc.target/arm/pr81863.c: Likewise.
> > * gcc.target/arm/thumb2-slow-flash-data-1.c: Likewise.
> > * gcc.target/arm/thumb2-slow-flash-data-2.c: Likewise.
> > * gcc.target/arm/thumb2-slow-flash-data-3.c: Likewise.
> > * gcc.target/arm/thumb2-slow-flash-data-4.c: Likewise.
> > * gcc.target/arm/thumb2-slow-flash-data-5.c: Likewise.
> > * gcc.target/arm/tls-disable-literal-pool.c: Likewise.
> >
> > Is this ok for trunk?
> >
> > Best regards,
> >
> > Thomas
> >
> > On Tue, 2 Oct 2018 at 13:39, Ramana Radhakrishnan
> > <ramana.radhakrish...@foss.arm.com> wrote:
> > >
> > > On 02/10/2018 11:42, Thomas Preudhomme wrote:
> > > > Hi Ramana,
> > > >
> > > > On Thu, 27 Sep 2018 at 11:14, Ramana Radhakrishnan
> > > > <ramana.radhakrish...@arm.com> wrote:
> > > >>
> > > >> On 27/09/2018 09:26, Kyrill Tkachov wrote:
> > > >>> Hi Thomas,
> > > >>>
> > > >>> On 26/09/18 18:39, Thomas Preudhomme wrote:
> > > >>>> Hi,
> > > >>>>
> > > >>>> GCC ICEs under -mslow-flash-data and -mword-relocations because there
> > > >>>> is no way to load an address, both literal pools and MOVW/MOVT being
> > > >>>> forbidden. This patch gives an error message when both options are
> > > >>>> specified by the user and adds the according dg-skip-if directives
> > > >>>> for
> > > >>>> tests that use either of these options.
> > > >>>>
> > > >>>> ChangeLog entries are as follows:
> > > >>>>
> > > >>>> *** gcc/ChangeLog ***
> > > >>>>
> > > >>>> 2018-09-25 Thomas Preud'homme <thomas.preudho...@linaro.org>
> > > >>>>
> > > >>>> PR target/87374
> > > >>>> * config/arm/arm.c (arm_option_check_internal): Disable the
> > > >>>> combined
> > > >>>> use of -mslow-flash-data and -mword-relocations.
> > > >>>>
> > > >>>> *** gcc/testsuite/ChangeLog ***
> > > >>>>
> > > >>>> 2018-09-25 Thomas Preud'homme <thomas.preudho...@linaro.org>
> > > >>>>
> > > >>>> PR target/87374
> > > >>>> * gcc.target/arm/movdi_movt.c: Skip if both -mslow-flash-data
> > > >>>> and
> > > >>>> -mword-relocations would be passed when compiling the test.
> > > >>>> * gcc.target/arm/movsi_movt.c: Likewise.
> > > >>>> * gcc.target/arm/pr81863.c: Likewise.
> > > >>>> * gcc.target/arm/thumb2-slow-flash-data-1.c: Likewise.
> > > >>>> * gcc.target/arm/thumb2-slow-flash-data-2.c: Likewise.
> > > >>>> * gcc.target/arm/thumb2-slow-flash-data-3.c: Likewise.
> > > >>>> * gcc.target/arm/thumb2-slow-flash-data-4.c: Likewise.
> > > >>>> * gcc.target/arm/thumb2-slow-flash-data-5.c: Likewise.
> > > >>>> * gcc.target/arm/tls-disable-literal-pool.c: Likewise.
> > > >>>>
> > > >>>>
> > > >>>> Testing: Bootstrapped in Thumb-2 mode. No testsuite regression when
> > > >>>> targeting arm-none-eabi. Modified tests get skipped as expected when
> > > >>>> running the testsuite with -mslow-flash-data (pr81863.c) or
> > > >>>> -mword-relocations (all the others).
> > > >>>>
> > > >>>>
> > > >>>> Is this ok for trunk? I'd also appreciate guidance on whether this is
> > > >>>> worth a backport. It's a simple patch but on the other hand it only
> > > >>>> prevents some option combination, it does not fix anything so I have
> > > >>>> mixed feelings.
> > > >>>
> > > >>> In my opinion -mslow-flash-data is more of a tuning option rather
> > > >>> than a security/ABI feature
> > > >>> and therefore erroring out on its combination with -mword-relocations
> > > >>> feels odd.
> > > >>> I'm leaning more towards making -mword-relocations or any other
> > > >>> option that really requires constant pools
> > > >>> to bypass/disable the effects of -mslow-flash-data instead.
> > > >>
> > > >> -mslow-flash-data and -mword-relocations are contradictory in their
> > > >> expectations. mslow-flash-data is for not putting anything in the
> > > >> literal pool whereas mword-relocations is purely around the use of movw
> > > >> / movt instructions for word sized values. I wish we had called
> > > >> -mslow-flash-data something else (probably -mno-literal-pools).
> > > >> -mslow-flash-data is used primarily by M-profile users and
> > > >> -mword-relocations IIUC was a point fix for use in the Linux kernel for
> > > >> module loads at a time when not all module loaders in the linux kernel
> > > >> were fixed for the movw / movt relocations and armv7-a / thumb2 was in
> > > >> it's infancy :). Thus they are used by different constituencies in
> > > >> general and I wouldn't see them used together by actual users.
> > > >
> > > > Technically, -mslow-flash-data does not forbid literal pool, it just
> > > > discourages it because it's slower than many instructions. -mpure-code
> > > > on the other hand reuse the same logic and does forbid literal pools.
> > > > We could treat -mslow-flash-data differently but the question is
> > > > whether it is worth the trouble.
> > >
> > > Well, yeah I don't see the need for it. You could argue that
> > > -mslow-flash-data can be porous but realistically if you want this as an
> > > effective performance option , such porosity should be discouraged very
> > > strongly ;)
> > >
> > > >
> > > > By the way, I've noticed that the documentation for -mword-relocations
> > > > says it defaults to on for -fpic and -fPIC but when looking through
> > > > the code I saw that target_word_relocation is not set in these case,
> > > > rather the initial commit checks that introduced -mword-relocation
> > > > also checks for flag_pic when checking target_word_relocation. However
> > > > a later commit added one more check for target_word_relocations but
> > > > nothing for flag_pic. I'm now consolidating this so that flag_pic sets
> > > > target_word_relocations. I'll do a regression testing with -fPIC and
> > > > then post an updated patch.
> > >
> > > I'm reasonably sure that's *not* going to have *any* effect on code
> > > generation as in the -fpic / -fPIC case we always produce the funny GOT
> > > unspecs and have never used movw / movt instructions in those sequences
> > > for addressing. If that had happened most of the world's dynamic
> > > libraries would have faulted by now because I don't think they can
> > > process absolute movw / movt relocations.
> > >
> > >
> > > It is automatically implied by the fact that we never produced PC
> > > relative versions of the immediates that get put into movw / movt
> > > instructions. I don't even remember us having effective relocations to
> > > implement this but this is going back a few years now.
> > >
> > >
> > > Sure that probably needs a comment rather than being implicit from the
> > > source or from my own head :)
> > >
> > > Ramana
From d21e1e0c343f60e4e7de293b4c3cb0e87bf22f2f Mon Sep 17 00:00:00 2001
From: Thomas Preud'homme <thomas.preudho...@linaro.org>
Date: Tue, 25 Sep 2018 15:55:10 +0100
Subject: [PATCH] [PATCH, GCC/ARM] Fix PR87374: ICE with -mslow-flash-data and
-mword-relocations
Hi,
GCC ICEs under -mslow-flash-data and -mword-relocations because there
is no way to load an address, both literal pools and MOVW/MOVT being
forbidden. This patch gives an error message when both options are
specified by the user and adds the according dg-skip-if directives for
tests that use either of these options. It also explicitely set the
option when in PIC mode as per documentation rather than always check
for target_word_relocation together with flag_pic.
ChangeLog entries are as follows:
*** gcc/ChangeLog ***
2018-10-02 Thomas Preud'homme <thomas.preudho...@linaro.org>
PR target/87374
* config/arm/arm.c (arm_option_check_internal): Disable the combined
use of -mslow-flash-data and -mword-relocations.
(arm_option_override): Enable -mword-relocations if -fpic or -fPIC.
* config/arm/arm.md (SYMBOL_REF MOVT splitter): Stop checking for
flag_pic.
* doc/invoke.texi (-mword-relocations): Mention conflict with
-mslow-flash-data.
(-mslow-flash-data): Reciprocally.
*** gcc/testsuite/ChangeLog ***
2018-09-25 Thomas Preud'homme <thomas.preudho...@linaro.org>
PR target/87374
* gcc.target/arm/movdi_movt.c: Skip if both -mslow-flash-data and
-mword-relocations would be passed when compiling the test.
* gcc.target/arm/movsi_movt.c: Likewise.
* gcc.target/arm/pr81863.c: Likewise.
* gcc.target/arm/thumb2-slow-flash-data-1.c: Likewise.
* gcc.target/arm/thumb2-slow-flash-data-2.c: Likewise.
* gcc.target/arm/thumb2-slow-flash-data-3.c: Likewise.
* gcc.target/arm/thumb2-slow-flash-data-4.c: Likewise.
* gcc.target/arm/thumb2-slow-flash-data-5.c: Likewise.
* gcc.target/arm/tls-disable-literal-pool.c: Likewise.
Is this ok for trunk?
Best regards,
Thomas
---
gcc/config/arm/arm.c | 22 +++++++++++++------
gcc/config/arm/arm.md | 2 +-
gcc/doc/invoke.texi | 4 ++--
gcc/testsuite/gcc.target/arm/movdi_movt.c | 1 +
gcc/testsuite/gcc.target/arm/movsi_movt.c | 1 +
gcc/testsuite/gcc.target/arm/pr81863.c | 1 +
.../gcc.target/arm/thumb2-slow-flash-data-1.c | 1 +
.../gcc.target/arm/thumb2-slow-flash-data-2.c | 1 +
.../gcc.target/arm/thumb2-slow-flash-data-3.c | 1 +
.../gcc.target/arm/thumb2-slow-flash-data-4.c | 1 +
.../gcc.target/arm/thumb2-slow-flash-data-5.c | 1 +
.../gcc.target/arm/tls-disable-literal-pool.c | 1 +
12 files changed, 27 insertions(+), 10 deletions(-)
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index 6332e68df05..043bbe534a2 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -2893,17 +2893,22 @@ arm_option_check_internal (struct gcc_options *opts)
flag_pic = 0;
}
- /* We only support -mpure-code and -mslow-flash-data on M-profile targets
- with MOVT. */
- if ((target_pure_code || target_slow_flash_data)
- && (!TARGET_HAVE_MOVT || arm_arch_notm || flag_pic || TARGET_NEON))
+ if (target_pure_code || target_slow_flash_data)
{
const char *flag = (target_pure_code ? "-mpure-code" :
"-mslow-flash-data");
- error ("%s only supports non-pic code on M-profile targets with the "
- "MOVT instruction", flag);
- }
+ /* We only support -mpure-code and -mslow-flash-data on M-profile targets
+ with MOVT. */
+ if (!TARGET_HAVE_MOVT || arm_arch_notm || flag_pic || TARGET_NEON)
+ error ("%s only supports non-pic code on M-profile targets with the "
+ "MOVT instruction", flag);
+
+ /* Cannot load addresses: -mslow-flash-data forbids literal pool and
+ -mword-relocations forbids relocation of MOVT/MOVW. */
+ if (target_word_relocations)
+ error ("%s incompatible with -mword-relocations", flag);
+ }
}
/* Recompute the global settings depending on target attribute options. */
@@ -3489,6 +3494,9 @@ arm_option_override (void)
arm_pic_register = pic_register;
}
+ if (flag_pic)
+ target_word_relocations = 1;
+
/* Enable -mfix-cortex-m3-ldrd by default for Cortex-M3 cores. */
if (fix_cm3_ldrd == 2)
{
diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
index 270b8e454b3..a773518cefa 100644
--- a/gcc/config/arm/arm.md
+++ b/gcc/config/arm/arm.md
@@ -6128,7 +6128,7 @@
[(set (match_operand:SI 0 "arm_general_register_operand" "")
(match_operand:SI 1 "general_operand" ""))]
"TARGET_USE_MOVT && GET_CODE (operands[1]) == SYMBOL_REF
- && !flag_pic && !target_word_relocations
+ && !target_word_relocations
&& !arm_tls_referenced_p (operands[1])"
[(clobber (const_int 0))]
{
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 7ef4e7a449b..8030b911cc4 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -16964,7 +16964,7 @@ this option and always use the original scheme.
Only generate absolute relocations on word-sized values (i.e. R_ARM_ABS32).
This is enabled by default on targets (uClinux, SymbianOS) where the runtime
loader imposes this restriction, and when @option{-fpic} or @option{-fPIC}
-is specified.
+is specified. This option conflicts with @option{-mslow-flash-data}.
@item -mfix-cortex-m3-ldrd
@opindex mfix-cortex-m3-ldrd
@@ -17001,7 +17001,7 @@ to Neon is high.
Assume loading data from flash is slower than fetching instruction.
Therefore literal load is minimized for better performance.
This option is only supported when compiling for ARMv7 M-profile and
-off by default.
+off by default. It conflicts with @option{-mword-relocations}.
@item -masm-syntax-unified
@opindex masm-syntax-unified
diff --git a/gcc/testsuite/gcc.target/arm/movdi_movt.c b/gcc/testsuite/gcc.target/arm/movdi_movt.c
index e2a28ccbd99..a01ffa0dc93 100644
--- a/gcc/testsuite/gcc.target/arm/movdi_movt.c
+++ b/gcc/testsuite/gcc.target/arm/movdi_movt.c
@@ -1,4 +1,5 @@
/* { dg-do compile { target { arm_cortex_m && { arm_thumb2_ok || arm_thumb1_movt_ok } } } } */
+/* { dg-skip-if "-mslow-flash-data and -mword-relocations incompatible" { *-*-* } { "-mword-relocations" } } */
/* { dg-options "-O2 -mslow-flash-data" } */
unsigned long long
diff --git a/gcc/testsuite/gcc.target/arm/movsi_movt.c b/gcc/testsuite/gcc.target/arm/movsi_movt.c
index 3cf46e2fd17..19d202ecd33 100644
--- a/gcc/testsuite/gcc.target/arm/movsi_movt.c
+++ b/gcc/testsuite/gcc.target/arm/movsi_movt.c
@@ -1,4 +1,5 @@
/* { dg-do compile { target { arm_cortex_m && { arm_thumb2_ok || arm_thumb1_movt_ok } } } } */
+/* { dg-skip-if "-mslow-flash-data and -mword-relocations incompatible" { *-*-* } { "-mword-relocations" } } */
/* { dg-options "-O2 -mslow-flash-data" } */
unsigned
diff --git a/gcc/testsuite/gcc.target/arm/pr81863.c b/gcc/testsuite/gcc.target/arm/pr81863.c
index 63b1ed66b2c..225a0c5cc2b 100644
--- a/gcc/testsuite/gcc.target/arm/pr81863.c
+++ b/gcc/testsuite/gcc.target/arm/pr81863.c
@@ -1,5 +1,6 @@
/* testsuite/gcc.target/arm/pr48183.c */
/* { dg-do compile } */
+/* { dg-skip-if "-mslow-flash-data and -mword-relocations incompatible" { *-*-* } { "-mslow-flash-data" } } */
/* { dg-options "-O2 -mword-relocations -march=armv7-a -marm" } */
/* { dg-final { scan-assembler-not "\[\\t \]+movw" } } */
diff --git a/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-1.c b/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-1.c
index 089a72b67f3..d10391a69ac 100644
--- a/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-1.c
+++ b/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-1.c
@@ -6,6 +6,7 @@
/* { dg-do compile } */
/* { dg-require-effective-target arm_cortex_m } */
/* { dg-require-effective-target arm_thumb2_ok } */
+/* { dg-skip-if "-mslow-flash-data and -mword-relocations incompatible" { *-*-* } { "-mword-relocations" } } */
/* { dg-options "-O2 -mthumb -mslow-flash-data" } */
float sf;
diff --git a/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-2.c b/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-2.c
index c87e050639d..90bd44e27e5 100644
--- a/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-2.c
+++ b/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-2.c
@@ -3,6 +3,7 @@
/* { dg-require-effective-target arm_thumb2_ok } */
/* { dg-skip-if "avoid conflicts with multilib options" { *-*-* } { "-mcpu=*" } { "-mcpu=cortex-m4" "-mcpu=cortex-m7" } } */
/* { dg-skip-if "do not override -mfloat-abi" { *-*-* } { "-mfloat-abi=*" } { "-mfloat-abi=hard" } } */
+/* { dg-skip-if "-mslow-flash-data and -mword-relocations incompatible" { *-*-* } { "-mword-relocations" } } */
/* { dg-options "-march=armv7e-m+fp -mfloat-abi=hard -O2 -mthumb -mslow-flash-data" } */
float f (float);
diff --git a/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-3.c b/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-3.c
index 8c6210ee6c9..5d9cd9c4df2 100644
--- a/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-3.c
+++ b/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-3.c
@@ -3,6 +3,7 @@
/* { dg-require-effective-target arm_thumb2_ok } */
/* { dg-skip-if "avoid conflicts with multilib options" { *-*-* } { "-mcpu=*" } { "-mcpu=cortex-m4" "-mcpu=cortex-m7" } } */
/* { dg-skip-if "do not override -mfloat-abi" { *-*-* } { "-mfloat-abi=*" } { "-mfloat-abi=hard" } } */
+/* { dg-skip-if "-mslow-flash-data and -mword-relocations incompatible" { *-*-* } { "-mword-relocations" } } */
/* { dg-options "-march=armv7e-m+fp -mfloat-abi=hard -mthumb -mslow-flash-data" } */
/* From PR71607 */
diff --git a/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-4.c b/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-4.c
index 1bcb6924ed2..0eeddd5e6ec 100644
--- a/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-4.c
+++ b/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-4.c
@@ -3,6 +3,7 @@
/* { dg-require-effective-target arm_thumb2_ok } */
/* { dg-skip-if "avoid conflicts with multilib options" { *-*-* } { "-mcpu=*" } { "-mcpu=cortex-m4" "-mcpu=cortex-m7" } } */
/* { dg-skip-if "do not override -mfloat-abi" { *-*-* } { "-mfloat-abi=*" } { "-mfloat-abi=hard" } } */
+/* { dg-skip-if "-mslow-flash-data and -mword-relocations incompatible" { *-*-* } { "-mword-relocations" } } */
/* { dg-options "-march=armv7e-m+fp -mfloat-abi=hard -O2 -mthumb -mslow-flash-data" } */
double __attribute__ ((target ("fpu=fpv5-d16")))
diff --git a/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-5.c b/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-5.c
index 808fff05faa..7d52f3801b6 100644
--- a/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-5.c
+++ b/gcc/testsuite/gcc.target/arm/thumb2-slow-flash-data-5.c
@@ -3,6 +3,7 @@
/* { dg-require-effective-target arm_thumb2_ok } */
/* { dg-skip-if "avoid conflicts with multilib options" { *-*-* } { "-mcpu=*" } { "-mcpu=cortex-m4" "-mcpu=cortex-m7" } } */
/* { dg-skip-if "do not override -mfloat-abi" { *-*-* } { "-mfloat-abi=*" } { "-mfloat-abi=hard" } } */
+/* { dg-skip-if "-mslow-flash-data and -mword-relocations incompatible" { *-*-* } { "-mword-relocations" } } */
/* { dg-options "-march=armv7e-m+fp -mfloat-abi=hard -O2 -mthumb -mslow-flash-data" } */
double __attribute__ ((target ("fpu=fpv5-sp-d16")))
diff --git a/gcc/testsuite/gcc.target/arm/tls-disable-literal-pool.c b/gcc/testsuite/gcc.target/arm/tls-disable-literal-pool.c
index 283201fdd97..834eaf6db92 100644
--- a/gcc/testsuite/gcc.target/arm/tls-disable-literal-pool.c
+++ b/gcc/testsuite/gcc.target/arm/tls-disable-literal-pool.c
@@ -2,6 +2,7 @@
/* { dg-require-effective-target tls_native } */
/* { dg-require-effective-target arm_cortex_m } */
/* { dg-require-effective-target arm_thumb2_ok } */
+/* { dg-skip-if "-mslow-flash-data and -mword-relocations incompatible" { *-*-* } { "-mword-relocations" } } */
/* { dg-options "-mslow-flash-data" } */
__thread int x = 0;
--
2.19.0
