| Message ID | 573AF507.6070602@foss.arm.com |
|---|---|
| State | New |
| Headers | show |
Ping. https://gcc.gnu.org/ml/gcc-patches/2016-05/msg01211.html Thanks, Kyrill On 17/05/16 11:40, Kyrill Tkachov wrote: > > On 13/05/16 12:05, Kyrill Tkachov wrote: >> Hi Christophe, >> >> On 12/05/16 20:57, Christophe Lyon wrote: >>> On 12 May 2016 at 11:48, Ramana Radhakrishnan <ramana.gcc@googlemail.com> wrote: >>>> On Thu, May 5, 2016 at 12:50 PM, Kyrill Tkachov >>>> <kyrylo.tkachov@foss.arm.com> wrote: >>>>> Hi all, >>>>> >>>>> In this PR we deal with some fallout from the conversion to unified >>>>> assembly. >>>>> We now end up emitting instructions like: >>>>> pop {r0,r1,r2,r3,pc}^ >>>>> which is not legal. We have to use an LDM form. >>>>> >>>>> There are bugs in two arm.c functions: output_return_instruction and >>>>> arm_output_multireg_pop. >>>>> >>>>> In output_return_instruction the buggy hunk from the conversion was: >>>>> else >>>>> - if (TARGET_UNIFIED_ASM) >>>>> sprintf (instr, "pop%s\t{", conditional); >>>>> - else >>>>> - sprintf (instr, "ldm%sfd\t%%|sp!, {", conditional); >>>>> >>>>> The code was already very obscurely structured and arguably the bug was >>>>> latent. >>>>> It emitted POP only when TARGET_UNIFIED_ASM was on, and since >>>>> TARGET_UNIFIED_ASM was on >>>>> only for Thumb, we never went down this path interrupt handling code, since >>>>> the interrupt >>>>> attribute is only available for ARM code. After the removal of >>>>> TARGET_UNIFIED_ASM we ended up >>>>> using POP unconditionally. So this patch adds a check for IS_INTERRUPT and >>>>> outputs the >>>>> appropriate LDM form. >>>>> >>>>> In arm_output_multireg_pop the buggy hunk was: >>>>> - if ((regno_base == SP_REGNUM) && TARGET_THUMB) >>>>> + if ((regno_base == SP_REGNUM) && update) >>>>> { >>>>> - /* Output pop (not stmfd) because it has a shorter encoding. */ >>>>> - gcc_assert (update); >>>>> sprintf (pattern, "pop%s\t{", conditional); >>>>> } >>>>> >>>>> Again, the POP was guarded on TARGET_THUMB and so would never be taken on >>>>> interrupt handling >>>>> routines. This patch guards that with the appropriate check on interrupt >>>>> return. >>>>> >>>>> Also, there are a couple of bugs in the 'else' branch of that 'if': >>>>> * The "ldmfd%s" was output without a '\t' at the end which meant that the >>>>> base register >>>>> name would be concatenated with the 'ldmfd', creating invalid assembly. >>>>> >>>>> * The logic: >>>>> >>>>> if (regno_base == SP_REGNUM) >>>>> /* update is never true here, hence there is no need to handle >>>>> pop here. */ >>>>> sprintf (pattern, "ldmfd%s", conditional); >>>>> >>>>> if (update) >>>>> sprintf (pattern, "ldmia%s\t", conditional); >>>>> else >>>>> sprintf (pattern, "ldm%s\t", conditional); >>>>> >>>>> Meant that for "regno == SP_REGNUM && !update" we'd end up printing >>>>> "ldmfd%sldm%s\t" >>>>> to pattern. I didn't manage to reproduce that condition though, so maybe it >>>>> can't ever occur. >>>>> This patch fixes both these issues nevertheless. >>>>> >>>>> I've added the testcase from the PR to catch the fix in >>>>> output_return_instruction. >>>>> The testcase doesn't catch the bugs in arm_output_multireg_pop, but the >>>>> existing tests >>>>> gcc.target/arm/interrupt-1.c and gcc.target/arm/interrupt-2.c would have >>>>> caught them >>>>> if only they were assemble tests rather than just compile. So this patch >>>>> makes them >>>>> assembly tests (and reverts the scan-assembler checks for the correct LDM >>>>> pattern). >>>>> >>>>> Bootstrapped and tested on arm-none-linux-gnueabihf. >>>>> Ok for trunk and GCC 6? >>>>> >>> Hi Kyrill, >>> >>> Did you test --with-mode=thumb? >>> When using arm mode, I see regressions: >>> >>> gcc.target/arm/neon-nested-apcs.c (test for excess errors) >>> gcc.target/arm/nested-apcs.c (test for excess errors) >> >> It's because I have a local patch in my binutils that makes gas warn on the >> deprecated sequences that these two tests generate (they use the deprecated -mapcs option), >> so these tests were already showing the (test for excess errors) FAIL for me, >> so I they didn't appear in my tests diff for this patch. :( >> >> I've reproduced the failure with a clean tree. >> Where before we generated: >> ldm sp, {fp, sp, pc} >> now we generate: >> pop {fp, sp, pc} >> >> which are not equivalent (pop performs a write-back) and gas warns: >> Warning: writeback of base register when in register list is UNPREDICTABLE >> >> I'm testing a patch to fix this. >> Sorry for the regression. > > Here is the fix. > I had remove the update from the condition for the "pop" erroneously. Of course, if we're not > updating the SP we can't use POP that has an implicit writeback. > > Bootstrapped on arm-none-linux-gnueabihf. Tested with -mthumb and -marm. > > Ok for trunk and GCC 6? > > Thanks, > Kyrill > > 2016-05-17 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > PR target/70830 > * config/arm/arm.c (arm_output_multireg_pop): Guard "pop" on update. >
Ping. Thanks, Kyrill On 24/05/16 13:48, Kyrill Tkachov wrote: > Ping. > https://gcc.gnu.org/ml/gcc-patches/2016-05/msg01211.html > > Thanks, > Kyrill > > On 17/05/16 11:40, Kyrill Tkachov wrote: >> >> On 13/05/16 12:05, Kyrill Tkachov wrote: >>> Hi Christophe, >>> >>> On 12/05/16 20:57, Christophe Lyon wrote: >>>> On 12 May 2016 at 11:48, Ramana Radhakrishnan <ramana.gcc@googlemail.com> wrote: >>>>> On Thu, May 5, 2016 at 12:50 PM, Kyrill Tkachov >>>>> <kyrylo.tkachov@foss.arm.com> wrote: >>>>>> Hi all, >>>>>> >>>>>> In this PR we deal with some fallout from the conversion to unified >>>>>> assembly. >>>>>> We now end up emitting instructions like: >>>>>> pop {r0,r1,r2,r3,pc}^ >>>>>> which is not legal. We have to use an LDM form. >>>>>> >>>>>> There are bugs in two arm.c functions: output_return_instruction and >>>>>> arm_output_multireg_pop. >>>>>> >>>>>> In output_return_instruction the buggy hunk from the conversion was: >>>>>> else >>>>>> - if (TARGET_UNIFIED_ASM) >>>>>> sprintf (instr, "pop%s\t{", conditional); >>>>>> - else >>>>>> - sprintf (instr, "ldm%sfd\t%%|sp!, {", conditional); >>>>>> >>>>>> The code was already very obscurely structured and arguably the bug was >>>>>> latent. >>>>>> It emitted POP only when TARGET_UNIFIED_ASM was on, and since >>>>>> TARGET_UNIFIED_ASM was on >>>>>> only for Thumb, we never went down this path interrupt handling code, since >>>>>> the interrupt >>>>>> attribute is only available for ARM code. After the removal of >>>>>> TARGET_UNIFIED_ASM we ended up >>>>>> using POP unconditionally. So this patch adds a check for IS_INTERRUPT and >>>>>> outputs the >>>>>> appropriate LDM form. >>>>>> >>>>>> In arm_output_multireg_pop the buggy hunk was: >>>>>> - if ((regno_base == SP_REGNUM) && TARGET_THUMB) >>>>>> + if ((regno_base == SP_REGNUM) && update) >>>>>> { >>>>>> - /* Output pop (not stmfd) because it has a shorter encoding. */ >>>>>> - gcc_assert (update); >>>>>> sprintf (pattern, "pop%s\t{", conditional); >>>>>> } >>>>>> >>>>>> Again, the POP was guarded on TARGET_THUMB and so would never be taken on >>>>>> interrupt handling >>>>>> routines. This patch guards that with the appropriate check on interrupt >>>>>> return. >>>>>> >>>>>> Also, there are a couple of bugs in the 'else' branch of that 'if': >>>>>> * The "ldmfd%s" was output without a '\t' at the end which meant that the >>>>>> base register >>>>>> name would be concatenated with the 'ldmfd', creating invalid assembly. >>>>>> >>>>>> * The logic: >>>>>> >>>>>> if (regno_base == SP_REGNUM) >>>>>> /* update is never true here, hence there is no need to handle >>>>>> pop here. */ >>>>>> sprintf (pattern, "ldmfd%s", conditional); >>>>>> >>>>>> if (update) >>>>>> sprintf (pattern, "ldmia%s\t", conditional); >>>>>> else >>>>>> sprintf (pattern, "ldm%s\t", conditional); >>>>>> >>>>>> Meant that for "regno == SP_REGNUM && !update" we'd end up printing >>>>>> "ldmfd%sldm%s\t" >>>>>> to pattern. I didn't manage to reproduce that condition though, so maybe it >>>>>> can't ever occur. >>>>>> This patch fixes both these issues nevertheless. >>>>>> >>>>>> I've added the testcase from the PR to catch the fix in >>>>>> output_return_instruction. >>>>>> The testcase doesn't catch the bugs in arm_output_multireg_pop, but the >>>>>> existing tests >>>>>> gcc.target/arm/interrupt-1.c and gcc.target/arm/interrupt-2.c would have >>>>>> caught them >>>>>> if only they were assemble tests rather than just compile. So this patch >>>>>> makes them >>>>>> assembly tests (and reverts the scan-assembler checks for the correct LDM >>>>>> pattern). >>>>>> >>>>>> Bootstrapped and tested on arm-none-linux-gnueabihf. >>>>>> Ok for trunk and GCC 6? >>>>>> >>>> Hi Kyrill, >>>> >>>> Did you test --with-mode=thumb? >>>> When using arm mode, I see regressions: >>>> >>>> gcc.target/arm/neon-nested-apcs.c (test for excess errors) >>>> gcc.target/arm/nested-apcs.c (test for excess errors) >>> >>> It's because I have a local patch in my binutils that makes gas warn on the >>> deprecated sequences that these two tests generate (they use the deprecated -mapcs option), >>> so these tests were already showing the (test for excess errors) FAIL for me, >>> so I they didn't appear in my tests diff for this patch. :( >>> >>> I've reproduced the failure with a clean tree. >>> Where before we generated: >>> ldm sp, {fp, sp, pc} >>> now we generate: >>> pop {fp, sp, pc} >>> >>> which are not equivalent (pop performs a write-back) and gas warns: >>> Warning: writeback of base register when in register list is UNPREDICTABLE >>> >>> I'm testing a patch to fix this. >>> Sorry for the regression. >> >> Here is the fix. >> I had remove the update from the condition for the "pop" erroneously. Of course, if we're not >> updating the SP we can't use POP that has an implicit writeback. >> >> Bootstrapped on arm-none-linux-gnueabihf. Tested with -mthumb and -marm. >> >> Ok for trunk and GCC 6? >> >> Thanks, >> Kyrill >> >> 2016-05-17 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >> >> PR target/70830 >> * config/arm/arm.c (arm_output_multireg_pop): Guard "pop" on update. >> >
On 17/05/16 11:40, Kyrill Tkachov wrote: > > On 13/05/16 12:05, Kyrill Tkachov wrote: >> Hi Christophe, >> >> On 12/05/16 20:57, Christophe Lyon wrote: >>> On 12 May 2016 at 11:48, Ramana Radhakrishnan <ramana.gcc@googlemail.com> wrote: >>>> On Thu, May 5, 2016 at 12:50 PM, Kyrill Tkachov >>>> <kyrylo.tkachov@foss.arm.com> wrote: >>>>> Hi all, >>>>> >>>>> In this PR we deal with some fallout from the conversion to unified >>>>> assembly. >>>>> We now end up emitting instructions like: >>>>> pop {r0,r1,r2,r3,pc}^ >>>>> which is not legal. We have to use an LDM form. >>>>> >>>>> There are bugs in two arm.c functions: output_return_instruction and >>>>> arm_output_multireg_pop. >>>>> >>>>> In output_return_instruction the buggy hunk from the conversion was: >>>>> else >>>>> - if (TARGET_UNIFIED_ASM) >>>>> sprintf (instr, "pop%s\t{", conditional); >>>>> - else >>>>> - sprintf (instr, "ldm%sfd\t%%|sp!, {", conditional); >>>>> >>>>> The code was already very obscurely structured and arguably the bug was >>>>> latent. >>>>> It emitted POP only when TARGET_UNIFIED_ASM was on, and since >>>>> TARGET_UNIFIED_ASM was on >>>>> only for Thumb, we never went down this path interrupt handling code, since >>>>> the interrupt >>>>> attribute is only available for ARM code. After the removal of >>>>> TARGET_UNIFIED_ASM we ended up >>>>> using POP unconditionally. So this patch adds a check for IS_INTERRUPT and >>>>> outputs the >>>>> appropriate LDM form. >>>>> >>>>> In arm_output_multireg_pop the buggy hunk was: >>>>> - if ((regno_base == SP_REGNUM) && TARGET_THUMB) >>>>> + if ((regno_base == SP_REGNUM) && update) >>>>> { >>>>> - /* Output pop (not stmfd) because it has a shorter encoding. */ >>>>> - gcc_assert (update); >>>>> sprintf (pattern, "pop%s\t{", conditional); >>>>> } >>>>> >>>>> Again, the POP was guarded on TARGET_THUMB and so would never be taken on >>>>> interrupt handling >>>>> routines. This patch guards that with the appropriate check on interrupt >>>>> return. >>>>> >>>>> Also, there are a couple of bugs in the 'else' branch of that 'if': >>>>> * The "ldmfd%s" was output without a '\t' at the end which meant that the >>>>> base register >>>>> name would be concatenated with the 'ldmfd', creating invalid assembly. >>>>> >>>>> * The logic: >>>>> >>>>> if (regno_base == SP_REGNUM) >>>>> /* update is never true here, hence there is no need to handle >>>>> pop here. */ >>>>> sprintf (pattern, "ldmfd%s", conditional); >>>>> >>>>> if (update) >>>>> sprintf (pattern, "ldmia%s\t", conditional); >>>>> else >>>>> sprintf (pattern, "ldm%s\t", conditional); >>>>> >>>>> Meant that for "regno == SP_REGNUM && !update" we'd end up printing >>>>> "ldmfd%sldm%s\t" >>>>> to pattern. I didn't manage to reproduce that condition though, so maybe it >>>>> can't ever occur. >>>>> This patch fixes both these issues nevertheless. >>>>> >>>>> I've added the testcase from the PR to catch the fix in >>>>> output_return_instruction. >>>>> The testcase doesn't catch the bugs in arm_output_multireg_pop, but the >>>>> existing tests >>>>> gcc.target/arm/interrupt-1.c and gcc.target/arm/interrupt-2.c would have >>>>> caught them >>>>> if only they were assemble tests rather than just compile. So this patch >>>>> makes them >>>>> assembly tests (and reverts the scan-assembler checks for the correct LDM >>>>> pattern). >>>>> >>>>> Bootstrapped and tested on arm-none-linux-gnueabihf. >>>>> Ok for trunk and GCC 6? >>>>> >>> Hi Kyrill, >>> >>> Did you test --with-mode=thumb? >>> When using arm mode, I see regressions: >>> >>> gcc.target/arm/neon-nested-apcs.c (test for excess errors) >>> gcc.target/arm/nested-apcs.c (test for excess errors) >> >> It's because I have a local patch in my binutils that makes gas warn on the >> deprecated sequences that these two tests generate (they use the deprecated -mapcs option), >> so these tests were already showing the (test for excess errors) FAIL for me, >> so I they didn't appear in my tests diff for this patch. :( >> >> I've reproduced the failure with a clean tree. >> Where before we generated: >> ldm sp, {fp, sp, pc} >> now we generate: >> pop {fp, sp, pc} >> >> which are not equivalent (pop performs a write-back) and gas warns: >> Warning: writeback of base register when in register list is UNPREDICTABLE >> >> I'm testing a patch to fix this. >> Sorry for the regression. > > Here is the fix. > I had remove the update from the condition for the "pop" erroneously. Of course, if we're not > updating the SP we can't use POP that has an implicit writeback. > > Bootstrapped on arm-none-linux-gnueabihf. Tested with -mthumb and -marm. > > Ok for trunk and GCC 6? > > Thanks, > Kyrill > > 2016-05-17 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > PR target/70830 > * config/arm/arm.c (arm_output_multireg_pop): Guard "pop" on update. > Ok. Thanks, Ramana
On 02/06/16 09:47, Ramana Radhakrishnan wrote: > On 17/05/16 11:40, Kyrill Tkachov wrote: >> On 13/05/16 12:05, Kyrill Tkachov wrote: >>> Hi Christophe, >>> >>> On 12/05/16 20:57, Christophe Lyon wrote: >>>> On 12 May 2016 at 11:48, Ramana Radhakrishnan <ramana.gcc@googlemail.com> wrote: >>>>> On Thu, May 5, 2016 at 12:50 PM, Kyrill Tkachov >>>>> <kyrylo.tkachov@foss.arm.com> wrote: >>>>>> Hi all, >>>>>> >>>>>> In this PR we deal with some fallout from the conversion to unified >>>>>> assembly. >>>>>> We now end up emitting instructions like: >>>>>> pop {r0,r1,r2,r3,pc}^ >>>>>> which is not legal. We have to use an LDM form. >>>>>> >>>>>> There are bugs in two arm.c functions: output_return_instruction and >>>>>> arm_output_multireg_pop. >>>>>> >>>>>> In output_return_instruction the buggy hunk from the conversion was: >>>>>> else >>>>>> - if (TARGET_UNIFIED_ASM) >>>>>> sprintf (instr, "pop%s\t{", conditional); >>>>>> - else >>>>>> - sprintf (instr, "ldm%sfd\t%%|sp!, {", conditional); >>>>>> >>>>>> The code was already very obscurely structured and arguably the bug was >>>>>> latent. >>>>>> It emitted POP only when TARGET_UNIFIED_ASM was on, and since >>>>>> TARGET_UNIFIED_ASM was on >>>>>> only for Thumb, we never went down this path interrupt handling code, since >>>>>> the interrupt >>>>>> attribute is only available for ARM code. After the removal of >>>>>> TARGET_UNIFIED_ASM we ended up >>>>>> using POP unconditionally. So this patch adds a check for IS_INTERRUPT and >>>>>> outputs the >>>>>> appropriate LDM form. >>>>>> >>>>>> In arm_output_multireg_pop the buggy hunk was: >>>>>> - if ((regno_base == SP_REGNUM) && TARGET_THUMB) >>>>>> + if ((regno_base == SP_REGNUM) && update) >>>>>> { >>>>>> - /* Output pop (not stmfd) because it has a shorter encoding. */ >>>>>> - gcc_assert (update); >>>>>> sprintf (pattern, "pop%s\t{", conditional); >>>>>> } >>>>>> >>>>>> Again, the POP was guarded on TARGET_THUMB and so would never be taken on >>>>>> interrupt handling >>>>>> routines. This patch guards that with the appropriate check on interrupt >>>>>> return. >>>>>> >>>>>> Also, there are a couple of bugs in the 'else' branch of that 'if': >>>>>> * The "ldmfd%s" was output without a '\t' at the end which meant that the >>>>>> base register >>>>>> name would be concatenated with the 'ldmfd', creating invalid assembly. >>>>>> >>>>>> * The logic: >>>>>> >>>>>> if (regno_base == SP_REGNUM) >>>>>> /* update is never true here, hence there is no need to handle >>>>>> pop here. */ >>>>>> sprintf (pattern, "ldmfd%s", conditional); >>>>>> >>>>>> if (update) >>>>>> sprintf (pattern, "ldmia%s\t", conditional); >>>>>> else >>>>>> sprintf (pattern, "ldm%s\t", conditional); >>>>>> >>>>>> Meant that for "regno == SP_REGNUM && !update" we'd end up printing >>>>>> "ldmfd%sldm%s\t" >>>>>> to pattern. I didn't manage to reproduce that condition though, so maybe it >>>>>> can't ever occur. >>>>>> This patch fixes both these issues nevertheless. >>>>>> >>>>>> I've added the testcase from the PR to catch the fix in >>>>>> output_return_instruction. >>>>>> The testcase doesn't catch the bugs in arm_output_multireg_pop, but the >>>>>> existing tests >>>>>> gcc.target/arm/interrupt-1.c and gcc.target/arm/interrupt-2.c would have >>>>>> caught them >>>>>> if only they were assemble tests rather than just compile. So this patch >>>>>> makes them >>>>>> assembly tests (and reverts the scan-assembler checks for the correct LDM >>>>>> pattern). >>>>>> >>>>>> Bootstrapped and tested on arm-none-linux-gnueabihf. >>>>>> Ok for trunk and GCC 6? >>>>>> >>>> Hi Kyrill, >>>> >>>> Did you test --with-mode=thumb? >>>> When using arm mode, I see regressions: >>>> >>>> gcc.target/arm/neon-nested-apcs.c (test for excess errors) >>>> gcc.target/arm/nested-apcs.c (test for excess errors) >>> It's because I have a local patch in my binutils that makes gas warn on the >>> deprecated sequences that these two tests generate (they use the deprecated -mapcs option), >>> so these tests were already showing the (test for excess errors) FAIL for me, >>> so I they didn't appear in my tests diff for this patch. :( >>> >>> I've reproduced the failure with a clean tree. >>> Where before we generated: >>> ldm sp, {fp, sp, pc} >>> now we generate: >>> pop {fp, sp, pc} >>> >>> which are not equivalent (pop performs a write-back) and gas warns: >>> Warning: writeback of base register when in register list is UNPREDICTABLE >>> >>> I'm testing a patch to fix this. >>> Sorry for the regression. >> Here is the fix. >> I had remove the update from the condition for the "pop" erroneously. Of course, if we're not >> updating the SP we can't use POP that has an implicit writeback. >> >> Bootstrapped on arm-none-linux-gnueabihf. Tested with -mthumb and -marm. >> >> Ok for trunk and GCC 6? >> >> Thanks, >> Kyrill >> >> 2016-05-17 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >> >> PR target/70830 >> * config/arm/arm.c (arm_output_multireg_pop): Guard "pop" on update. >> > > Ok. Thanks, I've committed it with r237027. I'll backport to GCC 6 in a few days if there's no fallout. Sorry again for the trouble, Kyrill > Thanks, > Ramana >
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c index 2cc7f7b452a62f898346a51ca7ede0d19bcfcfad..68985547a634bb93ab59416e24aaa046ab99f6a6 100644 --- a/gcc/config/arm/arm.c +++ b/gcc/config/arm/arm.c @@ -17624,10 +17624,8 @@ arm_output_multireg_pop (rtx *operands, bool return_pc, rtx cond, bool reverse, conditional = reverse ? "%?%D0" : "%?%d0"; /* Can't use POP if returning from an interrupt. */ - if ((regno_base == SP_REGNUM) && !(interrupt_p && return_pc)) - { - sprintf (pattern, "pop%s\t{", conditional); - } + if ((regno_base == SP_REGNUM) && update && !(interrupt_p && return_pc)) + sprintf (pattern, "pop%s\t{", conditional); else { /* Output ldmfd when the base register is SP, otherwise output ldmia.