Ping? I'd also like to backport this and the main patch (svn r279463, r10-5505-ge24f6408df1e4c5e8c09785d7b488c492dfb68b3) to the gcc-9 branch.
I found the problem addressed by this patch while validating the backport to gcc-9: although the patch applies cleanly except for testcases dg directives, there were some failures which I could finally reproduce on trunk with -fdisable-rtl-fwprop2. Here is a summary of the validations I ran using --target arm-eabi: * without my patches: (1) --with-cpu cortex-m0 (2) --with-cpu cortex-m4 (3) --with-cpu cortex-m4 CFLAGS_FOR_TARGET=-mpure-code (to build the libs with -mpure-code) (4) --with-cpu cortex-m4 CFLAGS_FOR_TARGET=-mpure-code --target-board=-mpure-code (to also run the tests with -mpure-code) * with my patches: (5) --with-cpu cortex-m0 CFLAGS_FOR_TARGET=-mpure-code --target-board=-mpure-code (6) --with-cpu cortex-m4 CFLAGS_FOR_TARGET=-mpure-code --target-board=-mpure-code Comparing (4) and (6) ensured that my (v6m) patches introduce no regression in v7m cases. Comparison of (1) vs (5) gave results similar to (2) vs (6), there's a bit of noise because some tests cases don't cope well with -mpure-code despite my previous testsuite-only patch (svn r277828) Comparison of (1) vs (2) gave similar results to (5) vs (6). Ideally, we may also want to backport svn r277828 (testsuite-only patch, to handle -mpure-code better), but that's not mandatory. In summary, is this patch OK for trunk? Are this patch and r279463, r10-5505-ge24f6408df1e4c5e8c09785d7b488c492dfb68b3 OK to backport to gcc-9? Thanks, Christophe On Thu, 13 Feb 2020 at 11:14, Christophe Lyon <christophe.l...@linaro.org> wrote: > > On Mon, 10 Feb 2020 at 17:45, Richard Earnshaw (lists) > <richard.earns...@arm.com> wrote: > > > > On 10/02/2020 09:27, Christophe Lyon wrote: > > > On Fri, 7 Feb 2020 at 17:55, Richard Earnshaw (lists) > > > <richard.earns...@arm.com> wrote: > > >> > > >> On 07/02/2020 16:43, Christophe Lyon wrote: > > >>> On Fri, 7 Feb 2020 at 14:49, Richard Earnshaw (lists) > > >>> <richard.earns...@arm.com> wrote: > > >>>> > > >>>> On 07/02/2020 13:19, Christophe Lyon wrote: > > >>>>> When running the testsuite with -fdisable-rtl-fwprop2 and -mpure-code > > >>>>> for cortex-m0, I noticed that some testcases were failing because we > > >>>>> still generate "ldr rX, .LCY", which is what we want to avoid with > > >>>>> -mpure-code. This is latent since a recent improvement in fwprop > > >>>>> (PR88833). > > >>>>> > > >>>>> In this patch I change the thumb1_movsi_insn pattern so that it emits > > >>>>> the desired instruction sequence when arm_disable_literal_pool is set. > > >>>>> > > >>>>> I tried to add a define_split instead, but couldn't make it work: the > > >>>>> compiler then complains it cannot split the instruction, while my new > > >>>>> define_split accepts the same operand types as thumb1_movsi_insn: > > >>>>> > > >>>>> c-c++-common/torture/complex-sign-mixed-add.c:41:1: error: could not > > >>>>> split insn > > >>>>> (insn 2989 425 4844 (set (reg/f:SI 3 r3 [1342]) > > >>>>> (symbol_ref/u:SI ("*.LC6") [flags 0x2])) 836 > > >>>>> {*thumb1_movsi_insn} > > >>>>> (expr_list:REG_EQUIV (symbol_ref/u:SI ("*.LC6") [flags 0x2]) > > >>>>> (nil))) > > >>>>> during RTL pass: final > > >>>>> > > >>>>> (define_split > > >>>>> [(set (match_operand:SI 0 "register_operand" "") > > >>>>> (match_operand:SI 1 "general_operand" ""))] > > >>>>> "TARGET_THUMB1 > > >>>>> && arm_disable_literal_pool > > >>>>> && GET_CODE (operands[1]) == SYMBOL_REF" > > >>>>> [(clobber (const_int 0))] > > >>>>> " > > >>>>> gen_thumb1_movsi_symbol_ref(operands[0], operands[1]); > > >>>>> DONE; > > >>>>> " > > >>>>> ) > > >>>>> and I put this in thumb1_movsi_insn: > > >>>>> if (GET_CODE (operands[1]) == SYMBOL_REF && arm_disable_literal_pool) > > >>>>> { > > >>>>> return \"#\"; > > >>>>> } > > >>>>> return \"ldr\\t%0, %1\"; > > >>>>> > > >>>>> 2020-02-07 Christophe Lyon <christophe.l...@linaro.org> > > >>>>> > > >>>>> * config/arm/thumb1.md (thumb1_movsi_insn): Fix ldr > > >>>>> alternative to > > >>>>> work with -mpure-code. > > >>>>> > > >>>> > > >>>> + case 0: > > >>>> + case 1: > > >>>> + return \"movs %0, %1\"; > > >>>> + case 2: > > >>>> + return \"movw %0, %1\"; > > >>>> > > >>>> This is OK, but please replace the hard tab in the strings for > > >>>> MOVS/MOVW > > >>>> with \\t. > > >>>> > > >>> > > >>> OK that was merely a cut & paste from the existing code. > > >>> > > >>> I'm concerned that the length attribute is becoming wrong with my > > >>> patch, isn't this a problem? > > >>> > > >> > > >> Potentially yes. The branch range code needs this to handle overly long > > >> jumps correctly. > > >> > > > > > > Do you mean that the probability of problems due to that shortcoming > > > is low enough that I can commit my patch? > > > > It's hard to say that. The number of instructions you generate for this > > is significant, so it likely will throw off the calculations and > > somebody will get unlucky. On the other hand, I don't think we should > > pessimize code for the non-pure-code variants by inflating the size for > > this unconditionally. > > > > It seems there are two ways to fix this. > > > > 1) create a new alternative for the pure-code variant with its own > > length attribute, then only enable that for the case you need. That > > would also allow you to go back to the templated asm format. > > 2) use a level of indirection to calculate the length - I haven't tried > > this, but it would be something like: > > > > - create a new attribute - lets say default_length > > - rename length for this pattern to default_length > > - create another new attribute - lets say purecode_length, add lengths > > for each alternative but adjusted for the purecode variant. > > - make the length attribute for this pattern select based on the > > disable_literal_pool variable between the default_length and > > purecode_length attributes. > > > > Hi Richard, > > Thanks for the suggestions. I've implemented option (1) above, does > it match what you had in mind? > > Tested on arm-eabi, with -mpure-code forced, no regression. Manually > checked that the expected sequence is generated with > -fdisable-rtl-fwprop2. > > Thanks, > > Christophe > > > R. > > > > > > > > Thanks, > > > > > > Christophe > > > > > >> R. > > >> > > >>> Thanks, > > >>> > > >>> Christophe > > >>> > > >>>> R. > > >> > >