Patchwork [ARM] Fix PR56797

login
register
mail settings
Submitter Greta Yorsh
Date April 23, 2013, 4:37 p.m.
Message ID <000c01ce4040$e548c840$afda58c0$@yorsh@arm.com>
Download mbox | patch
Permalink /patch/238961/
State New
Headers show

Comments

Greta Yorsh - April 23, 2013, 4:37 p.m.
Ok to backport to gcc4.8?

I'm attaching an updated version - just fixed a spelling error in the
comment. 

Thanks,
Greta

gcc/ChangeLog

	PR target/56797
	* config/arm/arm.c (load_multiple_sequence): Require SP
        as base register for loads if SP is in the register list.

> -----Original Message-----
> From: Richard Earnshaw
> Sent: 19 April 2013 12:34
> To: Greta Yorsh
> Cc: GCC Patches; raj.khem@gmail.com; Ramana Radhakrishnan
> Subject: Re: [PATCH, ARM] Fix PR56797
> 
> On 19/04/13 10:34, Greta Yorsh wrote:
> > Fix PR56797
> > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56797
> >
> > The problem is that peephole optimizer thinks it can generate an ldm,
> but
> > the pattern for ldm no longer matches, because after r188738 it
> requires
> > that if one of the destination registers is SP then the base register
> must
> > be SP, and it's not SP in the test case.
> >
> > The test case fails on armv5t but doesn't fail on armv6t2 or armv7-a
> because
> > peephole doesn't trigger there (because there is a different epilogue
> > sequence). It looks like a latent problem for other architecture or
> CPUs.
> >
> > This patch adds this condition to the peephole optimizer.
> >
> > No regression on qemu for arm-none-eabi and fixes the test reported
> in the
> > PR. I couldn't minimize the test sufficiently to include it in the
> > testsuite.
> >
> > Ok for trunk?
> >
> > Thanks,
> > Greta
> >
> > gcc/
> >
> > 2013-04-18  Greta Yorsh  <Greta.Yorsh@arm.com>
> >
> > 	PR target/56797
> > 	* config/arm/arm.c (load_multiple_sequence): Require SP
> >          as base register for loads if SP is in the register list.
> >
> 
> OK.
> 
> R.
Richard Earnshaw - April 24, 2013, 8:21 a.m.
On 23/04/13 17:37, Greta Yorsh wrote:
> Ok to backport to gcc4.8?
>
> I'm attaching an updated version - just fixed a spelling error in the
> comment.
>
> Thanks,
> Greta
>
> gcc/ChangeLog
>
> 	PR target/56797
> 	* config/arm/arm.c (load_multiple_sequence): Require SP
>          as base register for loads if SP is in the register list.
>

OK.

R.

>> -----Original Message-----
>> From: Richard Earnshaw
>> Sent: 19 April 2013 12:34
>> To: Greta Yorsh
>> Cc: GCC Patches; raj.khem@gmail.com; Ramana Radhakrishnan
>> Subject: Re: [PATCH, ARM] Fix PR56797
>>
>> On 19/04/13 10:34, Greta Yorsh wrote:
>>> Fix PR56797
>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56797
>>>
>>> The problem is that peephole optimizer thinks it can generate an ldm,
>> but
>>> the pattern for ldm no longer matches, because after r188738 it
>> requires
>>> that if one of the destination registers is SP then the base register
>> must
>>> be SP, and it's not SP in the test case.
>>>
>>> The test case fails on armv5t but doesn't fail on armv6t2 or armv7-a
>> because
>>> peephole doesn't trigger there (because there is a different epilogue
>>> sequence). It looks like a latent problem for other architecture or
>> CPUs.
>>>
>>> This patch adds this condition to the peephole optimizer.
>>>
>>> No regression on qemu for arm-none-eabi and fixes the test reported
>> in the
>>> PR. I couldn't minimize the test sufficiently to include it in the
>>> testsuite.
>>>
>>> Ok for trunk?
>>>
>>> Thanks,
>>> Greta
>>>
>>> gcc/
>>>
>>> 2013-04-18  Greta Yorsh  <Greta.Yorsh@arm.com>
>>>
>>> 	PR target/56797
>>> 	* config/arm/arm.c (load_multiple_sequence): Require SP
>>>           as base register for loads if SP is in the register list.
>>>
>>
>> OK.
>>
>> R.
>
>
> pr56797-ldm-peep-sp.patch.txt
>
>
> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> index d00849c..60fef78 100644
> --- a/gcc/config/arm/arm.c
> +++ b/gcc/config/arm/arm.c
> @@ -10347,6 +10347,13 @@ load_multiple_sequence (rtx *operands, int nops, int *regs, int *saved_order,
>   	      || (i != nops - 1 && unsorted_regs[i] == base_reg))
>   	    return 0;
>
> +          /* Don't allow SP to be loaded unless it is also the base
> +             register.  It guarantees that SP is reset correctly when
> +             an LDM instruction is interrupted.  Otherwise, we might
> +             end up with a corrupt stack.  */
> +          if (unsorted_regs[i] == SP_REGNUM && base_reg != SP_REGNUM)
> +            return 0;
> +
>   	  unsorted_offsets[i] = INTVAL (offset);
>   	  if (i == 0 || unsorted_offsets[i] < unsorted_offsets[order[0]])
>   	    order[0] = i;
>

Patch

diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index d00849c..60fef78 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -10347,6 +10347,13 @@  load_multiple_sequence (rtx *operands, int nops, int *regs, int *saved_order,
 	      || (i != nops - 1 && unsorted_regs[i] == base_reg))
 	    return 0;
 
+          /* Don't allow SP to be loaded unless it is also the base
+             register.  It guarantees that SP is reset correctly when
+             an LDM instruction is interrupted.  Otherwise, we might
+             end up with a corrupt stack.  */
+          if (unsorted_regs[i] == SP_REGNUM && base_reg != SP_REGNUM)
+            return 0;
+
 	  unsorted_offsets[i] = INTVAL (offset);
 	  if (i == 0 || unsorted_offsets[i] < unsorted_offsets[order[0]])
 	    order[0] = i;