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
