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  

  * config/arm/sync.md (arm_store_exclusive):
  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

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  

 * config/arm/sync.md (arm_store_exclusive):
 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 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  
>>
>> * config/arm/sync.md (arm_store_exclusive):
>> 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

[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  
> 
> * config/arm/sync.md (arm_store_exclusive):
> 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

[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  

* config/arm/sync.md (arm_store_exclusive):
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  

* config/arm/sync.md (arm_store_exclusive):
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]

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  

* config/arm/sync.md (arm_store_exclusive):
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.

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

[Kyrill Tkachov]

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  

* config/arm/sync.md (arm_store_exclusive):
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.
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"
   {
 if (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%?\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 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")])