| Message ID | 4E1610ED.2070108@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
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); > +}
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
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
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.
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
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.
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
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); +}
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 * 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.