| Message ID | 56E01D75.40807@foss.arm.com |
|---|---|
| State | New |
| Headers | show |
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.
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. >
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. >> >
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
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 >
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 >>
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
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")])
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.