[ARM] Fix PR49641

[ARM] Fix PR49641

[Bernd Schmidt]

[-- Attachment #1: Type: text/plain, Size: 447 bytes --]

This corrects an error in store_multiple_operation. We're only
generating the writeback version of the instruction on Thumb-1, so
that's where we must make sure the base register isn't also stored.

The ARMv7 manual is unfortunately not totally clear that this does in
fact produce unpredictable results; it seems to suggest that this is the
case only for the T2 encoding. Older documentation makes it clear.

Tested on arm-eabi{,mthumb}.


Bernd

[-- Attachment #2: pr49641.diff --]
[-- Type: text/plain, Size: 1582 bytes --]

	* config/arm/arm.c (store_multiple_sequence): Avoid cases where
	the base reg is stored iff compiling for Thumb1.

	* gcc.target/arm/pr49641.c: New test.

Index: gcc/config/arm/arm.c
===================================================================
--- gcc/config/arm/arm.c	(revision 175906)
+++ gcc/config/arm/arm.c	(working copy)
@@ -9950,7 +9950,10 @@ store_multiple_sequence (rtx *operands,
 	  /* If it isn't an integer register, then we can't do this.  */
 	  if (unsorted_regs[i] < 0
 	      || (TARGET_THUMB1 && unsorted_regs[i] > LAST_LO_REGNUM)
-	      || (TARGET_THUMB2 && unsorted_regs[i] == base_reg)
+	      /* For Thumb1, we'll generate an instruction with update,
+		 and the effects are unpredictable if the base reg is
+		 stored.  */
+	      || (TARGET_THUMB1 && unsorted_regs[i] == base_reg)
 	      || (TARGET_THUMB2 && unsorted_regs[i] == SP_REGNUM)
 	      || unsorted_regs[i] > 14)
 	    return 0;
Index: gcc/testsuite/gcc.target/arm/pr49641.c
===================================================================
--- gcc/testsuite/gcc.target/arm/pr49641.c	(revision 0)
+++ gcc/testsuite/gcc.target/arm/pr49641.c	(revision 0)
@@ -0,0 +1,18 @@
+/* { dg-do compile } */
+/* { dg-options "-mthumb -O2" } */
+/* { dg-require-effective-target arm_thumb1_ok } */
+/* { dg-final { scan-assembler-not "stmia\[\\t \]*r3!\[^\\n]*r3" } } */
+typedef struct {
+  void *t1, *t2, *t3;
+} z;
+extern volatile int y;
+static inline void foo(z *x) {
+  x->t1 = &x->t2;
+  x->t2 = ((void *)0);
+  x->t3 = &x->t1;
+}
+extern z v;
+void bar (void) {
+   y = 0;
+   foo(&v);
+}

Re: [ARM] Fix PR49641

[Richard Earnshaw]

On 07/07/11 21:02, Bernd Schmidt wrote:
> This corrects an error in store_multiple_operation. We're only
> generating the writeback version of the instruction on Thumb-1, so
> that's where we must make sure the base register isn't also stored.
> 
> The ARMv7 manual is unfortunately not totally clear that this does in
> fact produce unpredictable results; it seems to suggest that this is the
> case only for the T2 encoding. Older documentation makes it clear.
> 
> Tested on arm-eabi{,mthumb}.
> 

I agree that the wording here is unclear, but the pseudo code for the
decode makes the situation clearer, and does reflect what I really
believe to be the case.  Put explicitly:

For LDM:

- Encoding A1: Unpredictable if writeback and base in list (I believe
this is true for all architecture versions, despite what it says in the
current ARM ARM -- at least, my v5 copy certainly says unpredictable)

- Encoding T1: Not unpredictable, but deprecated (for base in list, the
loaded value used and writeback ignored).  Note, however, that in UAL
the ! operator on the base register must not be used if the base
register appears in the list.

- Encoding T2: Unpredictable if writeback and base in list



For STM:

- Encoding T2: Unpredictable if writeback and base in list regardless of
the position.

- Encodings T1 and A1: Unpredictable if writeback and base in list and
not lowest numbered register (note that encoding T1 always has
writeback).  In the case where the base is the first register in the
list, then the original value of base will be stored; deprecated.

This is all quite complicated, I hope I've expressed it correctly... :-)

R.

> 
> Bernd
> 
> 
> pr49641.diff
> 
> 
> 	* config/arm/arm.c (store_multiple_sequence): Avoid cases where
> 	the base reg is stored iff compiling for Thumb1.
> 
> 	* gcc.target/arm/pr49641.c: New test.
> 
> Index: gcc/config/arm/arm.c
> ===================================================================
> --- gcc/config/arm/arm.c	(revision 175906)
> +++ gcc/config/arm/arm.c	(working copy)
> @@ -9950,7 +9950,10 @@ store_multiple_sequence (rtx *operands,
>  	  /* If it isn't an integer register, then we can't do this.  */
>  	  if (unsorted_regs[i] < 0
>  	      || (TARGET_THUMB1 && unsorted_regs[i] > LAST_LO_REGNUM)
> -	      || (TARGET_THUMB2 && unsorted_regs[i] == base_reg)
> +	      /* For Thumb1, we'll generate an instruction with update,
> +		 and the effects are unpredictable if the base reg is
> +		 stored.  */
> +	      || (TARGET_THUMB1 && unsorted_regs[i] == base_reg)
>  	      || (TARGET_THUMB2 && unsorted_regs[i] == SP_REGNUM)
>  	      || unsorted_regs[i] > 14)
>  	    return 0;
> Index: gcc/testsuite/gcc.target/arm/pr49641.c
> ===================================================================
> --- gcc/testsuite/gcc.target/arm/pr49641.c	(revision 0)
> +++ gcc/testsuite/gcc.target/arm/pr49641.c	(revision 0)
> @@ -0,0 +1,18 @@
> +/* { dg-do compile } */
> +/* { dg-options "-mthumb -O2" } */
> +/* { dg-require-effective-target arm_thumb1_ok } */
> +/* { dg-final { scan-assembler-not "stmia\[\\t \]*r3!\[^\\n]*r3" } } */
> +typedef struct {
> +  void *t1, *t2, *t3;
> +} z;
> +extern volatile int y;
> +static inline void foo(z *x) {
> +  x->t1 = &x->t2;
> +  x->t2 = ((void *)0);
> +  x->t3 = &x->t1;
> +}
> +extern z v;
> +void bar (void) {
> +   y = 0;
> +   foo(&v);
> +}

Re: [ARM] Fix PR49641

[Bernd Schmidt]

On 07/13/11 16:01, Richard Earnshaw wrote:
> On 07/07/11 21:02, Bernd Schmidt wrote:
>> This corrects an error in store_multiple_operation. We're only
>> generating the writeback version of the instruction on Thumb-1, so
>> that's where we must make sure the base register isn't also stored.
>>
>> The ARMv7 manual is unfortunately not totally clear that this does in
>> fact produce unpredictable results; it seems to suggest that this is the
>> case only for the T2 encoding. Older documentation makes it clear.
>>
>> Tested on arm-eabi{,mthumb}.
>>
> 
> I agree that the wording here is unclear, but the pseudo code for the
> decode makes the situation clearer, and does reflect what I really
> believe to be the case.  Put explicitly:

[...]

I just remembered this patch. Your reply didn't actually comment on it,
so - ok to install?


bernd

Re: [ARM] Fix PR49641

[Bernd Schmidt]

On 07/13/11 16:03, Richard Earnshaw wrote:
>> 	* config/arm/arm.c (store_multiple_sequence): Avoid cases where
>> 	the base reg is stored iff compiling for Thumb1.
>>
>> 	* gcc.target/arm/pr49641.c: New test.

Ping.  Richard, you replied to the mail but didn't comment on the patch.


Bernd

Re: [ARM] Fix PR49641

[Richard Earnshaw]

On 14/10/11 14:31, Bernd Schmidt wrote:
> On 07/13/11 16:03, Richard Earnshaw wrote:
>>> 	* config/arm/arm.c (store_multiple_sequence): Avoid cases where
>>> 	the base reg is stored iff compiling for Thumb1.
>>>
>>> 	* gcc.target/arm/pr49641.c: New test.
> 
> Ping.  Richard, you replied to the mail but didn't comment on the patch.
> 
> 
> Bernd
> 


Sorry, I thought I'd made it clear that I don't think the compiler
should ever use STM with write-back if the base register is in the
stored list.  We must certainly never do it if the base register is not
the first register in the list as this has always been unpredictable.

BTW, this is not Thumb1 specific, it applies at all times.


So, no the patch is not OK as it stands.

R.

Re: [ARM] Fix PR49641

[Bernd Schmidt]

On 10/17/11 14:54, Richard Earnshaw wrote:
> On 14/10/11 14:31, Bernd Schmidt wrote:
>> On 07/13/11 16:03, Richard Earnshaw wrote:
>>>> 	* config/arm/arm.c (store_multiple_sequence): Avoid cases where
>>>> 	the base reg is stored iff compiling for Thumb1.
>>>>
>>>> 	* gcc.target/arm/pr49641.c: New test.
>>
>> Ping.  Richard, you replied to the mail but didn't comment on the patch.
>>
>>
>> Bernd
>>
> 
> 
> Sorry, I thought I'd made it clear that I don't think the compiler
> should ever use STM with write-back if the base register is in the
> stored list.  We must certainly never do it if the base register is not
> the first register in the list as this has always been unpredictable.
> 
> BTW, this is not Thumb1 specific, it applies at all times.
> 
> 
> So, no the patch is not OK as it stands.

I'm confused. The patch disables the STM if THUMB1 and the base register
is in the stored list. We only ever enable write-back for Thumb1 (see
gen_stm_seq). So, what's the problem?


Bernd

Re: [ARM] Fix PR49641

[Richard Earnshaw]

On 18/10/11 13:19, Bernd Schmidt wrote:
> On 10/17/11 14:54, Richard Earnshaw wrote:
>> On 14/10/11 14:31, Bernd Schmidt wrote:
>>> On 07/13/11 16:03, Richard Earnshaw wrote:
>>>>> 	* config/arm/arm.c (store_multiple_sequence): Avoid cases where
>>>>> 	the base reg is stored iff compiling for Thumb1.
>>>>>
>>>>> 	* gcc.target/arm/pr49641.c: New test.
>>>
>>> Ping.  Richard, you replied to the mail but didn't comment on the patch.
>>>
>>>
>>> Bernd
>>>
>>
>>
>> Sorry, I thought I'd made it clear that I don't think the compiler
>> should ever use STM with write-back if the base register is in the
>> stored list.  We must certainly never do it if the base register is not
>> the first register in the list as this has always been unpredictable.
>>
>> BTW, this is not Thumb1 specific, it applies at all times.
>>
>>
>> So, no the patch is not OK as it stands.
> 
> I'm confused. The patch disables the STM if THUMB1 and the base register
> is in the stored list. We only ever enable write-back for Thumb1 (see
> gen_stm_seq). So, what's the problem?

Well, if that's the case why do we need to test for Thumb1 at all?  And
why do we only enable write-back for Thumb1?  other ISA variants can
also do that (I know that Thumb1 requires write-back, but it's
optionally available for the other ISA flavours).

R.

Re: [ARM] Fix PR49641

[Bernd Schmidt]

On 10/18/11 14:30, Richard Earnshaw wrote:
> Well, if that's the case why do we need to test for Thumb1 at all?  And
> why do we only enable write-back for Thumb1?  other ISA variants can
> also do that (I know that Thumb1 requires write-back, but it's
> optionally available for the other ISA flavours).

We're not trying to generate a writeback sequence with our peepholes.
The problem is that on Thumb, that's the only instruction available, and
we want to make use of it if possible (i.e. register dead afterwards etc.).


Bernd

Re: [ARM] Fix PR49641

[Richard Earnshaw]

On 18/10/11 13:47, Bernd Schmidt wrote:
> On 10/18/11 14:30, Richard Earnshaw wrote:
>> Well, if that's the case why do we need to test for Thumb1 at all?  And
>> why do we only enable write-back for Thumb1?  other ISA variants can
>> also do that (I know that Thumb1 requires write-back, but it's
>> optionally available for the other ISA flavours).
> 
> We're not trying to generate a writeback sequence with our peepholes.
> The problem is that on Thumb, that's the only instruction available, and
> we want to make use of it if possible (i.e. register dead afterwards etc.).
> 
> 
> Bernd
> 


OK, I understand now.  However, I think it's misleading to talk about
thumb1 directly here -- it implies that this doesn't apply in other cases.

A better patch, would be (I think)


--- gcc/config/arm/arm.c	(revision 175906)
+++ gcc/config/arm/arm.c	(working copy)
@@ -9950,7 +9950,9 @@ store_multiple_sequence (rtx *operands,
 	  /* If it isn't an integer register, then we can't do this.  */
 	  if (unsorted_regs[i] < 0
 	      || (TARGET_THUMB1 && unsorted_regs[i] > LAST_LO_REGNUM)
-	      || (TARGET_THUMB2 && unsorted_regs[i] == base_reg)
+	      /* The effects are unpredictable if the base reg is
+		 both updated and stored.  */
+	      || (base_writeback && unsorted_regs[i] == base_reg)
 	      || (TARGET_THUMB2 && unsorted_regs[i] == SP_REGNUM)
 	      || unsorted_regs[i] > 14)
 	    return 0;

and then initialize base_writeback at the entry to the function (I
presume that currently we would just set it to be TARGET_THUMB1),
perhaps with a comment saying that we don't currently support
base_writeback for other ISA variants.

R.

Re: [ARM] Fix PR49641

[Sebastian Huber]

[-- Attachment #1: Type: text/plain, Size: 467 bytes --]

Hello,

what about the attached patch based on the original patch provided by Bernd 
Schmidt with modifications suggested by Richard Earnshaw.

-- 
Sebastian Huber, embedded brains GmbH

Address : Obere Lagerstr. 30, D-82178 Puchheim, Germany
Phone   : +49 89 18 90 80 79-6
Fax     : +49 89 18 90 80 79-9
E-Mail  : sebastian.huber@embedded-brains.de
PGP     : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.

[-- Attachment #2: pr49641.patch --]
[-- Type: text/x-patch, Size: 1928 bytes --]

	* config/arm/arm.c (store_multiple_sequence): Avoid cases where
	the base reg is stored iff compiling for Thumb1.

	* gcc.target/arm/pr49641.c: New test.

diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index fcdb8a1..63b5a8b 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -9812,6 +9812,9 @@ store_multiple_sequence (rtx *operands, int nops, int nops_total,
   rtx base_reg_rtx = NULL;
   int i, stm_case;
 
+  /* Write back of base register is currently only supported for Thumb 1.  */
+  int base_writeback = TARGET_THUMB1;
+
   /* Can only handle up to MAX_LDM_STM_OPS insns at present, though could be
      easily extended if required.  */
   gcc_assert (nops >= 2 && nops <= MAX_LDM_STM_OPS);
@@ -9869,7 +9872,9 @@ store_multiple_sequence (rtx *operands, int nops, int nops_total,
 	  /* If it isn't an integer register, then we can't do this.  */
 	  if (unsorted_regs[i] < 0
 	      || (TARGET_THUMB1 && unsorted_regs[i] > LAST_LO_REGNUM)
-	      || (TARGET_THUMB2 && unsorted_regs[i] == base_reg)
+	      /* The effects are unpredictable if the base register is
+		 both updated and stored.  */
+	      || (base_writeback && unsorted_regs[i] == base_reg)
 	      || (TARGET_THUMB2 && unsorted_regs[i] == SP_REGNUM)
 	      || unsorted_regs[i] > 14)
 	    return 0;
diff --git a/gcc/testsuite/gcc.target/arm/pr49641.c b/gcc/testsuite/gcc.target/arm/pr49641.c
new file mode 100644
index 0000000..7f9b376
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/pr49641.c
@@ -0,0 +1,18 @@
+/* { dg-do compile } */
+/* { dg-options "-mthumb -O2" } */
+/* { dg-require-effective-target arm_thumb1_ok } */
+/* { dg-final { scan-assembler-not "stmia\[\\t \]*r3!\[^\\n]*r3" } } */
+typedef struct {
+  void *t1, *t2, *t3;
+} z;
+extern volatile int y;
+static inline void foo(z *x) {
+  x->t1 = &x->t2;
+  x->t2 = ((void *)0);
+  x->t3 = &x->t1;
+}
+extern z v;
+void bar (void) {
+   y = 0;
+   foo(&v);
+}

Re: [ARM] Fix PR49641

[Richard Earnshaw]

On 24/10/11 14:30, Sebastian Huber wrote:
> Hello,
> 
> what about the attached patch based on the original patch provided by Bernd 
> Schmidt with modifications suggested by Richard Earnshaw.
> 
> 
> 
> pr49641.patch
> 
> 
> 	* config/arm/arm.c (store_multiple_sequence): Avoid cases where
> 	the base reg is stored iff compiling for Thumb1.
> 
> 	* gcc.target/arm/pr49641.c: New test.
> 
OK.

R.

Re: [ARM] Fix PR49641

[Sebastian Huber]

On 10/25/2011 06:56 PM, Richard Earnshaw wrote:
> On 24/10/11 14:30, Sebastian Huber wrote:
>> Hello,
>>
>> what about the attached patch based on the original patch provided by Bernd
>> Schmidt with modifications suggested by Richard Earnshaw.
>>
>>
>>
>> pr49641.patch
>>
>>
>> 	* config/arm/arm.c (store_multiple_sequence): Avoid cases where
>> 	the base reg is stored iff compiling for Thumb1.
>>
>> 	* gcc.target/arm/pr49641.c: New test.
>>
> OK.
>
> R.
>

Would someone mind committing it?  Thanks.

-- 
Sebastian Huber, embedded brains GmbH

Address : Obere Lagerstr. 30, D-82178 Puchheim, Germany
Phone   : +49 89 18 90 80 79-6
Fax     : +49 89 18 90 80 79-9
E-Mail  : sebastian.huber@embedded-brains.de
PGP     : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.

Re: [ARM] Fix PR49641

[Sebastian Huber]

On 10/31/2011 11:39 AM, Sebastian Huber wrote:
> On 10/25/2011 06:56 PM, Richard Earnshaw wrote:
>> On 24/10/11 14:30, Sebastian Huber wrote:
>>> Hello,
>>>
>>> what about the attached patch based on the original patch provided by Bernd
>>> Schmidt with modifications suggested by Richard Earnshaw.
>>>
>>>
>>>
>>> pr49641.patch
>>>
>>>
>>> * config/arm/arm.c (store_multiple_sequence): Avoid cases where
>>> the base reg is stored iff compiling for Thumb1.
>>>
>>> * gcc.target/arm/pr49641.c: New test.
>>>
>> OK.
>>
>> R.
>>
>
> Would someone mind committing it? Thanks.
>

Ping.

-- 
Sebastian Huber, embedded brains GmbH

Address : Obere Lagerstr. 30, D-82178 Puchheim, Germany
Phone   : +49 89 18 90 80 79-6
Fax     : +49 89 18 90 80 79-9
E-Mail  : sebastian.huber@embedded-brains.de
PGP     : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.

Re: [ARM] Fix PR49641

[Sebastian Huber]

On 11/08/2011 09:05 AM, Sebastian Huber wrote:
> On 10/31/2011 11:39 AM, Sebastian Huber wrote:
>> On 10/25/2011 06:56 PM, Richard Earnshaw wrote:
>>> On 24/10/11 14:30, Sebastian Huber wrote:
>>>> Hello,
>>>>
>>>> what about the attached patch based on the original patch provided by Bernd
>>>> Schmidt with modifications suggested by Richard Earnshaw.
>>>>
>>>>
>>>>
>>>> pr49641.patch
>>>>
>>>>
>>>> * config/arm/arm.c (store_multiple_sequence): Avoid cases where
>>>> the base reg is stored iff compiling for Thumb1.
>>>>
>>>> * gcc.target/arm/pr49641.c: New test.
>>>>
>>> OK.
>>>
>>> R.
>>>
>>
>> Would someone mind committing it? Thanks.
>>
>
> Ping.
>

What needs to be done to get this committed?  Here are the test results for a 
recent GCC 4.6 snapshot with this patch:

http://gcc.gnu.org/ml/gcc-testresults/2011-11/msg01619.html

-- 
Sebastian Huber, embedded brains GmbH

Address : Obere Lagerstr. 30, D-82178 Puchheim, Germany
Phone   : +49 89 18 90 80 79-6
Fax     : +49 89 18 90 80 79-9
E-Mail  : sebastian.huber@embedded-brains.de
PGP     : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.

Re: [ARM] Fix PR49641

[Richard Earnshaw]

On 16/11/11 08:43, Sebastian Huber wrote:
> On 11/08/2011 09:05 AM, Sebastian Huber wrote:
>> On 10/31/2011 11:39 AM, Sebastian Huber wrote:
>>> On 10/25/2011 06:56 PM, Richard Earnshaw wrote:
>>>> On 24/10/11 14:30, Sebastian Huber wrote:
>>>>> Hello,
>>>>>
>>>>> what about the attached patch based on the original patch provided by Bernd
>>>>> Schmidt with modifications suggested by Richard Earnshaw.
>>>>>
>>>>>
>>>>>
>>>>> pr49641.patch
>>>>>
>>>>>
>>>>> * config/arm/arm.c (store_multiple_sequence): Avoid cases where
>>>>> the base reg is stored iff compiling for Thumb1.
>>>>>
>>>>> * gcc.target/arm/pr49641.c: New test.
>>>>>
>>>> OK.
>>>>
>>>> R.
>>>>
>>>
>>> Would someone mind committing it? Thanks.
>>>
>>
>> Ping.
>>
> 
> What needs to be done to get this committed?  Here are the test results for a 
> recent GCC 4.6 snapshot with this patch:
> 
> http://gcc.gnu.org/ml/gcc-testresults/2011-11/msg01619.html
> 

Sorry for the delay.  Now committed to trunk and 4.6 branch.

R.