[PATCH][ARM] Use proper output modifier for DImode register in store exclusive patterns

[PATCH][ARM] Use proper output modifier for DImode register in store exclusive patterns

[Kyrill Tkachov]

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

Hi all,

I notice that the output code for our store exclusive patterns accesses unallocated memory.
It wants to output an strexd instruction with a pair of consecutive registers corresponding
to a DImode value. For that it creates the SImode top half of the DImode register and puts it
into operands[3]. But the pattern only defines entries only up to operands[2], with no match_dup 3
or like that, so operands[3] should technically be out of bounds.

We already have a mechanism for printing the top half of a DImode register, that's the 'H' output modifier.
So this patch changes those patterns to use that, eliminating the out of bounds access and making
the code a bit simpler as well.

Bootstrapped and tested on arm-none-linux-gnueabihf.

Ok for trunk?

Thanks,
Kyrill

2016-03-09  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     * config/arm/sync.md (arm_store_exclusive<mode>):
     Use 'H' output modifier on operands[2] rather than creating a new
     entry in out-of-bounds memory of the operands array.
     (arm_store_release_exclusivedi): Likewise.

[-- Attachment #2: arm-store-modifier.patch --]
[-- Type: text/x-patch, Size: 1604 bytes --]

diff --git a/gcc/config/arm/sync.md b/gcc/config/arm/sync.md
index 6dd2dc396210bc45374d13e1a20f124cc490b630..8158f53025400045569533a1e8c6583025d490c8 100644
--- a/gcc/config/arm/sync.md
+++ b/gcc/config/arm/sync.md
@@ -422,14 +422,13 @@ (define_insn "arm_store_exclusive<mode>"
   {
     if (<MODE>mode == DImode)
       {
-	rtx value = operands[2];
 	/* The restrictions on target registers in ARM mode are that the two
 	   registers are consecutive and the first one is even; Thumb is
 	   actually more flexible, but DI should give us this anyway.
-	   Note that the 1st register always gets the lowest word in memory.  */
-	gcc_assert ((REGNO (value) & 1) == 0 || TARGET_THUMB2);
-	operands[3] = gen_rtx_REG (SImode, REGNO (value) + 1);
-	return "strexd%?\t%0, %2, %3, %C1";
+	   Note that the 1st register always gets the
+	   lowest word in memory.  */
+	gcc_assert ((REGNO (operands[2]) & 1) == 0 || TARGET_THUMB2);
+	return "strexd%?\t%0, %2, %H2, %C1";
       }
     return "strex<sync_sfx>%?\t%0, %2, %C1";
   }
@@ -445,11 +444,9 @@ (define_insn "arm_store_release_exclusivedi"
 	  VUNSPEC_SLX))]
   "TARGET_HAVE_LDACQ && ARM_DOUBLEWORD_ALIGN"
   {
-    rtx value = operands[2];
     /* See comment in arm_store_exclusive<mode> above.  */
-    gcc_assert ((REGNO (value) & 1) == 0 || TARGET_THUMB2);
-    operands[3] = gen_rtx_REG (SImode, REGNO (value) + 1);
-    return "stlexd%?\t%0, %2, %3, %C1";
+    gcc_assert ((REGNO (operands[2]) & 1) == 0 || TARGET_THUMB2);
+    return "stlexd%?\t%0, %2, %H2, %C1";
   }
   [(set_attr "predicable" "yes")
    (set_attr "predicable_short_it" "no")])

Re: [PATCH][ARM] Use proper output modifier for DImode register in store exclusive patterns

[Kyrill Tkachov]

Hi all,

This seems to have fallen through the cracks.
Ping.
https://gcc.gnu.org/ml/gcc-patches/2016-03/msg00558.html

Thanks,
Kyrill

On 09/03/16 12:56, Kyrill Tkachov wrote:
> Hi all,
>
> I notice that the output code for our store exclusive patterns accesses unallocated memory.
> It wants to output an strexd instruction with a pair of consecutive registers corresponding
> to a DImode value. For that it creates the SImode top half of the DImode register and puts it
> into operands[3]. But the pattern only defines entries only up to operands[2], with no match_dup 3
> or like that, so operands[3] should technically be out of bounds.
>
> We already have a mechanism for printing the top half of a DImode register, that's the 'H' output modifier.
> So this patch changes those patterns to use that, eliminating the out of bounds access and making
> the code a bit simpler as well.
>
> Bootstrapped and tested on arm-none-linux-gnueabihf.
>
> Ok for trunk?
>
> Thanks,
> Kyrill
>
> 2016-03-09  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>
>     * config/arm/sync.md (arm_store_exclusive<mode>):
>     Use 'H' output modifier on operands[2] rather than creating a new
>     entry in out-of-bounds memory of the operands array.
>     (arm_store_release_exclusivedi): Likewise.

Re: [PATCH][ARM] Use proper output modifier for DImode register in store exclusive patterns

[Kyrill Tkachov]

Ping.

Thanks,
Kyrill

On 09/05/16 12:13, Kyrill Tkachov wrote:
> Hi all,
>
> This seems to have fallen through the cracks.
> Ping.
> https://gcc.gnu.org/ml/gcc-patches/2016-03/msg00558.html
>
> Thanks,
> Kyrill
>
> On 09/03/16 12:56, Kyrill Tkachov wrote:
>> Hi all,
>>
>> I notice that the output code for our store exclusive patterns accesses unallocated memory.
>> It wants to output an strexd instruction with a pair of consecutive registers corresponding
>> to a DImode value. For that it creates the SImode top half of the DImode register and puts it
>> into operands[3]. But the pattern only defines entries only up to operands[2], with no match_dup 3
>> or like that, so operands[3] should technically be out of bounds.
>>
>> We already have a mechanism for printing the top half of a DImode register, that's the 'H' output modifier.
>> So this patch changes those patterns to use that, eliminating the out of bounds access and making
>> the code a bit simpler as well.
>>
>> Bootstrapped and tested on arm-none-linux-gnueabihf.
>>
>> Ok for trunk?
>>
>> Thanks,
>> Kyrill
>>
>> 2016-03-09  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>
>>     * config/arm/sync.md (arm_store_exclusive<mode>):
>>     Use 'H' output modifier on operands[2] rather than creating a new
>>     entry in out-of-bounds memory of the operands array.
>>     (arm_store_release_exclusivedi): Likewise.
>

Re: [PATCH][ARM] Use proper output modifier for DImode register in store exclusive patterns

[Kyrill Tkachov]

Ping.

Thanks,
Kyrill

On 19/05/16 14:27, Kyrill Tkachov wrote:
> Ping.
>
> Thanks,
> Kyrill
>
> On 09/05/16 12:13, Kyrill Tkachov wrote:
>> Hi all,
>>
>> This seems to have fallen through the cracks.
>> Ping.
>> https://gcc.gnu.org/ml/gcc-patches/2016-03/msg00558.html
>>
>> Thanks,
>> Kyrill
>>
>> On 09/03/16 12:56, Kyrill Tkachov wrote:
>>> Hi all,
>>>
>>> I notice that the output code for our store exclusive patterns accesses unallocated memory.
>>> It wants to output an strexd instruction with a pair of consecutive registers corresponding
>>> to a DImode value. For that it creates the SImode top half of the DImode register and puts it
>>> into operands[3]. But the pattern only defines entries only up to operands[2], with no match_dup 3
>>> or like that, so operands[3] should technically be out of bounds.
>>>
>>> We already have a mechanism for printing the top half of a DImode register, that's the 'H' output modifier.
>>> So this patch changes those patterns to use that, eliminating the out of bounds access and making
>>> the code a bit simpler as well.
>>>
>>> Bootstrapped and tested on arm-none-linux-gnueabihf.
>>>
>>> Ok for trunk?
>>>
>>> Thanks,
>>> Kyrill
>>>
>>> 2016-03-09  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>>
>>>     * config/arm/sync.md (arm_store_exclusive<mode>):
>>>     Use 'H' output modifier on operands[2] rather than creating a new
>>>     entry in out-of-bounds memory of the operands array.
>>>     (arm_store_release_exclusivedi): Likewise.
>>
>

Re: [PATCH][ARM] Use proper output modifier for DImode register in store exclusive patterns

[Ramana Radhakrishnan]

On 09/03/16 12:56, Kyrill Tkachov wrote:
> Hi all,
> 
> I notice that the output code for our store exclusive patterns accesses unallocated memory.
> It wants to output an strexd instruction with a pair of consecutive registers corresponding
> to a DImode value. For that it creates the SImode top half of the DImode register and puts it
> into operands[3]. But the pattern only defines entries only up to operands[2], with no match_dup 3
> or like that, so operands[3] should technically be out of bounds.
> 
> We already have a mechanism for printing the top half of a DImode register, that's the 'H' output modifier.
> So this patch changes those patterns to use that, eliminating the out of bounds access and making
> the code a bit simpler as well.
> 
> Bootstrapped and tested on arm-none-linux-gnueabihf.
> 
> Ok for trunk?
> 
> Thanks,
> Kyrill
> 
> 2016-03-09  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
> 
>     * config/arm/sync.md (arm_store_exclusive<mode>):
>     Use 'H' output modifier on operands[2] rather than creating a new
>     entry in out-of-bounds memory of the operands array.
>     (arm_store_release_exclusivedi): Likewise.


Ok,

Thanks,
ramana

Re: [PATCH][ARM] Use proper output modifier for DImode register in store exclusive patterns

[Ramana Radhakrishnan]

On 01/06/16 10:02, Ramana Radhakrishnan wrote:
> 
> 
> On 09/03/16 12:56, Kyrill Tkachov wrote:
>> Hi all,
>>
>> I notice that the output code for our store exclusive patterns accesses unallocated memory.
>> It wants to output an strexd instruction with a pair of consecutive registers corresponding
>> to a DImode value. For that it creates the SImode top half of the DImode register and puts it
>> into operands[3]. But the pattern only defines entries only up to operands[2], with no match_dup 3
>> or like that, so operands[3] should technically be out of bounds.
>>
>> We already have a mechanism for printing the top half of a DImode register, that's the 'H' output modifier.
>> So this patch changes those patterns to use that, eliminating the out of bounds access and making
>> the code a bit simpler as well.
>>
>> Bootstrapped and tested on arm-none-linux-gnueabihf.
>>
>> Ok for trunk?
>>
>> Thanks,
>> Kyrill
>>
>> 2016-03-09  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>
>>     * config/arm/sync.md (arm_store_exclusive<mode>):
>>     Use 'H' output modifier on operands[2] rather than creating a new
>>     entry in out-of-bounds memory of the operands array.
>>     (arm_store_release_exclusivedi): Likewise.
> 
> 
> Ok,
> 
Ah hang on - is this safe for big-endian - remind me again why we shouldn't use 'Q' and 'R' here ?

Ramana

> Thanks,
> ramana
> 

Re: [PATCH][ARM] Use proper output modifier for DImode register in store exclusive patterns

[Kyrill Tkachov]

Hi Ramana,

On 01/06/16 10:07, Ramana Radhakrishnan wrote:
>
> On 01/06/16 10:02, Ramana Radhakrishnan wrote:
>>
>> On 09/03/16 12:56, Kyrill Tkachov wrote:
>>> Hi all,
>>>
>>> I notice that the output code for our store exclusive patterns accesses unallocated memory.
>>> It wants to output an strexd instruction with a pair of consecutive registers corresponding
>>> to a DImode value. For that it creates the SImode top half of the DImode register and puts it
>>> into operands[3]. But the pattern only defines entries only up to operands[2], with no match_dup 3
>>> or like that, so operands[3] should technically be out of bounds.
>>>
>>> We already have a mechanism for printing the top half of a DImode register, that's the 'H' output modifier.
>>> So this patch changes those patterns to use that, eliminating the out of bounds access and making
>>> the code a bit simpler as well.
>>>
>>> Bootstrapped and tested on arm-none-linux-gnueabihf.
>>>
>>> Ok for trunk?
>>>
>>> Thanks,
>>> Kyrill
>>>
>>> 2016-03-09  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>>
>>>      * config/arm/sync.md (arm_store_exclusive<mode>):
>>>      Use 'H' output modifier on operands[2] rather than creating a new
>>>      entry in out-of-bounds memory of the operands array.
>>>      (arm_store_release_exclusivedi): Likewise.
>>
>> Ok,
>>
> Ah hang on - is this safe for big-endian - remind me again why we shouldn't use 'Q' and 'R' here ?

Thanks for looking at this.
The existing code does an STRD of REG and REG+1 without any concern for endianness.
This patch simplifies that logic (since %H is just REG+1), so there's no change in behaviour.
The question is whether the current behaviour is wrong for big-endian.

I've looked at sections A8.8.214 and A3.3 in the Architecture Reference Manual (I'm using Rev C.c)
and my understanding is that doubleword quantities should not have their words swapped when storing
and that STREXD (and STRD) stores REG at [address] and REG+1 at [address+4] regardless of endianness.
So I think it's correct here to always store REG followed by REG+1.
I note that this is also what we do when storing a normal DImode value with STRD.
In output_move_double we use an STRD of the lower numbered register followed by the next one without
any checks on endianness.

Looking at the uses of Q and R and their comment in arm_print_operand they are only used when performing
arithmetic operations on the DImode values where choosing the most significant or the least significant
word matters.

Hope this makes sense,
Kyrill

> Ramana
>
>> Thanks,
>> ramana
>>

Re: [PATCH][ARM] Use proper output modifier for DImode register in store exclusive patterns

[Ramana Radhakrishnan]

On 01/06/16 11:18, Kyrill Tkachov wrote:
> Hi Ramana,
> 
> On 01/06/16 10:07, Ramana Radhakrishnan wrote:
>>
>> On 01/06/16 10:02, Ramana Radhakrishnan wrote:
>>>
>>> On 09/03/16 12:56, Kyrill Tkachov wrote:
>>>> Hi all,
>>>>
>>>> I notice that the output code for our store exclusive patterns accesses unallocated memory.
>>>> It wants to output an strexd instruction with a pair of consecutive registers corresponding
>>>> to a DImode value. For that it creates the SImode top half of the DImode register and puts it
>>>> into operands[3]. But the pattern only defines entries only up to operands[2], with no match_dup 3
>>>> or like that, so operands[3] should technically be out of bounds.
>>>>
>>>> We already have a mechanism for printing the top half of a DImode register, that's the 'H' output modifier.
>>>> So this patch changes those patterns to use that, eliminating the out of bounds access and making
>>>> the code a bit simpler as well.
>>>>
>>>> Bootstrapped and tested on arm-none-linux-gnueabihf.
>>>>
>>>> Ok for trunk?
>>>>
>>>> Thanks,
>>>> Kyrill
>>>>
>>>> 2016-03-09  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>>>
>>>>      * config/arm/sync.md (arm_store_exclusive<mode>):
>>>>      Use 'H' output modifier on operands[2] rather than creating a new
>>>>      entry in out-of-bounds memory of the operands array.
>>>>      (arm_store_release_exclusivedi): Likewise.
>>>
>>> Ok,
>>>
>> Ah hang on - is this safe for big-endian - remind me again why we shouldn't use 'Q' and 'R' here ?
> 
> Thanks for looking at this.
> The existing code does an STRD of REG and REG+1 without any concern for endianness.
> This patch simplifies that logic (since %H is just REG+1), so there's no change in behaviour.

Yep makes sense - thanks, not enough coffee this morning.

The patch is ok.

Ramana