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....@googlemail.com>
>>> wrote:
>>>> On Thu, May 5, 2016 at 12:50 PM, Kyrill Tkachov
>>>> <kyrylo.tkac...@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.tkac...@arm.com>
>
> PR target/70830
> * config/arm/arm.c (arm_output_multireg_pop): Guard "pop" on update.
>
Ok.
Thanks,
Ramana
