| Message ID | 20151005090058.GA20023@jaguar.corp.atmel.com |
|---|---|
| State | New |
| Headers | show |
Ping! Regards Senthil On Mon, Oct 05, 2015 at 02:30:58PM +0530, Senthil Kumar Selvaraj wrote: > Hi, > > As part of support for io and io_low attributes, the upper bound of > the range check for low IO and IO addresses was changed from hardcoded > values to hardcoded_range_end + 1 - GET_MODE_SIZE(mode). > > GCC passes VOID as the mode from genrecog, and GET_MODE_SIZE returns > 0, resulting in the range getting incorrectly extended by a byte. > > Not sure why it was done, as the mode of the operand shouldn't really > matter when computing the upper bound. In any case, the insns that use > the predicate already have a mem:QI wrapping it, and all the bit > addressable instructions operate on a single IO register only. > > This patch reverts the check back to a hardcoded value, and adds a > test to prevent regressions. > > No new regression failures. If ok, could someone commit please? I > don't have commit access. > > > Regards > Senthil > > gcc/ChangeLog > > 2015-10-05 Senthil Kumar Selvaraj <senthil_kumar.selvaraj@atmel.com> > > PR target/67839 > * config/avr/predicates.md (low_io_address_operand): Don't > consider MODE when computing upper bound. > (io_address_operand): Likewise. > > > gcc/testsuite/ChangeLog > > 2015-10-05 Senthil Kumar Selvaraj <senthil_kumar.selvaraj@atmel.com> > > PR target/67839 > * gcc.target/avr/pr67839.c: New test. > > > > diff --git gcc/config/avr/predicates.md gcc/config/avr/predicates.md > index 2d12bc6..622bc0b 100644 > --- gcc/config/avr/predicates.md > +++ gcc/config/avr/predicates.md > @@ -46,7 +46,7 @@ > (define_special_predicate "low_io_address_operand" > (ior (and (match_code "const_int") > (match_test "IN_RANGE (INTVAL (op) - avr_arch->sfr_offset, > - 0, 0x20 - GET_MODE_SIZE (mode))")) > + 0, 0x1F)")) > (and (match_code "symbol_ref") > (match_test "SYMBOL_REF_FLAGS (op) & SYMBOL_FLAG_IO_LOW")))) > > @@ -60,7 +60,7 @@ > (define_special_predicate "io_address_operand" > (ior (and (match_code "const_int") > (match_test "IN_RANGE (INTVAL (op) - avr_arch->sfr_offset, > - 0, 0x40 - GET_MODE_SIZE (mode))")) > + 0, 0x3F)")) > (and (match_code "symbol_ref") > (match_test "SYMBOL_REF_FLAGS (op) & SYMBOL_FLAG_IO")))) > > diff --git gcc/testsuite/gcc.target/avr/pr67839.c gcc/testsuite/gcc.target/avr/pr67839.c > new file mode 100644 > index 0000000..604ab4b > --- /dev/null > +++ gcc/testsuite/gcc.target/avr/pr67839.c > @@ -0,0 +1,29 @@ > +/* { dg-do compile } */ > +/* { dg-options "-Os" } */ > +/* { dg-final { scan-assembler "sbi 0x1f,0" } } */ > +/* { dg-final { scan-assembler "cbi 0x1f,0" } } */ > +/* { dg-final { scan-assembler-not "sbi 0x20,0" } } */ > +/* { dg-final { scan-assembler-not "cbi 0x20,0" } } */ > +/* { dg-final { scan-assembler "in r\\d+,__SREG__" } } */ > +/* { dg-final { scan-assembler "out __SREG__,r\\d+" } } */ > +/* { dg-final { scan-assembler-not "in r\\d+,0x40" } } */ > +/* { dg-final { scan-assembler-not "out 0x40, r\\d+" } } */ > + > +/* This testcase verifies that SBI/CBI/SBIS/SBIC > + and IN/OUT instructions are not generated for > + an IO addresses outside the valid range. > +*/ > +#define IO_ADDR(x) (*((volatile char *)x + __AVR_SFR_OFFSET__)) > +int main () > +{ > + IO_ADDR(0x1f) |= 1; > + IO_ADDR(0x1f) &= 0xFE; > + > + IO_ADDR(0x20) |= 1; > + IO_ADDR(0x20) &= 0xFE; > + > + IO_ADDR(0x3f) = IO_ADDR(0x3f); > + > + IO_ADDR(0x40) = IO_ADDR(0x40); > + return 0; > +}
Ping! Regards Senthil On Fri, Oct 16, 2015 at 03:17:17PM +0530, Senthil Kumar Selvaraj wrote: > Ping! > > Regards > Senthil > > On Mon, Oct 05, 2015 at 02:30:58PM +0530, Senthil Kumar Selvaraj wrote: > > Hi, > > > > As part of support for io and io_low attributes, the upper bound of > > the range check for low IO and IO addresses was changed from hardcoded > > values to hardcoded_range_end + 1 - GET_MODE_SIZE(mode). > > > > GCC passes VOID as the mode from genrecog, and GET_MODE_SIZE returns > > 0, resulting in the range getting incorrectly extended by a byte. > > > > Not sure why it was done, as the mode of the operand shouldn't really > > matter when computing the upper bound. In any case, the insns that use > > the predicate already have a mem:QI wrapping it, and all the bit > > addressable instructions operate on a single IO register only. > > > > This patch reverts the check back to a hardcoded value, and adds a > > test to prevent regressions. > > > > No new regression failures. If ok, could someone commit please? I > > don't have commit access. > > > > > > Regards > > Senthil > > > > gcc/ChangeLog > > > > 2015-10-05 Senthil Kumar Selvaraj <senthil_kumar.selvaraj@atmel.com> > > > > PR target/67839 > > * config/avr/predicates.md (low_io_address_operand): Don't > > consider MODE when computing upper bound. > > (io_address_operand): Likewise. > > > > > > gcc/testsuite/ChangeLog > > > > 2015-10-05 Senthil Kumar Selvaraj <senthil_kumar.selvaraj@atmel.com> > > > > PR target/67839 > > * gcc.target/avr/pr67839.c: New test. > > > > > > > > diff --git gcc/config/avr/predicates.md gcc/config/avr/predicates.md > > index 2d12bc6..622bc0b 100644 > > --- gcc/config/avr/predicates.md > > +++ gcc/config/avr/predicates.md > > @@ -46,7 +46,7 @@ > > (define_special_predicate "low_io_address_operand" > > (ior (and (match_code "const_int") > > (match_test "IN_RANGE (INTVAL (op) - avr_arch->sfr_offset, > > - 0, 0x20 - GET_MODE_SIZE (mode))")) > > + 0, 0x1F)")) > > (and (match_code "symbol_ref") > > (match_test "SYMBOL_REF_FLAGS (op) & SYMBOL_FLAG_IO_LOW")))) > > > > @@ -60,7 +60,7 @@ > > (define_special_predicate "io_address_operand" > > (ior (and (match_code "const_int") > > (match_test "IN_RANGE (INTVAL (op) - avr_arch->sfr_offset, > > - 0, 0x40 - GET_MODE_SIZE (mode))")) > > + 0, 0x3F)")) > > (and (match_code "symbol_ref") > > (match_test "SYMBOL_REF_FLAGS (op) & SYMBOL_FLAG_IO")))) > > > > diff --git gcc/testsuite/gcc.target/avr/pr67839.c gcc/testsuite/gcc.target/avr/pr67839.c > > new file mode 100644 > > index 0000000..604ab4b > > --- /dev/null > > +++ gcc/testsuite/gcc.target/avr/pr67839.c > > @@ -0,0 +1,29 @@ > > +/* { dg-do compile } */ > > +/* { dg-options "-Os" } */ > > +/* { dg-final { scan-assembler "sbi 0x1f,0" } } */ > > +/* { dg-final { scan-assembler "cbi 0x1f,0" } } */ > > +/* { dg-final { scan-assembler-not "sbi 0x20,0" } } */ > > +/* { dg-final { scan-assembler-not "cbi 0x20,0" } } */ > > +/* { dg-final { scan-assembler "in r\\d+,__SREG__" } } */ > > +/* { dg-final { scan-assembler "out __SREG__,r\\d+" } } */ > > +/* { dg-final { scan-assembler-not "in r\\d+,0x40" } } */ > > +/* { dg-final { scan-assembler-not "out 0x40, r\\d+" } } */ > > + > > +/* This testcase verifies that SBI/CBI/SBIS/SBIC > > + and IN/OUT instructions are not generated for > > + an IO addresses outside the valid range. > > +*/ > > +#define IO_ADDR(x) (*((volatile char *)x + __AVR_SFR_OFFSET__)) > > +int main () > > +{ > > + IO_ADDR(0x1f) |= 1; > > + IO_ADDR(0x1f) &= 0xFE; > > + > > + IO_ADDR(0x20) |= 1; > > + IO_ADDR(0x20) &= 0xFE; > > + > > + IO_ADDR(0x3f) = IO_ADDR(0x3f); > > + > > + IO_ADDR(0x40) = IO_ADDR(0x40); > > + return 0; > > +}
2015-10-28 9:28 GMT+03:00 Senthil Kumar Selvaraj <senthil_kumar.selvaraj@atmel.com>: > > Ping! > > Regards > Senthil > On Fri, Oct 16, 2015 at 03:17:17PM +0530, Senthil Kumar Selvaraj wrote: > > Ping! > > > > Regards > > Senthil > > > > On Mon, Oct 05, 2015 at 02:30:58PM +0530, Senthil Kumar Selvaraj wrote: > > > Hi, > > > > > > As part of support for io and io_low attributes, the upper bound of > > > the range check for low IO and IO addresses was changed from hardcoded > > > values to hardcoded_range_end + 1 - GET_MODE_SIZE(mode). > > > > > > GCC passes VOID as the mode from genrecog, and GET_MODE_SIZE returns > > > 0, resulting in the range getting incorrectly extended by a byte. > > > > > > Not sure why it was done, as the mode of the operand shouldn't really > > > matter when computing the upper bound. In any case, the insns that use > > > the predicate already have a mem:QI wrapping it, and all the bit > > > addressable instructions operate on a single IO register only. > > > > > > This patch reverts the check back to a hardcoded value, and adds a > > > test to prevent regressions. > > > > > > No new regression failures. If ok, could someone commit please? I > > > don't have commit access. > > > > > > > > > Regards > > > Senthil > > > > > > gcc/ChangeLog > > > > > > 2015-10-05 Senthil Kumar Selvaraj <senthil_kumar.selvaraj@atmel.com> > > > > > > PR target/67839 > > > * config/avr/predicates.md (low_io_address_operand): Don't > > > consider MODE when computing upper bound. > > > (io_address_operand): Likewise. > > > > > > > > > gcc/testsuite/ChangeLog > > > > > > 2015-10-05 Senthil Kumar Selvaraj <senthil_kumar.selvaraj@atmel.com> > > > > > > PR target/67839 > > > * gcc.target/avr/pr67839.c: New test. > > > Committed. Denis
diff --git gcc/config/avr/predicates.md gcc/config/avr/predicates.md index 2d12bc6..622bc0b 100644 --- gcc/config/avr/predicates.md +++ gcc/config/avr/predicates.md @@ -46,7 +46,7 @@ (define_special_predicate "low_io_address_operand" (ior (and (match_code "const_int") (match_test "IN_RANGE (INTVAL (op) - avr_arch->sfr_offset, - 0, 0x20 - GET_MODE_SIZE (mode))")) + 0, 0x1F)")) (and (match_code "symbol_ref") (match_test "SYMBOL_REF_FLAGS (op) & SYMBOL_FLAG_IO_LOW")))) @@ -60,7 +60,7 @@ (define_special_predicate "io_address_operand" (ior (and (match_code "const_int") (match_test "IN_RANGE (INTVAL (op) - avr_arch->sfr_offset, - 0, 0x40 - GET_MODE_SIZE (mode))")) + 0, 0x3F)")) (and (match_code "symbol_ref") (match_test "SYMBOL_REF_FLAGS (op) & SYMBOL_FLAG_IO")))) diff --git gcc/testsuite/gcc.target/avr/pr67839.c gcc/testsuite/gcc.target/avr/pr67839.c new file mode 100644 index 0000000..604ab4b --- /dev/null +++ gcc/testsuite/gcc.target/avr/pr67839.c @@ -0,0 +1,29 @@ +/* { dg-do compile } */ +/* { dg-options "-Os" } */ +/* { dg-final { scan-assembler "sbi 0x1f,0" } } */ +/* { dg-final { scan-assembler "cbi 0x1f,0" } } */ +/* { dg-final { scan-assembler-not "sbi 0x20,0" } } */ +/* { dg-final { scan-assembler-not "cbi 0x20,0" } } */ +/* { dg-final { scan-assembler "in r\\d+,__SREG__" } } */ +/* { dg-final { scan-assembler "out __SREG__,r\\d+" } } */ +/* { dg-final { scan-assembler-not "in r\\d+,0x40" } } */ +/* { dg-final { scan-assembler-not "out 0x40, r\\d+" } } */ + +/* This testcase verifies that SBI/CBI/SBIS/SBIC + and IN/OUT instructions are not generated for + an IO addresses outside the valid range. +*/ +#define IO_ADDR(x) (*((volatile char *)x + __AVR_SFR_OFFSET__)) +int main () +{ + IO_ADDR(0x1f) |= 1; + IO_ADDR(0x1f) &= 0xFE; + + IO_ADDR(0x20) |= 1; + IO_ADDR(0x20) &= 0xFE; + + IO_ADDR(0x3f) = IO_ADDR(0x3f); + + IO_ADDR(0x40) = IO_ADDR(0x40); + return 0; +}