Ping? Best regards,
Thomas On Tue, 23 Oct 2018 at 10:10, Thomas Preudhomme <thomas.preudho...@linaro.org> wrote: > > 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
