This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH][ARM] PR target/70830: Avoid POP-{reglist}^ when returning from interrupt handlers


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)

Christophe

>> Thanks,
>> Kyrill
>>
>> 2016-05-05  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>
>>     PR target/70830
>>     * config/arm/arm.c (arm_output_multireg_pop): Avoid POP instruction
>>     when popping the PC and within an interrupt handler routine.
>>     Add missing tab to output of "ldmfd".
>>     (output_return_instruction): Output LDMFD with SP update rather
>>     than POP when returning from interrupt handler.
>>
>> 2016-05-05  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>
>>     PR target/70830
>>     * gcc.target/arm/interrupt-1.c: Change dg-compile to dg-assemble.
>>     Add -save-temps to dg-options.
>>     Scan for ldmfd rather than pop instruction.
>>     * gcc.target/arm/interrupt-2.c: Likewise.
>>     * gcc.target/arm/pr70830.c: New test.
>
>
> OK for affected branches and trunk  - thanks for fixing this and sorry
> about the breakage.
>
> Ramana


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]