Re: [patch,avr] Add support for devices with flash accessible by LD.
On Friday 09 June 2017 03:59 PM, Georg-Johann Lay wrote: Hi, This patch adds support for devices that can access flash memory by LD* instructions, hence there is no need to put .rodata in RAM. The default linker script for the new multilib versions already supports this feature, it's similar to avrtiny, cf. https://sourceware.org/PR21472 This patch does the following: * Add multilib variants avrxmega3 and avrxmega3/short-calls. * Add new option -mshort-calls for multilib selection between devices with <= 8KiB flash and > 8KiB flash. * Add specs handling for -mshort-calls: The compiler knows if this option is needed or not appropriate (similar to -msp8). * Add new ISA feature AVR_ISA_RCALL for multilib selection via -mshort-calls. * Add a new row to architecture description that contains the start address of flash memory in the RAM address range. (The actual value is not needed). * For devices with flash in RAM space, don't let .rodata objects trigger need for __do_copy_data. * Add some devices. * Add configure test for Binutils PR21472. * Adjust documentation etc. Even the smaller devices with flash <= 8KiB support JMP+CALL, but we get better code by assuming RJMP+RCALL: Jump tables are more efficient and insn length computation is more exact (CALL -> RCALL relaxation would need -mrelax and insn size would still be 4 bytes). Moreover, avr-libc uses __AVR_HAVE_JMP_CALL__ to determine vector entry size, and libgcc uses that macro for flash size estimation. Hi Johann, + AVR_ISA_RCALL = 0x10 /* Use RJMP / RCALL even though RJMP / RCALL +are available (-mshort-calls). */ I think you meant "even though JMP/ CALL are avariable". +AVR_MCU ("attiny817",ARCH_AVRXMEGA3, AVR_ISA_RCALL, "__AVR_ATtiny817__", 0x3e00, 0x0, 0x2000) +AVR_MCU ("attiny1616", ARCH_AVRXMEGA3, AVR_ISA_NONE, "__AVR_ATtiny1616__", 0x3800, 0x0, 0x4000) Add attiny1614 device. AVR_MCU ("attiny1614", ARCH_AVRXMEGA3, AVR_ISA_NONE, "__AVR_ATtiny1614__", 0x3800, 0x0, 0x4000) Regards, Pitchumani
Re: Ping: Re: [patch, avr] Add flash size to device info and make wrap around default
On Tuesday 29 November 2016 10:06 PM, Denis Chertykov wrote: 2016-11-28 10:17 GMT+03:00 Pitchumani Sivanupandi : On Saturday 26 November 2016 12:11 AM, Denis Chertykov wrote: I'm sorry for delay. I have a problem with the patch: (Stripping trailing CRs from patch; use --binary to disable.) patching file avr-arch.h (Stripping trailing CRs from patch; use --binary to disable.) patching file avr-devices.c (Stripping trailing CRs from patch; use --binary to disable.) patching file avr-mcus.def Hunk #1 FAILED at 62. 1 out of 1 hunk FAILED -- saving rejects to file avr-mcus.def.rej (Stripping trailing CRs from patch; use --binary to disable.) patching file gen-avr-mmcu-specs.c Hunk #1 succeeded at 215 (offset 5 lines). (Stripping trailing CRs from patch; use --binary to disable.) patching file specs.h Hunk #1 succeeded at 58 (offset 1 line). Hunk #2 succeeded at 66 (offset 1 line). There are changes in avr-mcus.def after this patch is submitted. Now, I have incorporated the changes and attached the resolved patch. Regards, Pitchumani gcc/ChangeLog 2016-11-09 Pitchumani Sivanupandi * config/avr/avr-arch.h (avr_mcu_t): Add flash_size member. * config/avr/avr-devices.c(avr_mcu_types): Add flash size info. * config/avr/avr-mcu.def: Likewise. * config/avr/gen-avr-mmcu-specs.c (print_mcu): Remove hard-coded prefix check to find wrap-around value, instead use MCU flash size. For 8k flash devices, update link_pmem_wrap spec string to add --pmem-wrap-around=8k. * config/avr/specs.h: Remove link_pmem_wrap from LINK_RELAX_SPEC and add to linker specs (LINK_SPEC) directly. Committed. It looks like only avr-mcus.def and ChangeLog are committed. Without the other changes trunk build is broken. Regards, Pitchumani
Re: [RFC, tentative patch] Adjust cost for conversion expression
On Thursday 24 November 2016 04:54 PM, Richard Biener wrote: On Thu, 24 Nov 2016, Pitchumani Sivanupandi wrote: GCC inlines small functions if the code size after expansion is not excedded. For test case (inline.c, avr-gcc -Os -S inline.c) code size become higher if 'func2' is inlined. It happens because the CONVERT_EXPR/ NOP_EXPR are considered as zero cost expression. Few conversions will cost additional instructions. For targets like AVR it will cost considerably as it's register size is just one byte. Attached the tentative patch that changes the CONVERT_EXPR/ NOP_EXPR cost to 1 if the LHS is bigger than RHS and target's word_mode. Is this Ok? Would it be reasonable if cost evaluated as below instead of constant 1? if (LHS PRECISION > RHS PRECISION) cost = LHS_PRECISION / word_mode - 1 else cost = 0 Built GCC for native with bootstrap enabled. No issues. I believe a better check would be tree_nop_conversion_p (). Thus CASE_CONVERT: return tree_nop_conversion_p (type, TREE_TYPE (op0)) ? 0 : 1; note that + rhs_code = gimple_assign_rhs_code (stmt); + if ((rhs_code == NOP_EXPR) || (rhs_code == CONVERT_EXPR)) + { +cost += estimate_operator_cost (rhs_code, weights, + gimple_assign_lhs (stmt), + gimple_assign_rhs1 (stmt)); + } is super-ugly - please simply add the type of the expression as an additional argument (see gimple_expr_type (), but the type of the LHS would do as well). Note that doing this change might require some re-tuning of inliner params, but otherwise it's totally sensible. Thanks. Attached the revised patch. When reg-tested for x86_64 found following failures. FAIL: gcc.dg/uninit-19.c FAIL: gcc.dg/vect/vect-104.c For uninit-19.c, index to dereference float array is converted to long unsigned int and that is not tree_nop_conversion_p. This caused function cost to increase and auto inline is rejected. I think, this may be huge penalty for target like x86_64 which has rich ISA. Any suggestions to avoid hitting such targets? Regards, Pitchumani diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c index 6899d2a..e9f45be 100644 --- a/gcc/tree-inline.c +++ b/gcc/tree-inline.c @@ -3867,19 +3867,19 @@ estimate_move_cost (tree type, bool ARG_UNUSED (speed_p)) static int estimate_operator_cost (enum tree_code code, eni_weights *weights, - tree op1 ATTRIBUTE_UNUSED, tree op2) + tree op1, tree op2, tree op0) { switch (code) { /* These are "free" conversions, or their presumed cost is folded into other operations. */ case RANGE_EXPR: -CASE_CONVERT: case COMPLEX_EXPR: case PAREN_EXPR: case VIEW_CONVERT_EXPR: return 0; - +CASE_CONVERT: + return tree_nop_conversion_p (op0, TREE_TYPE (op1)) ? 0 : 1; /* Assign cost of 1 to usual operations. ??? We may consider mapping RTL costs to this. */ case COND_EXPR: @@ -4068,13 +4068,14 @@ estimate_num_insns (gimple *stmt, eni_weights *weights) gimple_assign_rhs1 (stmt), get_gimple_rhs_class (gimple_assign_rhs_code (stmt)) == GIMPLE_BINARY_RHS - ? gimple_assign_rhs2 (stmt) : NULL); + ? gimple_assign_rhs2 (stmt) : NULL, + gimple_expr_type (stmt)); break; case GIMPLE_COND: cost = 1 + estimate_operator_cost (gimple_cond_code (stmt), weights, gimple_op (stmt, 0), - gimple_op (stmt, 1)); + gimple_op (stmt, 1), gimple_expr_type (stmt)); break; case GIMPLE_SWITCH: @@ -4129,7 +4130,7 @@ estimate_num_insns (gimple *stmt, eni_weights *weights) &dconst2))) return estimate_operator_cost (MULT_EXPR, weights, gimple_call_arg (stmt, 0), - gimple_call_arg (stmt, 0)); + gimple_call_arg (stmt, 0), gimple_expr_type (stmt)); break; default:
Re: Ping: Re: [patch, avr] Add flash size to device info and make wrap around default
On Saturday 26 November 2016 12:11 AM, Denis Chertykov wrote: I'm sorry for delay. I have a problem with the patch: (Stripping trailing CRs from patch; use --binary to disable.) patching file avr-arch.h (Stripping trailing CRs from patch; use --binary to disable.) patching file avr-devices.c (Stripping trailing CRs from patch; use --binary to disable.) patching file avr-mcus.def Hunk #1 FAILED at 62. 1 out of 1 hunk FAILED -- saving rejects to file avr-mcus.def.rej (Stripping trailing CRs from patch; use --binary to disable.) patching file gen-avr-mmcu-specs.c Hunk #1 succeeded at 215 (offset 5 lines). (Stripping trailing CRs from patch; use --binary to disable.) patching file specs.h Hunk #1 succeeded at 58 (offset 1 line). Hunk #2 succeeded at 66 (offset 1 line). There are changes in avr-mcus.def after this patch is submitted. Now, I have incorporated the changes and attached the resolved patch. Regards, Pitchumani gcc/ChangeLog 2016-11-09 Pitchumani Sivanupandi * config/avr/avr-arch.h (avr_mcu_t): Add flash_size member. * config/avr/avr-devices.c(avr_mcu_types): Add flash size info. * config/avr/avr-mcu.def: Likewise. * config/avr/gen-avr-mmcu-specs.c (print_mcu): Remove hard-coded prefix check to find wrap-around value, instead use MCU flash size. For 8k flash devices, update link_pmem_wrap spec string to add --pmem-wrap-around=8k. * config/avr/specs.h: Remove link_pmem_wrap from LINK_RELAX_SPEC and add to linker specs (LINK_SPEC) directly. diff --git a/gcc/config/avr/avr-arch.h b/gcc/config/avr/avr-arch.h index a740a15..e6a2d75 100644 --- a/gcc/config/avr/avr-arch.h +++ b/gcc/config/avr/avr-arch.h @@ -122,6 +122,9 @@ typedef struct /* Number of 64k segments in the flash. */ int n_flash; + + /* Flash size in bytes. */ + int flash_size; } avr_mcu_t; /* AVR device specific features. diff --git a/gcc/config/avr/avr-devices.c b/gcc/config/avr/avr-devices.c index 7d13ba4..cef3b9a 100644 --- a/gcc/config/avr/avr-devices.c +++ b/gcc/config/avr/avr-devices.c @@ -111,12 +111,12 @@ avr_texinfo[] = const avr_mcu_t avr_mcu_types[] = { -#define AVR_MCU(NAME, ARCH, DEV_ATTRIBUTE, MACRO, DATA_SEC, TEXT_SEC, N_FLASH)\ - { NAME, ARCH, DEV_ATTRIBUTE, MACRO, DATA_SEC, TEXT_SEC, N_FLASH }, +#define AVR_MCU(NAME, ARCH, DEV_ATTRIBUTE, MACRO, DATA_SEC, TEXT_SEC, N_FLASH, FLASH_SIZE)\ + { NAME, ARCH, DEV_ATTRIBUTE, MACRO, DATA_SEC, TEXT_SEC, N_FLASH, FLASH_SIZE }, #include "avr-mcus.def" #undef AVR_MCU /* End of list. */ - { NULL, ARCH_UNKNOWN, AVR_ISA_NONE, NULL, 0, 0, 0 } + { NULL, ARCH_UNKNOWN, AVR_ISA_NONE, NULL, 0, 0, 0, 0 } }; diff --git a/gcc/config/avr/avr-mcus.def b/gcc/config/avr/avr-mcus.def index e5b4cda..4008741 100644 --- a/gcc/config/avr/avr-mcus.def +++ b/gcc/config/avr/avr-mcus.def @@ -62,295 +62,298 @@ N_FLASH Number of 64 KiB flash segments, rounded up. The default value for -mn-flash=. + FLASH_SIZEFlash size in bytes. + "avr2" must be first for the "0" default to work as intended. */ /* Classic, <= 8K. */ -AVR_MCU ("avr2", ARCH_AVR2, AVR_ERRATA_SKIP, NULL, 0x0060, 0x0, 6) -AVR_MCU ("at90s2313",ARCH_AVR2, AVR_SHORT_SP, "__AVR_AT90S2313__", 0x0060, 0x0, 1) -AVR_MCU ("at90s2323",ARCH_AVR2, AVR_SHORT_SP, "__AVR_AT90S2323__", 0x0060, 0x0, 1) -AVR_MCU ("at90s2333",ARCH_AVR2, AVR_SHORT_SP, "__AVR_AT90S2333__", 0x0060, 0x0, 1) -AVR_MCU ("at90s2343",ARCH_AVR2, AVR_SHORT_SP, "__AVR_AT90S2343__", 0x0060, 0x0, 1) -AVR_MCU ("attiny22", ARCH_AVR2, AVR_SHORT_SP, "__AVR_ATtiny22__", 0x0060, 0x0, 1) -AVR_MCU ("attiny26", ARCH_AVR2, AVR_SHORT_SP, "__AVR_ATtiny26__", 0x0060, 0x0, 1) -AVR_MCU ("at90s4414",ARCH_AVR2, AVR_ISA_NONE, "__AVR_AT90S4414__", 0x0060, 0x0, 1) -AVR_MCU ("at90s4433",ARCH_AVR2, AVR_SHORT_SP, "__AVR_AT90S4433__", 0x0060, 0x0, 1) -AVR_MCU ("at90s4434",ARCH_AVR2, AVR_ISA_NONE, "__AVR_AT90S4434__", 0x0060, 0x0, 1) -AVR_MCU ("at90s8515",ARCH_AVR2, AVR_ERRATA_SKIP, "__AVR_AT90S8515__", 0x0060, 0x0, 1) -AVR_MCU ("at90c8534",ARCH_AVR2, AVR_ISA_NONE, "__AVR_AT90C8534__", 0x0060, 0x0, 1) -AVR_MCU ("at90s8535",ARCH_AVR2, AVR_ISA_NONE, "__AVR_AT90S8535__", 0x0060, 0x0, 1) +AVR_MCU ("avr2", ARCH_AVR2, AVR_ERRATA_SKIP, NULL, 0x0060, 0x0, 6, 0x2000) + +AVR_MCU ("at90s2313",ARCH_AVR2, AVR_SHORT_SP, "__AVR_AT90S2313__", 0x0060, 0x0, 1, 0x800) +AVR_MCU ("at90s2323",ARCH_AVR2, AVR_S
[RFC, tentative patch] Adjust cost for conversion expression
GCC inlines small functions if the code size after expansion is not excedded. For test case (inline.c, avr-gcc -Os -S inline.c) code size become higher if 'func2' is inlined. It happens because the CONVERT_EXPR/ NOP_EXPR are considered as zero cost expression. Few conversions will cost additional instructions. For targets like AVR it will cost considerably as it's register size is just one byte. Attached the tentative patch that changes the CONVERT_EXPR/ NOP_EXPR cost to 1 if the LHS is bigger than RHS and target's word_mode. Is this Ok? Would it be reasonable if cost evaluated as below instead of constant 1? if (LHS PRECISION > RHS PRECISION) cost = LHS_PRECISION / word_mode - 1 else cost = 0 Built GCC for native with bootstrap enabled. No issues. Regards, Pitchumani typedef unsigned int uint16_t __attribute__((__mode__(__HI__))); typedef unsigned int uint32_t __attribute__ ((__mode__ (__SI__))); char c = 12; volatile long l = 1; volatile uint32_t (*p)(); static uint16_t bar () { return c; } static uint32_t foo () { return ((uint32_t)bar() << 16 | bar()); } __attribute__((noinline)) void func2 (uint32_t(*ptr)()) { p = ptr(); l = foo(); } void main () { func2(foo); } diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c index 6899d2a..dbb305d 100644 --- a/gcc/tree-inline.c +++ b/gcc/tree-inline.c @@ -3867,19 +3867,28 @@ estimate_move_cost (tree type, bool ARG_UNUSED (speed_p)) static int estimate_operator_cost (enum tree_code code, eni_weights *weights, - tree op1 ATTRIBUTE_UNUSED, tree op2) + tree op1, tree op2) { + unsigned int lhs_prec, rhs_prec; switch (code) { /* These are "free" conversions, or their presumed cost is folded into other operations. */ case RANGE_EXPR: -CASE_CONVERT: case COMPLEX_EXPR: case PAREN_EXPR: case VIEW_CONVERT_EXPR: return 0; - +CASE_CONVERT: +{ + if (op1 && op2) { +lhs_prec = element_precision (TREE_TYPE (op1)); +rhs_prec = element_precision (TREE_TYPE (op2)); +if ((lhs_prec > rhs_prec) && (lhs_prec > word_mode)) + return 1; + } + return 0; +} /* Assign cost of 1 to usual operations. ??? We may consider mapping RTL costs to this. */ case COND_EXPR: @@ -4026,6 +4035,7 @@ estimate_num_insns (gimple *stmt, eni_weights *weights) { unsigned cost, i; enum gimple_code code = gimple_code (stmt); + enum tree_code rhs_code; tree lhs; tree rhs; @@ -4064,9 +4074,17 @@ estimate_num_insns (gimple *stmt, eni_weights *weights) if (gimple_assign_load_p (stmt)) cost += estimate_move_cost (TREE_TYPE (rhs), weights->time_based); - cost += estimate_operator_cost (gimple_assign_rhs_code (stmt), weights, + rhs_code = gimple_assign_rhs_code (stmt); + if ((rhs_code == NOP_EXPR) || (rhs_code == CONVERT_EXPR)) + { +cost += estimate_operator_cost (rhs_code, weights, + gimple_assign_lhs (stmt), + gimple_assign_rhs1 (stmt)); + } + else +cost += estimate_operator_cost (rhs_code, weights, gimple_assign_rhs1 (stmt), - get_gimple_rhs_class (gimple_assign_rhs_code (stmt)) + get_gimple_rhs_class (rhs_code) == GIMPLE_BINARY_RHS ? gimple_assign_rhs2 (stmt) : NULL); break;
Ping: Re: [patch, avr] Add flash size to device info and make wrap around default
Ping! On Monday 14 November 2016 07:03 PM, Pitchumani Sivanupandi wrote: Ping! On Thursday 10 November 2016 01:53 PM, Pitchumani Sivanupandi wrote: On Wednesday 09 November 2016 08:05 PM, Georg-Johann Lay wrote: On 09.11.2016 10:14, Pitchumani Sivanupandi wrote: On Tuesday 08 November 2016 02:57 PM, Georg-Johann Lay wrote: On 08.11.2016 08:08, Pitchumani Sivanupandi wrote: I have updated patch to include the flash size as well. Took that info from device headers (it was fed into crt's device information note section also). The new option would render -mn-flash superfluous, but we should keep it for backward compatibility. Ok. Shouldn't link_pmem_wrap then be removed from link_relax, i.e. from LINK_RELAX_SPEC? And what happens if relaxation is off? Yes. Removed link_pmem_wrap from link_relax. Disabling relaxation doesn't change -mpmem-wrap-around behavior. flashsize-and-wrap-around.patch diff --git a/gcc/config/avr/avr-mcus.def b/gcc/config/avr/avr-mcus.def index 6bcc6ff..9d4aa1a 100644 /* /* Classic, > 8K, <= 64K. */ -AVR_MCU ("avr3", ARCH_AVR3, AVR_ISA_NONE, NULL,0x0060, 0x0, 1) -AVR_MCU ("at43usb355", ARCH_AVR3, AVR_ISA_NONE, "__AVR_AT43USB355__",0x0060, 0x0, 1) -AVR_MCU ("at76c711", ARCH_AVR3, AVR_ISA_NONE, "__AVR_AT76C711__", 0x0060, 0x0, 1) +AVR_MCU ("avr3", ARCH_AVR3, AVR_ISA_NONE, NULL,0x0060, 0x0, 1, 0x6000) +AVR_MCU ("at43usb355", ARCH_AVR3, AVR_ISA_NONE, "__AVR_AT43USB355__",0x0060, 0x0, 1, 0x6000) +AVR_MCU ("at76c711", ARCH_AVR3, AVR_ISA_NONE, "__AVR_AT76C711__", 0x0060, 0x0, 1, 0x4000) +AVR_MCU ("at43usb320", ARCH_AVR3, AVR_ISA_NONE, "__AVR_AT43USB320__",0x0060, 0x0, 1, 0x1) /* Classic, == 128K. */ -AVR_MCU ("avr31",ARCH_AVR31, AVR_ERRATA_SKIP, NULL,0x0060, 0x0, 2) -AVR_MCU ("atmega103",ARCH_AVR31, AVR_ERRATA_SKIP, "__AVR_ATmega103__", 0x0060, 0x0, 2) -AVR_MCU ("at43usb320", ARCH_AVR31, AVR_ISA_NONE, "__AVR_AT43USB320__", 0x0060, 0x0, 2) +AVR_MCU ("avr31",ARCH_AVR31, AVR_ERRATA_SKIP, NULL,0x0060, 0x0, 2, 0x2) +AVR_MCU ("atmega103",ARCH_AVR31, AVR_ERRATA_SKIP, "__AVR_ATmega103__", 0x0060, 0x0, 2, 0x2) /* Classic + MOVW + JMP/CALL. */ If at43usb320 is in the wrong multilib, then this should be handled as separate issue / patch together with its own PR. Sorry for the confusion. I just noticed that some fields don't match... It is not even clear to me from the data sheet if avr3 is the correct multilib or perhaps avr35 (if it supports MOVW) or even avr5 (if it also has MUL) as there is no reference to the exact instruction set -- Atmochip will know. Moreover, such a change should be sync'ed with avr-libc as all multilib stuff is hand-wired there: no use of --print-foo meta information retrieval by avr-libc :-(( I filed PR78275 and https://savannah.nongnu.org/bugs/index.php?49565 for this one. Thats better. I've attached the updated patch. If OK, could someone commit please? I'll try if I could find some more info for AT43USB320. Regards, Pitchumani
Re: [patch, avr] Add flash size to device info and make wrap around default
Ping! On Thursday 10 November 2016 01:53 PM, Pitchumani Sivanupandi wrote: On Wednesday 09 November 2016 08:05 PM, Georg-Johann Lay wrote: On 09.11.2016 10:14, Pitchumani Sivanupandi wrote: On Tuesday 08 November 2016 02:57 PM, Georg-Johann Lay wrote: On 08.11.2016 08:08, Pitchumani Sivanupandi wrote: I have updated patch to include the flash size as well. Took that info from device headers (it was fed into crt's device information note section also). The new option would render -mn-flash superfluous, but we should keep it for backward compatibility. Ok. Shouldn't link_pmem_wrap then be removed from link_relax, i.e. from LINK_RELAX_SPEC? And what happens if relaxation is off? Yes. Removed link_pmem_wrap from link_relax. Disabling relaxation doesn't change -mpmem-wrap-around behavior. flashsize-and-wrap-around.patch diff --git a/gcc/config/avr/avr-mcus.def b/gcc/config/avr/avr-mcus.def index 6bcc6ff..9d4aa1a 100644 /* /* Classic, > 8K, <= 64K. */ -AVR_MCU ("avr3", ARCH_AVR3, AVR_ISA_NONE, NULL,0x0060, 0x0, 1) -AVR_MCU ("at43usb355", ARCH_AVR3, AVR_ISA_NONE, "__AVR_AT43USB355__",0x0060, 0x0, 1) -AVR_MCU ("at76c711", ARCH_AVR3, AVR_ISA_NONE, "__AVR_AT76C711__", 0x0060, 0x0, 1) +AVR_MCU ("avr3", ARCH_AVR3, AVR_ISA_NONE, NULL,0x0060, 0x0, 1, 0x6000) +AVR_MCU ("at43usb355", ARCH_AVR3, AVR_ISA_NONE, "__AVR_AT43USB355__",0x0060, 0x0, 1, 0x6000) +AVR_MCU ("at76c711", ARCH_AVR3, AVR_ISA_NONE, "__AVR_AT76C711__", 0x0060, 0x0, 1, 0x4000) +AVR_MCU ("at43usb320", ARCH_AVR3, AVR_ISA_NONE, "__AVR_AT43USB320__",0x0060, 0x0, 1, 0x1) /* Classic, == 128K. */ -AVR_MCU ("avr31",ARCH_AVR31, AVR_ERRATA_SKIP, NULL,0x0060, 0x0, 2) -AVR_MCU ("atmega103",ARCH_AVR31, AVR_ERRATA_SKIP, "__AVR_ATmega103__", 0x0060, 0x0, 2) -AVR_MCU ("at43usb320", ARCH_AVR31, AVR_ISA_NONE, "__AVR_AT43USB320__", 0x0060, 0x0, 2) +AVR_MCU ("avr31",ARCH_AVR31, AVR_ERRATA_SKIP, NULL,0x0060, 0x0, 2, 0x2) +AVR_MCU ("atmega103",ARCH_AVR31, AVR_ERRATA_SKIP, "__AVR_ATmega103__", 0x0060, 0x0, 2, 0x2) /* Classic + MOVW + JMP/CALL. */ If at43usb320 is in the wrong multilib, then this should be handled as separate issue / patch together with its own PR. Sorry for the confusion. I just noticed that some fields don't match... It is not even clear to me from the data sheet if avr3 is the correct multilib or perhaps avr35 (if it supports MOVW) or even avr5 (if it also has MUL) as there is no reference to the exact instruction set -- Atmochip will know. Moreover, such a change should be sync'ed with avr-libc as all multilib stuff is hand-wired there: no use of --print-foo meta information retrieval by avr-libc :-(( I filed PR78275 and https://savannah.nongnu.org/bugs/index.php?49565 for this one. Thats better. I've attached the updated patch. If OK, could someone commit please? I'll try if I could find some more info for AT43USB320. Regards, Pitchumani
Re: [patch, avr] Add flash size to device info and make wrap around default
On Wednesday 09 November 2016 08:05 PM, Georg-Johann Lay wrote: On 09.11.2016 10:14, Pitchumani Sivanupandi wrote: On Tuesday 08 November 2016 02:57 PM, Georg-Johann Lay wrote: On 08.11.2016 08:08, Pitchumani Sivanupandi wrote: I have updated patch to include the flash size as well. Took that info from device headers (it was fed into crt's device information note section also). The new option would render -mn-flash superfluous, but we should keep it for backward compatibility. Ok. Shouldn't link_pmem_wrap then be removed from link_relax, i.e. from LINK_RELAX_SPEC? And what happens if relaxation is off? Yes. Removed link_pmem_wrap from link_relax. Disabling relaxation doesn't change -mpmem-wrap-around behavior. flashsize-and-wrap-around.patch diff --git a/gcc/config/avr/avr-mcus.def b/gcc/config/avr/avr-mcus.def index 6bcc6ff..9d4aa1a 100644 /* /* Classic, > 8K, <= 64K. */ -AVR_MCU ("avr3", ARCH_AVR3, AVR_ISA_NONE, NULL,0x0060, 0x0, 1) -AVR_MCU ("at43usb355", ARCH_AVR3, AVR_ISA_NONE, "__AVR_AT43USB355__",0x0060, 0x0, 1) -AVR_MCU ("at76c711", ARCH_AVR3, AVR_ISA_NONE, "__AVR_AT76C711__", 0x0060, 0x0, 1) +AVR_MCU ("avr3", ARCH_AVR3, AVR_ISA_NONE, NULL,0x0060, 0x0, 1, 0x6000) +AVR_MCU ("at43usb355", ARCH_AVR3, AVR_ISA_NONE, "__AVR_AT43USB355__",0x0060, 0x0, 1, 0x6000) +AVR_MCU ("at76c711", ARCH_AVR3, AVR_ISA_NONE, "__AVR_AT76C711__", 0x0060, 0x0, 1, 0x4000) +AVR_MCU ("at43usb320", ARCH_AVR3, AVR_ISA_NONE, "__AVR_AT43USB320__",0x0060, 0x0, 1, 0x1) /* Classic, == 128K. */ -AVR_MCU ("avr31",ARCH_AVR31, AVR_ERRATA_SKIP, NULL,0x0060, 0x0, 2) -AVR_MCU ("atmega103",ARCH_AVR31, AVR_ERRATA_SKIP, "__AVR_ATmega103__", 0x0060, 0x0, 2) -AVR_MCU ("at43usb320", ARCH_AVR31, AVR_ISA_NONE, "__AVR_AT43USB320__", 0x0060, 0x0, 2) +AVR_MCU ("avr31",ARCH_AVR31, AVR_ERRATA_SKIP, NULL,0x0060, 0x0, 2, 0x2) +AVR_MCU ("atmega103",ARCH_AVR31, AVR_ERRATA_SKIP, "__AVR_ATmega103__", 0x0060, 0x0, 2, 0x2) /* Classic + MOVW + JMP/CALL. */ If at43usb320 is in the wrong multilib, then this should be handled as separate issue / patch together with its own PR. Sorry for the confusion. I just noticed that some fields don't match... It is not even clear to me from the data sheet if avr3 is the correct multilib or perhaps avr35 (if it supports MOVW) or even avr5 (if it also has MUL) as there is no reference to the exact instruction set -- Atmochip will know. Moreover, such a change should be sync'ed with avr-libc as all multilib stuff is hand-wired there: no use of --print-foo meta information retrieval by avr-libc :-(( I filed PR78275 and https://savannah.nongnu.org/bugs/index.php?49565 for this one. Thats better. I've attached the updated patch. If OK, could someone commit please? I'll try if I could find some more info for AT43USB320. Regards, Pitchumani diff --git a/gcc/config/avr/avr-arch.h b/gcc/config/avr/avr-arch.h index 42eaee5..e0961d4 100644 --- a/gcc/config/avr/avr-arch.h +++ b/gcc/config/avr/avr-arch.h @@ -122,6 +122,9 @@ typedef struct /* Number of 64k segments in the flash. */ int n_flash; + + /* Flash size in bytes. */ + int flash_size; } avr_mcu_t; /* AVR device specific features. diff --git a/gcc/config/avr/avr-devices.c b/gcc/config/avr/avr-devices.c index 7d13ba4..cef3b9a 100644 --- a/gcc/config/avr/avr-devices.c +++ b/gcc/config/avr/avr-devices.c @@ -111,12 +111,12 @@ avr_texinfo[] = const avr_mcu_t avr_mcu_types[] = { -#define AVR_MCU(NAME, ARCH, DEV_ATTRIBUTE, MACRO, DATA_SEC, TEXT_SEC, N_FLASH)\ - { NAME, ARCH, DEV_ATTRIBUTE, MACRO, DATA_SEC, TEXT_SEC, N_FLASH }, +#define AVR_MCU(NAME, ARCH, DEV_ATTRIBUTE, MACRO, DATA_SEC, TEXT_SEC, N_FLASH, FLASH_SIZE)\ + { NAME, ARCH, DEV_ATTRIBUTE, MACRO, DATA_SEC, TEXT_SEC, N_FLASH, FLASH_SIZE }, #include "avr-mcus.def" #undef AVR_MCU /* End of list. */ - { NULL, ARCH_UNKNOWN, AVR_ISA_NONE, NULL, 0, 0, 0 } + { NULL, ARCH_UNKNOWN, AVR_ISA_NONE, NULL, 0, 0, 0, 0 } }; diff --git a/gcc/config/avr/avr-mcus.def b/gcc/config/avr/avr-mcus.def index 6bcc6ff..6e51c2e 100644 --- a/gcc/config/avr/avr-mcus.def +++ b/gcc/config/avr/avr-mcus.def @@ -62,295 +62,297 @@ N_FLASH Number of 64 KiB flash segments, rounded up. The default value for -mn-flash=. + FLASH_SIZEFlash size in bytes. + "avr2" must be first for the "0" default to work as intended. */ /* Classic, <= 8K. */ -AVR_MCU ("avr2",
Re: [patch, avr] Add flash size to device info and make wrap around default
On Tuesday 08 November 2016 02:57 PM, Georg-Johann Lay wrote: On 08.11.2016 08:08, Pitchumani Sivanupandi wrote: I have updated patch to include the flash size as well. Took that info from device headers (it was fed into crt's device information note section also). The new option would render -mn-flash superfluous, but we should keep it for backward compatibility. Ok. Shouldn't link_pmem_wrap then be removed from link_relax, i.e. from LINK_RELAX_SPEC? And what happens if relaxation is off? Yes. Removed link_pmem_wrap from link_relax. Disabling relaxation doesn't change -mpmem-wrap-around behavior. Now, wrap around behavior is changed as follows: For 8K flash devices: Device specs adds --pmem-wrap-around=8k linker option if -mno-pmem-wrap-around is NOT enabled. It makes the --pmem-wrap-around=8k linker option default for 8k flash devices. For 16/32/64K flash devices: Spec string 'link_pmem_wrap' added to all 16/32/64k flash devices specs. Other wise no changes i.e. It adds --pmem-wrap-around=16/32/64k option if -mpmem-wrap-around option is enabled. For other devices, no changes in device specs. Reg tested with default and -mrelax options enabled. No issues. Regards, Pitchumani gcc/ChangeLog 2016-11-08 Pitchumani Sivanupandi * config/avr/avr-arch.h (avr_mcu_t): Add flash_size member. * config/avr/avr-devices.c(avr_mcu_types): Add flash size info. * config/avr/avr-mcu.def: Likewise. * config/avr/gen-avr-mmcu-specs.c (print_mcu): Remove hard-coded prefix check to find wrap-around value, instead use MCU flash size. For 8k flash devices, update link_pmem_wrap spec string to add --pmem-wrap-around=8k. * config/avr/specs.h: Remove link_pmem_wrap from LINK_RELAX_SPEC and add to linker specs (LINK_SPEC) directly. flashsize-and-wrap-around.patch diff --git a/gcc/config/avr/avr-mcus.def b/gcc/config/avr/avr-mcus.def index 6bcc6ff..9d4aa1a 100644 /* Classic, == 128K. */ -AVR_MCU ("avr31",ARCH_AVR31, AVR_ERRATA_SKIP, NULL,0x0060, 0x0, 2) -AVR_MCU ("atmega103",ARCH_AVR31, AVR_ERRATA_SKIP, "__AVR_ATmega103__", 0x0060, 0x0, 2) -AVR_MCU ("at43usb320", ARCH_AVR31, AVR_ISA_NONE, "__AVR_AT43USB320__", 0x0060, 0x0, 2) +AVR_MCU ("avr31",ARCH_AVR31, AVR_ERRATA_SKIP, NULL,0x0060, 0x0, 2, 0x2) +AVR_MCU ("atmega103",ARCH_AVR31, AVR_ERRATA_SKIP, "__AVR_ATmega103__", 0x0060, 0x0, 2, 0x2) +AVR_MCU ("at43usb320", ARCH_AVR31, AVR_ISA_NONE, "__AVR_AT43USB320__", 0x0060, 0x0, 2, 0x1) This looks incorrect: either .flash_size should be 0x2 or n_flash be 1. As you draw the information from internal hardware descriptions, I'd guess that the new information is more reliable? ...as this also determines whether AT43USB320supports ELPM this also means that the device is in the wrong multilib set? I couldn't find the data sheet at atmel.com, and the one I found on the net only mentions 64KiB program memory. It mentions LPM on pp. 9 but has no reference to the supported instruction set. From the manual I would conclude that this device should be avr3, not avr31? Yes. I couldn't find any other source other than the datasheet in net. This device do not have internal program memory. Flash size is set to 64K as the device could address only 64K. No RAMPZ register, so no ELPM. Moved device to AVR3 architecture. +AVR_MCU ("atxmega384c3", ARCH_AVRXMEGA6, AVR_ISA_RMW, "__AVR_ATxmega384C3__", 0x2000, 0x0, 6, 0x62000) +AVR_MCU ("atxmega384d3", ARCH_AVRXMEGA6, AVR_ISA_NONE, "__AVR_ATxmega384D3__", 0x2000, 0x0, 6, 0x62000) /* Xmega, 128K < Flash, RAM > 64K RAM. */ Two more glitches; presumably, .n_flash should be 7 here? Or even better, we can drop .n_flash field in the future and use the more reliable, finer grained information from .flash_size instead? Updated. Yes, in future we could use only flash_size. Regards, Pitchumani gcc/ChangeLog 2016-11-09 Pitchumani Sivanupandi * config/avr/avr-arch.h (avr_mcu_t): Add flash_size member. * config/avr/avr-devices.c(avr_mcu_types): Add flash size info. * config/avr/avr-mcu.def: Likewise. Moved at43usb32x to AVR3 architecture as it do not support ELPM (no RAMPZ register). * config/avr/gen-avr-mmcu-specs.c (print_mcu): Remove hard-coded prefix check to find wrap-around value, instead use MCU flash size. For 8k flash devices, update link_pmem_wrap spec string to add --pmem-wrap-around=8k. * config/avr/specs.h: Remove link_pmem_wrap from LINK_RELAX_SPEC and add to linker specs (LINK_SPEC) directly. diff --git a/gcc/config/avr/avr-arch.h b/gcc/config/avr/avr-arch.h index 42eaee5..e0961d4 100644 --- a/gcc/config/avr/avr-arch.h +++ b/gcc/config/avr/
[patch, avr] Add flash size to device info and make wrap around default (was: Re: [patch, avr] Make pmem-wrap-around option as default)
On Thursday 03 November 2016 06:19 PM, Georg-Johann Lay wrote: On 03.11.2016 08:58, Pitchumani Sivanupandi wrote: Most of the AVR's 8k memorydevices have only rjmp instruction, not jmp. So, it is important to wrap around jump destination to check if it can reach backwards. Currently link specs passes --pmem-wrap-around=xxK when mrelax and mpmem-wrap-around options are enabled. Attached patch changes the specs so that option --pmem-wrap-around=8K is passed for 8k memory devices if -mno-pmem-wrap-around is not enabled. If OK, could someone commit please? Note: Currently 8k devices are identified based on name prefix. We are working on alternative method to incorporate flash memory size. Currently, "at90usb8" this the only prefix that results in wrap_k = 8, so without adding knowledge about flash sizes to the compiler, the change makes not much sense... (but does not do harm either). There are ~40 devices with 8K flash. Current checks are out-dated. The right place for flash size info would be avr-mcus.def together with a new command option to specify the flash size and draw conclusions from it. When I asked Atmel about a mapping of Device to flash size, the support told me to check the data sheets -- not really practical for ~300 devices in avr-mcus.def :-) Atmel *must* have information about this... I have updated patch to include the flash size as well. Took that info from device headers (it was fed into crt's device information note section also). The new option would render -mn-flash superfluous, but we should keep it for backward compatibility. Ok. Shouldn't link_pmem_wrap then be removed from link_relax, i.e. from LINK_RELAX_SPEC? And what happens if relaxation is off? Yes. Removed link_pmem_wrap from link_relax. Disabling relaxation doesn't change -mpmem-wrap-around behavior. Now, wrap around behavior is changed as follows: For 8K flash devices: Device specs adds --pmem-wrap-around=8k linker option if -mno-pmem-wrap-around is NOT enabled. It makes the --pmem-wrap-around=8k linker option default for 8k flash devices. For 16/32/64K flash devices: Spec string 'link_pmem_wrap' added to all 16/32/64k flash devices specs. Other wise no changes i.e. It adds --pmem-wrap-around=16/32/64k option if -mpmem-wrap-around option is enabled. For other devices, no changes in device specs. Reg tested with default and -mrelax options enabled. No issues. If OK, could someone commit please? Regards, Pitchumani gcc/ChangeLog 2016-11-08 Pitchumani Sivanupandi * config/avr/avr-arch.h (avr_mcu_t): Add flash_size member. * config/avr/avr-devices.c(avr_mcu_types): Add flash size info. * config/avr/avr-mcu.def: Likewise. * config/avr/gen-avr-mmcu-specs.c (print_mcu): Remove hard-coded prefix check to find wrap-around value, instead use MCU flash size. For 8k flash devices, update link_pmem_wrap spec string to add --pmem-wrap-around=8k. * config/avr/specs.h: Remove link_pmem_wrap from LINK_RELAX_SPEC and add to linker specs (LINK_SPEC) directly. diff --git a/gcc/config/avr/avr-arch.h b/gcc/config/avr/avr-arch.h index 42eaee5..e0961d4 100644 --- a/gcc/config/avr/avr-arch.h +++ b/gcc/config/avr/avr-arch.h @@ -122,6 +122,9 @@ typedef struct /* Number of 64k segments in the flash. */ int n_flash; + + /* Flash size in bytes. */ + int flash_size; } avr_mcu_t; /* AVR device specific features. diff --git a/gcc/config/avr/avr-devices.c b/gcc/config/avr/avr-devices.c index 7d13ba4..cef3b9a 100644 --- a/gcc/config/avr/avr-devices.c +++ b/gcc/config/avr/avr-devices.c @@ -111,12 +111,12 @@ avr_texinfo[] = const avr_mcu_t avr_mcu_types[] = { -#define AVR_MCU(NAME, ARCH, DEV_ATTRIBUTE, MACRO, DATA_SEC, TEXT_SEC, N_FLASH)\ - { NAME, ARCH, DEV_ATTRIBUTE, MACRO, DATA_SEC, TEXT_SEC, N_FLASH }, +#define AVR_MCU(NAME, ARCH, DEV_ATTRIBUTE, MACRO, DATA_SEC, TEXT_SEC, N_FLASH, FLASH_SIZE)\ + { NAME, ARCH, DEV_ATTRIBUTE, MACRO, DATA_SEC, TEXT_SEC, N_FLASH, FLASH_SIZE }, #include "avr-mcus.def" #undef AVR_MCU /* End of list. */ - { NULL, ARCH_UNKNOWN, AVR_ISA_NONE, NULL, 0, 0, 0 } + { NULL, ARCH_UNKNOWN, AVR_ISA_NONE, NULL, 0, 0, 0, 0 } }; diff --git a/gcc/config/avr/avr-mcus.def b/gcc/config/avr/avr-mcus.def index 6bcc6ff..9d4aa1a 100644 --- a/gcc/config/avr/avr-mcus.def +++ b/gcc/config/avr/avr-mcus.def @@ -62,295 +62,297 @@ N_FLASH Number of 64 KiB flash segments, rounded up. The default value for -mn-flash=. + FLASH_SIZEFlash size in bytes. + "avr2" must be first for the "0" default to work as intended. */ /* Classic, <= 8K. */ -AVR_MCU ("avr2", ARCH_AVR2, AVR_ERRATA_SKIP, NULL, 0x0060, 0x0, 6) -AVR_MCU ("at90s2313",ARCH_AVR2, AVR_SHORT_SP, "__AVR_AT90S2313__", 0x0060, 0x0, 1) -AVR_MCU ("at90s2323",
[patch, avr] Make pmem-wrap-around option as default
Most of the AVR's 8k memorydevices have only rjmp instruction, not jmp. So, it is important to wrap around jump destination to check if it can reach backwards. Currently link specs passes --pmem-wrap-around=xxK when mrelax and mpmem-wrap-around options are enabled. Attached patch changes the specs so that option --pmem-wrap-around=8K is passed for 8k memory devices if -mno-pmem-wrap-around is not enabled. If OK, could someone commit please? Note: Currently 8k devices are identified based on name prefix. We are working on alternative method to incorporate flash memory size. Regards, Pitchumani gcc/ChangeLog 2016-11-03 Pitchumani Sivanupandi * config/avr/gen-avr-mmcu-specs.c (print_mcu): Update link_pmem_wrap spec string to add linker option --pmem-wrap-around=8k. * config/avr/specs.h: Add link_pmem_wrap to linker specs (LINK_SPEC). diff --git a/gcc/config/avr/gen-avr-mmcu-specs.c b/gcc/config/avr/gen-avr-mmcu-specs.c index 7fca756..41dbd80 100644 --- a/gcc/config/avr/gen-avr-mmcu-specs.c +++ b/gcc/config/avr/gen-avr-mmcu-specs.c @@ -220,7 +220,9 @@ print_mcu (const avr_mcu_t *mcu) : 0; fprintf (f, "*link_pmem_wrap:\n"); - if (wrap_k) + if (wrap_k == 8) +fprintf (f, "\t%%{!mno-pmem-wrap-around: --pmem-wrap-around=8k}"); + else if (wrap_k > 8) fprintf (f, "\t%%{mpmem-wrap-around: --pmem-wrap-around=%dk}", wrap_k); fprintf (f, "\n\n"); diff --git a/gcc/config/avr/specs.h b/gcc/config/avr/specs.h index 52763cc..fbf0ce6 100644 --- a/gcc/config/avr/specs.h +++ b/gcc/config/avr/specs.h @@ -65,6 +65,7 @@ along with GCC; see the file COPYING3. If not see "%(link_data_start) " \ "%(link_text_start) " \ "%(link_relax) " \ + "%(link_pmem_wrap) " \ "%{shared:%eshared is not supported} " #undef LIB_SPEC
Re: [patch, avr, pr71676 and pr71678] Issues with casesi expand
On Wednesday 19 October 2016 07:51 PM, Georg-Johann Lay wrote: On 17.10.2016 09:27, Pitchumani Sivanupandi wrote: On Thursday 13 October 2016 08:42 PM, Georg-Johann Lay wrote: On 13.10.2016 13:44, Pitchumani Sivanupandi wrote: On Monday 26 September 2016 08:19 PM, Georg-Johann Lay wrote: On 26.09.2016 15:19, Pitchumani Sivanupandi wrote: Attached patch for PR71676 and PR71678. PR71676 is for AVR target that generates wrong code when switch case index is more than 16 bits. Switch case index of larger than SImode are checked for out of range before 'casesi' expand. RTL expand of casesi gets index as SImode, but index is compared in HImode and ignores upper 16bits. Attached patch changes the expansion for casesi to make the index comparison in SImode and code generation accordingly. PR71678 is ICE because below pattern in 'casesi' is not recognized. (set (reg:HI 47) (minus:HI (subreg:HI (subreg:SI (reg:DI 44) 0) 0) (reg:HI 45))) Fix of PR71676 avoids the above pattern as it changes the comparison to SImode. But this means that all comparisons are now performed in SImode which is a great performance loss for most programs which will switch on 16-bit values. IMO we need a less intrusive (w.r.t. performance) approach. Yes. I tried to split 'casesi' into several based on case values so that compare is done in less expensive modes (i.e. QI or HI). In few cases it is not possible without SImode subtract/ compare. Pattern casesi will have index in SI mode. So, out of range checks will be expensive as most common uses (in AVR) of case values will be in QI/HI mode. e.g. if case values in QI range if upper three bytes index is set goto out_of_range offset = index - lower_bound (QImode) if offset > case_range (QImode) goto out_of_range goto jump_table + offset else if case values in HI range if index[2,3] is set goto out_of_range offset = index - lower_bound (HImode) if offset > case_range (HImode) goto out_of_range goto jump_table + offset This modification will not work for the negative index values. Because code to check upper bytes of index will be expensive than the SImode subtract/ compare. So, I'm trying to update fix to have SImode subtract/ compare if the case values include negative integers. For, others will try to optimize as mentioned above. Is that approach OK? But the above code will be executed at run time and add even more overhead, or am I missing something? If you conclude statically at expand time from the case ranges then we might hit a similar problem as with the original subreg computation. No. Lower bound and case range are const_int_operand, known at compile time. Yes, but if the range if form 10 to 90, say, then you still don't know whether HImode and QImode is appropriate or not which adds to code size and register pressure. As I mentioned earlier, I am working on a different approach which would revert your changes: The casesi is basically unaltered (except for operand clean-ups and avoidance of clobbering subregs). The ups of my approach are: * The original value is known and whether is is QI or HI. * The signedness is known which allows to use the maximum range of QI resp. HI depending on the sign. * Also works on negative values. * All is done at compile time, no need for extra code. * No intermediate 32-bit values, no unnecessary increase of reg pressure. * Optimization can be switched off by -fdisable if desired. * Result can be seen in dumps. The downsides are: * Also needs some lines of code (~400). * Makes assumptions on the anatomy of the code, i.e. extension precedes casesi. First we should decide which route to follow as the changes are conflicting each other. I have not so much time to work on the stuff but the results are promising. If you are interested in the changes, I can supply it but it's all still work in progress. Ok. I'll put this patch on hold for now. Please share if you have draft version of your fix is ready. Thanks. Regards, Pitchumani
Re: [patch, avr, pr71676 and pr71678] Issues with casesi expand
On Thursday 13 October 2016 08:42 PM, Georg-Johann Lay wrote: On 13.10.2016 13:44, Pitchumani Sivanupandi wrote: On Monday 26 September 2016 08:19 PM, Georg-Johann Lay wrote: On 26.09.2016 15:19, Pitchumani Sivanupandi wrote: Attached patch for PR71676 and PR71678. PR71676 is for AVR target that generates wrong code when switch case index is more than 16 bits. Switch case index of larger than SImode are checked for out of range before 'casesi' expand. RTL expand of casesi gets index as SImode, but index is compared in HImode and ignores upper 16bits. Attached patch changes the expansion for casesi to make the index comparison in SImode and code generation accordingly. PR71678 is ICE because below pattern in 'casesi' is not recognized. (set (reg:HI 47) (minus:HI (subreg:HI (subreg:SI (reg:DI 44) 0) 0) (reg:HI 45))) Fix of PR71676 avoids the above pattern as it changes the comparison to SImode. But this means that all comparisons are now performed in SImode which is a great performance loss for most programs which will switch on 16-bit values. IMO we need a less intrusive (w.r.t. performance) approach. Yes. I tried to split 'casesi' into several based on case values so that compare is done in less expensive modes (i.e. QI or HI). In few cases it is not possible without SImode subtract/ compare. Pattern casesi will have index in SI mode. So, out of range checks will be expensive as most common uses (in AVR) of case values will be in QI/HI mode. e.g. if case values in QI range if upper three bytes index is set goto out_of_range offset = index - lower_bound (QImode) if offset > case_range (QImode) goto out_of_range goto jump_table + offset else if case values in HI range if index[2,3] is set goto out_of_range offset = index - lower_bound (HImode) if offset > case_range (HImode) goto out_of_range goto jump_table + offset This modification will not work for the negative index values. Because code to check upper bytes of index will be expensive than the SImode subtract/ compare. So, I'm trying to update fix to have SImode subtract/ compare if the case values include negative integers. For, others will try to optimize as mentioned above. Is that approach OK? But the above code will be executed at run time and add even more overhead, or am I missing something? If you conclude statically at expand time from the case ranges then we might hit a similar problem as with the original subreg computation. No. Lower bound and case range are const_int_operand, known at compile time. Tried to optimize code generated based on case values range. Attached the revised patch. Tested with avrtest, no regression found. Is it OK? Unfortunately, the generated code (setting cc0, a reg and pc) cannot be wrapped into an unspec or parallel and then later be rectified... I am thinking about a new avr target pass to tidy up the code if no 32-bit computation is needed, but this will be some effort. Ok. Regards, Pitchumani gcc/ChangeLog 2016-10-17 Pitchumani Sivanupandi PR target/71676 PR target/71678 * config/avr/avr.md (casesi_index_qi, casesi_index_hi, casesi_index_si): Add new expands, called by casesi based on case values range. gcc/testsuite/ChangeLog 2016-10-17 Pitchumani Sivanupandi PR target/71676 PR target/71678 * gcc.target/avr/pr71676-1.c: New test. * gcc.target/avr/pr71676.c: New test. * gcc.target/avr/pr71678.c: New test. diff --git a/gcc/config/avr/avr.md b/gcc/config/avr/avr.md index 97f3561..b58db14 100644 --- a/gcc/config/avr/avr.md +++ b/gcc/config/avr/avr.md @@ -5152,16 +5152,95 @@ (set_attr "isa" "eijmp") (set_attr "cc" "clobber")]) +; casesi for QI mode case values +(define_expand "casesi_index_qi" + [(set (cc0) +(compare:QI (subreg:QI (match_dup 0) 0) +(match_operand 2 "const_int_operand" ""))) -(define_expand "casesi" - [(parallel [(set (match_dup 6) - (minus:HI (subreg:HI (match_operand:SI 0 "register_operand" "") 0) - (match_operand:HI 1 "register_operand" ""))) - (clobber (scratch:QI))]) - (parallel [(set (cc0) - (compare (match_dup 6) -(match_operand:HI 2 "register_operand" ""))) - (clobber (match_scratch:QI 9 ""))]) + (set (pc) +(if_then_else (gtu (cc0) + (const_int 0)) + (label_ref (match_operand 4 "" "")) + (pc))) + + (set (match_dup 9) +(match_dup 7)) + + (parallel [(set (pc) + (unspec:HI [(match_dup 9)] UNSPEC_INDEX_JMP)) + (u
Re: [patch, avr, pr71676 and pr71678] Issues with casesi expand
On Monday 26 September 2016 08:19 PM, Georg-Johann Lay wrote: On 26.09.2016 15:19, Pitchumani Sivanupandi wrote: Attached patch for PR71676 and PR71678. PR71676 is for AVR target that generates wrong code when switch case index is more than 16 bits. Switch case index of larger than SImode are checked for out of range before 'casesi' expand. RTL expand of casesi gets index as SImode, but index is compared in HImode and ignores upper 16bits. Attached patch changes the expansion for casesi to make the index comparison in SImode and code generation accordingly. PR71678 is ICE because below pattern in 'casesi' is not recognized. (set (reg:HI 47) (minus:HI (subreg:HI (subreg:SI (reg:DI 44) 0) 0) (reg:HI 45))) Fix of PR71676 avoids the above pattern as it changes the comparison to SImode. But this means that all comparisons are now performed in SImode which is a great performance loss for most programs which will switch on 16-bit values. IMO we need a less intrusive (w.r.t. performance) approach. Yes. I tried to split 'casesi' into several based on case values so that compare is done in less expensive modes (i.e. QI or HI). In few cases it is not possible without SImode subtract/ compare. Pattern casesi will have index in SI mode. So, out of range checks will be expensive as most common uses (in AVR) of case values will be in QI/HI mode. e.g. if case values in QI range if upper three bytes index is set goto out_of_range offset = index - lower_bound (QImode) if offset > case_range (QImode) goto out_of_range goto jump_table + offset else if case values in HI range if index[2,3] is set goto out_of_range offset = index - lower_bound (HImode) if offset > case_range (HImode) goto out_of_range goto jump_table + offset This modification will not work for the negative index values. Because code to check upper bytes of index will be expensive than the SImode subtract/ compare. So, I'm trying to update fix to have SImode subtract/ compare if the case values include negative integers. For, others will try to optimize as mentioned above. Is that approach OK? Alternatively we can have flags to generate shorter code for 'casesi' using HImode subtract/ compare. But correctness is not guaranteed (PR71676). Regards, Pitchumani
[patch, avr, pr71676 and pr71678] Issues with casesi expand
Attached patch for PR71676 and PR71678. PR71676 is for AVR target that generates wrong code when switch case index is more than 16 bits. Switch case index of larger than SImode are checked for out of range before 'casesi' expand. RTL expand of casesi gets index as SImode, but index is compared in HImode and ignores upper 16bits. Attached patch changes the expansion for casesi to make the index comparison in SImode and code generation accordingly. PR71678 is ICE because below pattern in 'casesi' is not recognized. (set (reg:HI 47) (minus:HI (subreg:HI (subreg:SI (reg:DI 44) 0) 0) (reg:HI 45))) Fix of PR71676 avoids the above pattern as it changes the comparison to SImode. Regtested using avrtest. No regression found. If OK, could someone commit please? Is this OK for gcc-5-branch? Regards, Pitchumani gcc/ChangeLog 2016-09-26 Pitchumani Sivanupandi PR target/71676 PR target/71678 * config/avr/avr.md (casesi): Change index compare to SI mode. gcc/testsuite/ChangeLog 2016-09-26 Pitchumani Sivanupandi PR target/71676 PR target/71678 * gcc.target/avr/pr71676-1.c: New test. * gcc.target/avr/pr71676.c: New test. * gcc.target/avr/pr71678.c: New test. diff --git a/gcc/config/avr/avr.md b/gcc/config/avr/avr.md index 97f3561..4b1bf9c 100644 --- a/gcc/config/avr/avr.md +++ b/gcc/config/avr/avr.md @@ -5155,12 +5155,12 @@ (define_expand "casesi" [(parallel [(set (match_dup 6) - (minus:HI (subreg:HI (match_operand:SI 0 "register_operand" "") 0) - (match_operand:HI 1 "register_operand" ""))) + (minus:SI (match_operand:SI 0 "register_operand" "") + (match_operand:SI 1 "register_operand" ""))) (clobber (scratch:QI))]) (parallel [(set (cc0) (compare (match_dup 6) -(match_operand:HI 2 "register_operand" ""))) +(match_operand:SI 2 "register_operand" ""))) (clobber (match_scratch:QI 9 ""))]) (set (pc) @@ -5179,20 +5179,20 @@ (clobber (match_dup 8))])] "" { -operands[6] = gen_reg_rtx (HImode); +operands[6] = gen_reg_rtx (SImode); if (AVR_HAVE_EIJMP_EICALL) { -operands[7] = operands[6]; +operands[7] = simplify_gen_subreg (HImode, operands[6], SImode, 0); operands[8] = all_regs_rtx[24]; operands[10] = gen_rtx_REG (HImode, REG_Z); } else { -operands[7] = gen_rtx_PLUS (HImode, operands[6], +operands[7] = gen_rtx_PLUS (HImode, simplify_gen_subreg (HImode, operands[6], SImode, 0), gen_rtx_LABEL_REF (VOIDmode, operands[3])); operands[8] = const0_rtx; -operands[10] = operands[6]; +operands[10] = simplify_gen_subreg (HImode, operands[6], SImode, 0); } }) diff --git a/gcc/testsuite/gcc.target/avr/pr71676-1.c b/gcc/testsuite/gcc.target/avr/pr71676-1.c new file mode 100644 index 000..9a74909 --- /dev/null +++ b/gcc/testsuite/gcc.target/avr/pr71676-1.c @@ -0,0 +1,332 @@ +/* { dg-do run } */ +/* { dg-options "-Os -Wno-overflow" } */ + +#include "exit-abort.h" +volatile unsigned char y; + +unsigned char __attribute__((noinline)) foo1 (char x) +{ +switch (x) +{ + case (char)0x11: y = 7; break; + case (char)0x12: y = 4; break; + case (char)0x13: y = 8; break; + case (char)0x14: y = 21; break; + case (char)0x15: y = 65; break; + case (char)0x16: y = 27; break; + case (char)0x17: y = 72; break; + case (char)0x18: y = 39; break; + default: y=0; +} +return y; +} + +unsigned char __attribute__((noinline)) foo2 (char x) +{ +switch (x) +{ + case 0x01: y = 7; break; + case 0x02: y = 4; break; + case 0x03: y = 8; break; + case 0x04: y = 21; break; + case 0x05: y = 65; break; + case 0x06: y = 27; break; + case 0x07: y = 72; break; + case 0x08: y = 39; break; + default: y=0; +} +return y; +} + +unsigned char __attribute__((noinline)) foo3 (char x) +{ +switch (x) +{ + case 0x101L: y = 7; break; + case 0x102L: y = 4; break; + case 0x103L: y = 8; break; + case 0x104L: y = 21; break; + case 0x105L: y = 65; break; + case 0x106L: y = 27; break; + case 0x107L: y = 72; break; + case 0x108L: y = 39; break; + default: y=0; +} +return y; +} + +unsigned char __attribute__((noinline)) foo4 (char x) +{ +switch (x) +{ + case 0x10001LL: y = 7; break; + case 0x10002LL: y = 4; break; + case 0x10003LL: y = 8; break; + case 0x10004LL: y = 21; brea
Re: [patch, avr] Fix mmcu to specs issues
Ping! On Friday 29 July 2016 05:14 PM, Pitchumani Sivanupandi wrote: On Friday 29 July 2016 02:06 PM, Georg-Johann Lay wrote: On 28.07.2016 13:50, Pitchumani Sivanupandi wrote: On Tuesday 26 July 2016 06:00 PM, Georg-Johann Lay wrote: On 26.07.2016 12:20, Pitchumani Sivanupandi wrote: avr-gcc expected to find the device specs in the search paths specified. But it doesn't work as expected when device specs in different place than the installed location. Example-1: avr-gcc wrongly assumes the device specs will be in first identified 'device-specs' folder in prefix path. avr-gcc natively supports device at90pwm1, but complains as it couldn't find the specs file in the first 'device-specs' directory in the search path. $ avr-gcc test.c -mmcu=at90pwm1 -B /home/install/dev/atxmega128a1u/ avr-gcc: error: cannot access device-specs for _at90pwm1_ expected at Are the "_"s literally? Then the spec file name should be "specs-_at90pwm1_". No, It was mangled character shown by my terminal for format specifier "%qs" in printf. Ignore it. ... Example-2: Similar issue happens when -flto option is enabled and device specs in custom search path. atxmega128a1u device specs in custom path and that is passed with -B option. Architecture spec files such as specs-avrxmega7 will be in installed location. $ avr-gcc test.c -mmcu=atxmega128a1u -flto -B /home/install/dev/atxmega128a1u/ avr-gcc: error: cannot access device-specs for _avrxmega7_ expected at _/home/install/dev/atxmega128a1u/device-specs/specs-avrxmega7_ avr-gcc: note: supported core architectures: avr2 avr25 avr3 avr31 avr35 avr4 avr5 avr51 avr6 avrxmega2 avrxmega4 avrxmega5 avrxmega6 avrxmega7 avrtiny avr1 avr-gcc: note: you can provide your own specs files, see <http://gcc.gnu.org/onlinedocs/gcc/Spec-Files.html> for details lto-wrapper: fatal error: avr-gcc returned 1 exit status compilation terminated. /home/avr-trunk/install/lib/gcc/avr/7.0.0/../../../../avr/bin/ld: error: lto-wrapper failed collect2: error: ld returned 1 exit status Attached patch to address these issues. Fix: Replace device-specs-file spec function with mmcu-to-device-specs. This will not assume that specs files are in first identified device-specs directory. Instead this spec function will let gcc identify the absolute path of specs file using spec string "device-specs-file (device-specs/specs-%s)". IIUC this leads to problems with LTO and when the install path contains spaces which windows distros usually have. The problem is that the spec function cannot escape the spaces as it would need more than 1 escape level. It might also be the case that -mmcu= is specified more than once (with the same MCU), but I don't remember exactly in which situations this happens... Doesn't this lead to inclusion of more than one specs-file? Yes, it lead to space problem. Modified the patch because of path with spaces problem. When LTO enabled, multiple specs-file are included as -mmcu= is fed back to gcc. It happens with/ without of this patch. e.g. For atmega164p, device's and architecture's specs file given when invoking collect2. -specs=device-specs/specs-atmega164p -specs=device-specs/specs-avr5 Attached new patch. It just removes the conditions that led to originally stated issues. (In driver-avr.c:avr_devicespecs_file) Removed first condition as -mmcu=avr* shall come when LTO enabled. Second condition to check absolute path is wrong as the specfile_name composed here will not be available if the specs file is not present in first 'device-specs' directory. With this approach, we can't issue 'descriptive' error for unavailable specs-file. But, avr-gcc will issue below error if specs file is not found. $ avr-gcc -mmcu=unknown test.c avr-gcc: error: device-specs/specs-unknown: No such file or directory Is that Ok? Looks reasonable, just ... Regards, Pitchumani gcc/ChangeLog 2016-07-28 Pitchumani Sivanupandi * config/avr/driver-avr.c (specfiles_doc_url): Remove. (avr_diagnose_devicespecs_error): Remove. (avr_devicespecs_file): Remove conditions to check specs-file, let -specs= option handler do the validation. const char* avr_devicespecs_file (int argc, const char **argv) { - char *specfile_name; const char *mmcu = NULL; #ifdef DEBUG_SPECS @@ -111,12 +88,10 @@ avr_devicespecs_file (int argc, const char **argv) break; } - specfile_name = concat (argv[0], dir_separator_str, "specs-", mmcu, NULL); - #ifdef DEBUG_SPECS if (verbose_flag) -fnotice (stderr, "'%s': mmcu='%s'\n'%s': specfile='%s'\n\n", - __FUNCTION__, mmcu, __FUNCTION__, specfile_name); +fnotice (stderr, "'%s': mmcu='%s'\n\n", + __FUNCTION__, mmcu); #endif ... this DEBUG_SPECS
Re: [patch, avr] Fix mmcu to specs issues
On Friday 29 July 2016 02:06 PM, Georg-Johann Lay wrote: On 28.07.2016 13:50, Pitchumani Sivanupandi wrote: On Tuesday 26 July 2016 06:00 PM, Georg-Johann Lay wrote: On 26.07.2016 12:20, Pitchumani Sivanupandi wrote: avr-gcc expected to find the device specs in the search paths specified. But it doesn't work as expected when device specs in different place than the installed location. Example-1: avr-gcc wrongly assumes the device specs will be in first identified 'device-specs' folder in prefix path. avr-gcc natively supports device at90pwm1, but complains as it couldn't find the specs file in the first 'device-specs' directory in the search path. $ avr-gcc test.c -mmcu=at90pwm1 -B /home/install/dev/atxmega128a1u/ avr-gcc: error: cannot access device-specs for _at90pwm1_ expected at Are the "_"s literally? Then the spec file name should be "specs-_at90pwm1_". No, It was mangled character shown by my terminal for format specifier "%qs" in printf. Ignore it. ... Example-2: Similar issue happens when -flto option is enabled and device specs in custom search path. atxmega128a1u device specs in custom path and that is passed with -B option. Architecture spec files such as specs-avrxmega7 will be in installed location. $ avr-gcc test.c -mmcu=atxmega128a1u -flto -B /home/install/dev/atxmega128a1u/ avr-gcc: error: cannot access device-specs for _avrxmega7_ expected at _/home/install/dev/atxmega128a1u/device-specs/specs-avrxmega7_ avr-gcc: note: supported core architectures: avr2 avr25 avr3 avr31 avr35 avr4 avr5 avr51 avr6 avrxmega2 avrxmega4 avrxmega5 avrxmega6 avrxmega7 avrtiny avr1 avr-gcc: note: you can provide your own specs files, see <http://gcc.gnu.org/onlinedocs/gcc/Spec-Files.html> for details lto-wrapper: fatal error: avr-gcc returned 1 exit status compilation terminated. /home/avr-trunk/install/lib/gcc/avr/7.0.0/../../../../avr/bin/ld: error: lto-wrapper failed collect2: error: ld returned 1 exit status Attached patch to address these issues. Fix: Replace device-specs-file spec function with mmcu-to-device-specs. This will not assume that specs files are in first identified device-specs directory. Instead this spec function will let gcc identify the absolute path of specs file using spec string "device-specs-file (device-specs/specs-%s)". IIUC this leads to problems with LTO and when the install path contains spaces which windows distros usually have. The problem is that the spec function cannot escape the spaces as it would need more than 1 escape level. It might also be the case that -mmcu= is specified more than once (with the same MCU), but I don't remember exactly in which situations this happens... Doesn't this lead to inclusion of more than one specs-file? Yes, it lead to space problem. Modified the patch because of path with spaces problem. When LTO enabled, multiple specs-file are included as -mmcu= is fed back to gcc. It happens with/ without of this patch. e.g. For atmega164p, device's and architecture's specs file given when invoking collect2. -specs=device-specs/specs-atmega164p -specs=device-specs/specs-avr5 Attached new patch. It just removes the conditions that led to originally stated issues. (In driver-avr.c:avr_devicespecs_file) Removed first condition as -mmcu=avr* shall come when LTO enabled. Second condition to check absolute path is wrong as the specfile_name composed here will not be available if the specs file is not present in first 'device-specs' directory. With this approach, we can't issue 'descriptive' error for unavailable specs-file. But, avr-gcc will issue below error if specs file is not found. $ avr-gcc -mmcu=unknown test.c avr-gcc: error: device-specs/specs-unknown: No such file or directory Is that Ok? Looks reasonable, just ... Regards, Pitchumani gcc/ChangeLog 2016-07-28 Pitchumani Sivanupandi * config/avr/driver-avr.c (specfiles_doc_url): Remove. (avr_diagnose_devicespecs_error): Remove. (avr_devicespecs_file): Remove conditions to check specs-file, let -specs= option handler do the validation. const char* avr_devicespecs_file (int argc, const char **argv) { - char *specfile_name; const char *mmcu = NULL; #ifdef DEBUG_SPECS @@ -111,12 +88,10 @@ avr_devicespecs_file (int argc, const char **argv) break; } - specfile_name = concat (argv[0], dir_separator_str, "specs-", mmcu, NULL); - #ifdef DEBUG_SPECS if (verbose_flag) -fnotice (stderr, "'%s': mmcu='%s'\n'%s': specfile='%s'\n\n", - __FUNCTION__, mmcu, __FUNCTION__, specfile_name); +fnotice (stderr, "'%s': mmcu='%s'\n\n", + __FUNCTION__, mmcu); #endif ... this DEBUG_SPECS makes hardly any sense any more because it doesn't print any specs-re
Re: [patch, avr] Fix mmcu to specs issues
On Tuesday 26 July 2016 06:00 PM, Georg-Johann Lay wrote: On 26.07.2016 12:20, Pitchumani Sivanupandi wrote: avr-gcc expected to find the device specs in the search paths specified. But it doesn't work as expected when device specs in different place than the installed location. Example-1: avr-gcc wrongly assumes the device specs will be in first identified 'device-specs' folder in prefix path. avr-gcc natively supports device at90pwm1, but complains as it couldn't find the specs file in the first 'device-specs' directory in the search path. $ avr-gcc test.c -mmcu=at90pwm1 -B /home/install/dev/atxmega128a1u/ avr-gcc: error: cannot access device-specs for _at90pwm1_ expected at Are the "_"s literally? Then the spec file name should be "specs-_at90pwm1_". No, It was mangled character shown by my terminal for format specifier "%qs" in printf. Ignore it. ... Example-2: Similar issue happens when -flto option is enabled and device specs in custom search path. atxmega128a1u device specs in custom path and that is passed with -B option. Architecture spec files such as specs-avrxmega7 will be in installed location. $ avr-gcc test.c -mmcu=atxmega128a1u -flto -B /home/install/dev/atxmega128a1u/ avr-gcc: error: cannot access device-specs for _avrxmega7_ expected at _/home/install/dev/atxmega128a1u/device-specs/specs-avrxmega7_ avr-gcc: note: supported core architectures: avr2 avr25 avr3 avr31 avr35 avr4 avr5 avr51 avr6 avrxmega2 avrxmega4 avrxmega5 avrxmega6 avrxmega7 avrtiny avr1 avr-gcc: note: you can provide your own specs files, see <http://gcc.gnu.org/onlinedocs/gcc/Spec-Files.html> for details lto-wrapper: fatal error: avr-gcc returned 1 exit status compilation terminated. /home/avr-trunk/install/lib/gcc/avr/7.0.0/../../../../avr/bin/ld: error: lto-wrapper failed collect2: error: ld returned 1 exit status Attached patch to address these issues. Fix: Replace device-specs-file spec function with mmcu-to-device-specs. This will not assume that specs files are in first identified device-specs directory. Instead this spec function will let gcc identify the absolute path of specs file using spec string "device-specs-file (device-specs/specs-%s)". IIUC this leads to problems with LTO and when the install path contains spaces which windows distros usually have. The problem is that the spec function cannot escape the spaces as it would need more than 1 escape level. It might also be the case that -mmcu= is specified more than once (with the same MCU), but I don't remember exactly in which situations this happens... Doesn't this lead to inclusion of more than one specs-file? Yes, it lead to space problem. Modified the patch because of path with spaces problem. When LTO enabled, multiple specs-file are included as -mmcu= is fed back to gcc. It happens with/ without of this patch. e.g. For atmega164p, device's and architecture's specs file given when invoking collect2. -specs=device-specs/specs-atmega164p -specs=device-specs/specs-avr5 Attached new patch. It just removes the conditions that led to originally stated issues. (In driver-avr.c:avr_devicespecs_file) Removed first condition as -mmcu=avr* shall come when LTO enabled. Second condition to check absolute path is wrong as the specfile_name composed here will not be available if the specs file is not present in first 'device-specs' directory. With this approach, we can't issue 'descriptive' error for unavailable specs-file. But, avr-gcc will issue below error if specs file is not found. $ avr-gcc -mmcu=unknown test.c avr-gcc: error: device-specs/specs-unknown: No such file or directory Is that Ok? Regards, Pitchumani gcc/ChangeLog 2016-07-28 Pitchumani Sivanupandi * config/avr/driver-avr.c (specfiles_doc_url): Remove. (avr_diagnose_devicespecs_error): Remove. (avr_devicespecs_file): Remove conditions to check specs-file, let -specs= option handler do the validation. diff --git a/gcc/config/avr/driver-avr.c b/gcc/config/avr/driver-avr.c index 83ca373..647f91b 100644 --- a/gcc/config/avr/driver-avr.c +++ b/gcc/config/avr/driver-avr.c @@ -29,41 +29,18 @@ along with GCC; see the file COPYING3. If not see static const char dir_separator_str[] = { DIR_SEPARATOR, 0 }; -static const char specfiles_doc_url[] = - "http://gcc.gnu.org/onlinedocs/gcc/Spec-Files.html";; - - -static const char* -avr_diagnose_devicespecs_error (const char *mcu, const char *filename) -{ - error ("cannot access device-specs for %qs expected at %qs", - mcu, filename); - - // Inform about natively supported devices and cores. - - if (strncmp (mcu, "avr", strlen ("avr"))) -avr_inform_devices (); - - avr_inform_core_architectures (); - - inform (input_location, "you can provide your own specs files, " - "see <%
[patch, avr] Fix mmcu to specs issues
in first identified device-specs directory. Instead this spec function will let gcc identify the absolute path of specs file using spec string "device-specs-file (device-specs/specs-%s)". New spec function for device-specs-file, that will check the access for device specs file and issue diagnostics. If Ok, could someone commit please? I do not have commit access. Regards, Pitchumani gcc/ChangeLog 2016-07-26 Pitchumani Sivanupandi * config/avr/avr.h: Declare spec handle function avr_mmcu_to_devicespecs. (EXTRA_SPEC_FUNCTIONS): Add mmcu-to-device-specs spec function. (DRIVER_SELF_SPECS): Replace device-specs-file spec function with mmcu-to-device-specs. * config/avr/driver-avr.c (avr_diagnose_devicespecs_error): Remove. (avr_devicespecs_file): Implement it for device-specs-file spec function. Issue error if device specs file is not accessible. (avr_mmcu_to_devicespecs): Implement it for mmcu-to-device-specs spec function. Return device-specs-file spec string with deduced specs file. diff --git a/gcc/config/avr/avr.h b/gcc/config/avr/avr.h index 5eb90b5..f69ed22 100644 --- a/gcc/config/avr/avr.h +++ b/gcc/config/avr/avr.h @@ -508,9 +508,11 @@ typedef struct avr_args #define ADJUST_INSN_LENGTH(INSN, LENGTH)\ (LENGTH = avr_adjust_insn_length (INSN, LENGTH)) +extern const char *avr_mmcu_to_devicespecs (int, const char**); extern const char *avr_devicespecs_file (int, const char**); #define EXTRA_SPEC_FUNCTIONS \ + { "mmcu-to-device-specs", avr_mmcu_to_devicespecs }, \ { "device-specs-file", avr_devicespecs_file }, /* Driver self specs has lmited functionality w.r.t. '%s' for dynamic specs. @@ -519,7 +521,7 @@ extern const char *avr_devicespecs_file (int, const char**); #undef DRIVER_SELF_SPECS #define DRIVER_SELF_SPECS \ - " %:device-specs-file(device-specs%s %{mmcu=*:%*})" + " %:mmcu-to-device-specs(device-specs%s %{mmcu=*:%*})" /* No libstdc++ for now. Empty string doesn't work. */ #define LIBSTDCXX "gcc" diff --git a/gcc/config/avr/driver-avr.c b/gcc/config/avr/driver-avr.c index 83ca373..1d75c01 100644 --- a/gcc/config/avr/driver-avr.c +++ b/gcc/config/avr/driver-avr.c @@ -32,12 +32,42 @@ static const char dir_separator_str[] = { DIR_SEPARATOR, 0 }; static const char specfiles_doc_url[] = "http://gcc.gnu.org/onlinedocs/gcc/Spec-Files.html";; +/* Implement spec function 'device-specs-file'. -static const char* -avr_diagnose_devicespecs_error (const char *mcu, const char *filename) + Compose -specs=. Issue error if the spec file is not + accessible. +*/ +const char* +avr_devicespecs_file (int argc, const char **argv) { - error ("cannot access device-specs for %qs expected at %qs", - mcu, filename); + const char *specfile_name = NULL; + const char *mcu = NULL; + + switch (argc) +{ +case 2: + specfile_name = argv[0]; + mcu = argv[1]; + break; + +default: + fatal_error (input_location, + "bad usage of spec function %qs", "device-specs-file"); + return ""; +} + + if (!access (specfile_name, R_OK)) +{ +#ifdef DEBUG_SPECS + if (verbose_flag) +fnotice (stderr, "'%s': mmcu='%s'\n'%s': specfile='%s'\n\n", + __FUNCTION__, mcu, __FUNCTION__, specfile_name); +#endif + return concat ("-specs=", specfile_name, NULL); +} + + error ("cannot access device-specs for %qs expected at GCC-SEARCH-DIRS/device-specs.", + mcu); // Inform about natively supported devices and cores. @@ -49,21 +79,21 @@ avr_diagnose_devicespecs_error (const char *mcu, const char *filename) inform (input_location, "you can provide your own specs files, " "see <%s> for details", specfiles_doc_url); - return X_NODEVLIB; + return ""; } -/* Implement spec function `device-specs-file´. +/* Implement spec function `mmcu-to-device-specs'. - Compose -specs=%s. If everything went well then argv[0] - is the inflated (absolute) specs directory and argv[1] is a device or - core name as supplied by -mmcu=*. When building GCC the path might - be relative. */ + Validate mcu name given with -mmcu option. Use spec function + device-specs-file to get -specs=. If everything went + well then argv[0] is the inflated (absolute) first specs directory and + argv[1] is a device or core name as supplied by -mmcu=*. When building + GCC the path might be relative. */ const char* -avr_devicespecs_file (int argc, const char **argv) +avr_mmcu_to_devicespecs (int argc, const char **argv) { - char *specfile_name; const char *mmcu = NULL; #ifdef DEBUG_SPECS @@ -76,7 +106,7 @@ avr_devicespecs_file (int argc, c
Re: [patch, avr,wwwdocs] PR 58655
Ping! On Wednesday 22 June 2016 12:05 PM, Pitchumani Sivanupandi wrote: On Tuesday 21 June 2016 09:39 PM, Georg-Johann Lay wrote: Pitchumani Sivanupandi schrieb: Attached patches add documentation for -mfract-convert-truncate option and add that info to release notes (gcc-4.9 changes). If OK, could someone commit please? I do not have commit access. Regards, Pitchumani gcc/ChangeLog 2016-06-21 Pitchumani Sivanupandi PR target/58655 * doc/invoke.texi (AVR Options): Document -mfract-convert-truncate option. --- a/wwwdocs/htdocs/gcc-4.9/changes.html +++ b/wwwdocs/htdocs/gcc-4.9/changes.html @@ -579,6 +579,14 @@ auto incr(T x) { return x++; } size when compiling for the M-profile processors. +AVR + + +A new command-line option -mfract-convert-truncate has been added. tags around the option. +It allows compiler to use truncation instead of rounding towards +0 for fractional int types. "zero" instead of "0", and it's for fixed-point types, not for int types. + + IA-32/x86-64 -mfpmath=sse is now implied by -ffast-math ... @emph{Blackfin Options} @gccoptlist{-mcpu=@var{cpu}@r{[}-@var{sirevision}@r{]} @gol @@ -14586,6 +14586,10 @@ sbiw r26, const ; X -= const @opindex mtiny-stack Only change the lower 8@tie{}bits of the stack pointer. +@item -mfract-convert-truncate +@opindex mfract-convert-truncate +Allow to use truncation instead of rounding towards 0 for fractional int types. Same here: "zero" and "fixed-point". + @item -nodevicelib @opindex nodevicelib Don't link against AVR-LibC's device specific library @code{lib.a}. Thanks Johann. Updated the patches. Regards, Pitchumani gcc/ChangeLog 2016-06-22 Pitchumani Sivanupandi PR target/58655 * config/avr/avr.opt (-mfract-convert-truncate): Update description. * doc/invoke.texi (AVR Options): Document it.
Re: [patch, avr,wwwdocs] PR 58655
On Tuesday 21 June 2016 09:39 PM, Georg-Johann Lay wrote: Pitchumani Sivanupandi schrieb: Attached patches add documentation for -mfract-convert-truncate option and add that info to release notes (gcc-4.9 changes). If OK, could someone commit please? I do not have commit access. Regards, Pitchumani gcc/ChangeLog 2016-06-21 Pitchumani Sivanupandi PR target/58655 * doc/invoke.texi (AVR Options): Document -mfract-convert-truncate option. --- a/wwwdocs/htdocs/gcc-4.9/changes.html +++ b/wwwdocs/htdocs/gcc-4.9/changes.html @@ -579,6 +579,14 @@ auto incr(T x) { return x++; } size when compiling for the M-profile processors. +AVR + + +A new command-line option -mfract-convert-truncate has been added. tags around the option. +It allows compiler to use truncation instead of rounding towards +0 for fractional int types. "zero" instead of "0", and it's for fixed-point types, not for int types. + + IA-32/x86-64 -mfpmath=sse is now implied by -ffast-math ... @emph{Blackfin Options} @gccoptlist{-mcpu=@var{cpu}@r{[}-@var{sirevision}@r{]} @gol @@ -14586,6 +14586,10 @@ sbiw r26, const ; X -= const @opindex mtiny-stack Only change the lower 8@tie{}bits of the stack pointer. +@item -mfract-convert-truncate +@opindex mfract-convert-truncate +Allow to use truncation instead of rounding towards 0 for fractional int types. Same here: "zero" and "fixed-point". + @item -nodevicelib @opindex nodevicelib Don't link against AVR-LibC's device specific library @code{lib.a}. Thanks Johann. Updated the patches. Regards, Pitchumani gcc/ChangeLog 2016-06-22 Pitchumani Sivanupandi PR target/58655 * config/avr/avr.opt (-mfract-convert-truncate): Update description. * doc/invoke.texi (AVR Options): Document it. diff --git a/gcc/config/avr/avr.opt b/gcc/config/avr/avr.opt index 05aa4b6..1af792b 100644 --- a/gcc/config/avr/avr.opt +++ b/gcc/config/avr/avr.opt @@ -97,7 +97,7 @@ Warn if the ISR is misspelled, i.e. without __vector prefix. Enabled by default. mfract-convert-truncate Target Report Mask(FRACT_CONV_TRUNC) -Allow to use truncation instead of rounding towards 0 for fractional int types. +Allow to use truncation instead of rounding towards zero for fractional fixed-point types. nodevicelib Driver Target Report RejectNegative diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index e000218..040fb6e 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -643,8 +643,8 @@ Objective-C and Objective-C++ Dialects}. @emph{AVR Options} @gccoptlist{-mmcu=@var{mcu} -maccumulate-args -mbranch-cost=@var{cost} @gol -mcall-prologues -mint8 -mn_flash=@var{size} -mno-interrupts @gol --mrelax -mrmw -mstrict-X -mtiny-stack -nodevicelib -Waddr-space-convert @gol --Wmisspelled-isr} +-mrelax -mrmw -mstrict-X -mtiny-stack -mfract-convert-truncate -nodevicelib @gol +-Waddr-space-convert -Wmisspelled-isr} @emph{Blackfin Options} @gccoptlist{-mcpu=@var{cpu}@r{[}-@var{sirevision}@r{]} @gol @@ -14586,6 +14586,10 @@ sbiw r26, const ; X -= const @opindex mtiny-stack Only change the lower 8@tie{}bits of the stack pointer. +@item -mfract-convert-truncate +@opindex mfract-convert-truncate +Allow to use truncation instead of rounding towards zero for fractional fixed-point types. + @item -nodevicelib @opindex nodevicelib Don't link against AVR-LibC's device specific library @code{lib.a}. --- a/wwwdocs/htdocs/gcc-4.9/changes.html +++ b/wwwdocs/htdocs/gcc-4.9/changes.html @@ -579,6 +579,14 @@ auto incr(T x) { return x++; } size when compiling for the M-profile processors. +AVR + + +A new command-line option -mfract-convert-truncate has been +added. It allows compiler to use truncation instead of rounding towards +zero for fractional fixed-point types. + + IA-32/x86-64 -mfpmath=sse is now implied by -ffast-math
[patch, avr] PR 58655
Attached patches add documentation for -mfract-convert-truncate option and add that info to release notes (gcc-4.9 changes). If OK, could someone commit please? I do not have commit access. Regards, Pitchumani gcc/ChangeLog 2016-06-21 Pitchumani Sivanupandi PR target/58655 * doc/invoke.texi (AVR Options): Document -mfract-convert-truncate option. --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -643,8 +643,8 @@ Objective-C and Objective-C++ Dialects}. @emph{AVR Options} @gccoptlist{-mmcu=@var{mcu} -maccumulate-args -mbranch-cost=@var{cost} @gol -mcall-prologues -mint8 -mn_flash=@var{size} -mno-interrupts @gol --mrelax -mrmw -mstrict-X -mtiny-stack -nodevicelib -Waddr-space-convert @gol --Wmisspelled-isr} +-mrelax -mrmw -mstrict-X -mtiny-stack -mfract-convert-truncate -nodevicelib @gol +-Waddr-space-convert -Wmisspelled-isr} @emph{Blackfin Options} @gccoptlist{-mcpu=@var{cpu}@r{[}-@var{sirevision}@r{]} @gol @@ -14586,6 +14586,10 @@ sbiw r26, const ; X -= const @opindex mtiny-stack Only change the lower 8@tie{}bits of the stack pointer. +@item -mfract-convert-truncate +@opindex mfract-convert-truncate +Allow to use truncation instead of rounding towards 0 for fractional int types. + @item -nodevicelib @opindex nodevicelib Don't link against AVR-LibC's device specific library @code{lib.a}. --- a/wwwdocs/htdocs/gcc-4.9/changes.html +++ b/wwwdocs/htdocs/gcc-4.9/changes.html @@ -579,6 +579,14 @@ auto incr(T x) { return x++; } size when compiling for the M-profile processors. +AVR + + +A new command-line option -mfract-convert-truncate has been added. +It allows compiler to use truncation instead of rounding towards +0 for fractional int types. + + IA-32/x86-64 -mfpmath=sse is now implied by -ffast-math
Re: [patch, avr] Fix PR67353
On Mon, 2016-06-13 at 17:48 +0200, Georg-Johann Lay wrote: > Pitchumani Sivanupandi schrieb: > > > > $ avr-gcc test.c -Wno-misspelled-isr > > $ > What about -Werror=misspelled-isr? Updated patch. > > > > diff --git a/gcc/config/avr/avr.c b/gcc/config/avr/avr.c > > index ba5cd91..587bdbc 100644 > > --- a/gcc/config/avr/avr.c > > +++ b/gcc/config/avr/avr.c > > @@ -753,7 +753,7 @@ avr_set_current_function (tree decl) > > that the name of the function is "__vector_NN" so as to > > catch > > when the user misspells the vector name. */ > > > > - if (!STR_PREFIX_P (name, "__vector")) > > + if ((!STR_PREFIX_P (name, "__vector")) && > > (avr_warn_misspelled_isr)) > > warning_at (loc, 0, "%qs appears to be a misspelled %s > > handler", > If, instead of the "0" the respective OPT_... enum is used in the > call > to warning_at, the -Werror= should work as expected (and explicit > "&& > avr_warn_misspelled_isr" no more needed). Ok. Updated patch as per the comments. If OK, could someone commit please? Regards, Pitchumani gcc/ChangeLog 2016-06-15 Pitchumani Sivanupandi PR target/67353 * config/avr/avr.c (avr_set_current_function): Warn misspelled interrupt/ signal handler if -Wmisspelled-isr flag is enabled. * config/avr/avr.opt (Wmisspelled-isr): New warning flag. Enabled by default to warn misspelled interrupt/ signal handler. * doc/invoke.texi (AVR Options): Document it. Update description for -nodevicelib option. diff --git a/gcc/config/avr/avr.c b/gcc/config/avr/avr.c index ba5cd91..b327624 100644 --- a/gcc/config/avr/avr.c +++ b/gcc/config/avr/avr.c @@ -754,8 +754,8 @@ avr_set_current_function (tree decl) when the user misspells the vector name. */ if (!STR_PREFIX_P (name, "__vector")) -warning_at (loc, 0, "%qs appears to be a misspelled %s handler", -name, isr); +warning_at (loc, OPT_Wmisspelled_isr, "%qs appears to be a misspelled " + "%s handler, missing __vector prefix", name, isr); } /* Don't print the above diagnostics more than once. */ diff --git a/gcc/config/avr/avr.opt b/gcc/config/avr/avr.opt index 8809b9b..05aa4b6 100644 --- a/gcc/config/avr/avr.opt +++ b/gcc/config/avr/avr.opt @@ -91,6 +91,10 @@ Waddr-space-convert Warning C Report Var(avr_warn_addr_space_convert) Init(0) Warn if the address space of an address is changed. +Wmisspelled-isr +Warning C C++ Report Var(avr_warn_misspelled_isr) Init(1) +Warn if the ISR is misspelled, i.e. without __vector prefix. Enabled by default. + mfract-convert-truncate Target Report Mask(FRACT_CONV_TRUNC) Allow to use truncation instead of rounding towards 0 for fractional int types. diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index aa11209..0bf39c5 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -640,7 +640,8 @@ Objective-C and Objective-C++ Dialects}. @emph{AVR Options} @gccoptlist{-mmcu=@var{mcu} -maccumulate-args -mbranch-cost=@var{cost} @gol -mcall-prologues -mint8 -mn_flash=@var{size} -mno-interrupts @gol --mrelax -mrmw -mstrict-X -mtiny-stack -nodevicelib -Waddr-space-convert} +-mrelax -mrmw -mstrict-X -mtiny-stack -nodevicelib -Waddr-space-convert @gol +-Wmisspelled-isr} @emph{Blackfin Options} @gccoptlist{-mcpu=@var{cpu}@r{[}-@var{sirevision}@r{]} @gol @@ -14554,12 +14555,17 @@ Only change the lower 8@tie{}bits of the stack pointer. @item -nodevicelib @opindex nodevicelib -Don't link against AVR-LibC's device specific library @code{libdev.a}. +Don't link against AVR-LibC's device specific library @code{lib.a}. @item -Waddr-space-convert @opindex Waddr-space-convert Warn about conversions between address spaces in the case where the resulting address space is not contained in the incoming address space. + +@item -Wmisspelled-isr +@opindex Wmisspelled-isr +Warn if the ISR is misspelled, i.e. without __vector prefix. +Enabled by default. @end table @subsubsection @code{EIND} and Devices with More Than 128 Ki Bytes of Flash
Re: [patch, avr] Fix PR67353
On Mon, 2016-06-13 at 17:48 +0200, Georg-Johann Lay wrote: > Pitchumani Sivanupandi schrieb: > > > > $ avr-gcc test.c -Wno-misspelled-isr > > $ > What about -Werror=misspelled-isr? Updated patch. > > > > diff --git a/gcc/config/avr/avr.c b/gcc/config/avr/avr.c > > index ba5cd91..587bdbc 100644 > > --- a/gcc/config/avr/avr.c > > +++ b/gcc/config/avr/avr.c > > @@ -753,7 +753,7 @@ avr_set_current_function (tree decl) > > that the name of the function is "__vector_NN" so as to > > catch > > when the user misspells the vector name. */ > > > > - if (!STR_PREFIX_P (name, "__vector")) > > + if ((!STR_PREFIX_P (name, "__vector")) && > > (avr_warn_misspelled_isr)) > > warning_at (loc, 0, "%qs appears to be a misspelled %s > > handler", > If, instead of the "0" the respective OPT_... enum is used in the > call > to warning_at, the -Werror= should work as expected (and explicit > "&& > avr_warn_misspelled_isr" no more needed). Ok. Updated patch as per the comments. If OK, could someone commit please? Regards, Pitchumani diff --git a/gcc/config/avr/avr.c b/gcc/config/avr/avr.c index ba5cd91..b327624 100644 --- a/gcc/config/avr/avr.c +++ b/gcc/config/avr/avr.c @@ -754,8 +754,8 @@ avr_set_current_function (tree decl) when the user misspells the vector name. */ if (!STR_PREFIX_P (name, "__vector")) -warning_at (loc, 0, "%qs appears to be a misspelled %s handler", -name, isr); +warning_at (loc, OPT_Wmisspelled_isr, "%qs appears to be a misspelled " + "%s handler, missing __vector prefix", name, isr); } /* Don't print the above diagnostics more than once. */ diff --git a/gcc/config/avr/avr.opt b/gcc/config/avr/avr.opt index 8809b9b..05aa4b6 100644 --- a/gcc/config/avr/avr.opt +++ b/gcc/config/avr/avr.opt @@ -91,6 +91,10 @@ Waddr-space-convert Warning C Report Var(avr_warn_addr_space_convert) Init(0) Warn if the address space of an address is changed. +Wmisspelled-isr +Warning C C++ Report Var(avr_warn_misspelled_isr) Init(1) +Warn if the ISR is misspelled, i.e. without __vector prefix. Enabled by default. + mfract-convert-truncate Target Report Mask(FRACT_CONV_TRUNC) Allow to use truncation instead of rounding towards 0 for fractional int types. diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index aa11209..0bf39c5 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -640,7 +640,8 @@ Objective-C and Objective-C++ Dialects}. @emph{AVR Options} @gccoptlist{-mmcu=@var{mcu} -maccumulate-args -mbranch-cost=@var{cost} @gol -mcall-prologues -mint8 -mn_flash=@var{size} -mno-interrupts @gol --mrelax -mrmw -mstrict-X -mtiny-stack -nodevicelib -Waddr-space-convert} +-mrelax -mrmw -mstrict-X -mtiny-stack -nodevicelib -Waddr-space-convert @gol +-Wmisspelled-isr} @emph{Blackfin Options} @gccoptlist{-mcpu=@var{cpu}@r{[}-@var{sirevision}@r{]} @gol @@ -14554,12 +14555,17 @@ Only change the lower 8@tie{}bits of the stack pointer. @item -nodevicelib @opindex nodevicelib -Don't link against AVR-LibC's device specific library @code{libdev.a}. +Don't link against AVR-LibC's device specific library @code{lib.a}. @item -Waddr-space-convert @opindex Waddr-space-convert Warn about conversions between address spaces in the case where the resulting address space is not contained in the incoming address space. + +@item -Wmisspelled-isr +@opindex Wmisspelled-isr +Warn if the ISR is misspelled, i.e. without __vector prefix. +Enabled by default. @end table @subsubsection @code{EIND} and Devices with More Than 128 Ki Bytes of Flash
Re: [patch, avr] Fix PR67353
On Fri, 2016-06-10 at 20:08 +0200, Georg-Johann Lay wrote: > Pitchumani Sivanupandi schrieb: > > > > Hi, > > > > This patch introduces new flags for warning 'misspelled interrupt/ > > signal handler'. Flag -Wmisspelled-isr is enabled by default and it > > will warn user if the interrupt/ signal handler is without > > '__vector' > > prefix. Flag -Wno-misspelled-isr shall be enabled by user to allow > > custom names, i.e. without __vector prefix. > > > > // avr-gcc -c test.c > > void custom_interruption(void) __attribute__((signal)); > > void custom_interruption(void) {} > > > > Behavior after applying this patch: > > > > $ avr-gcc test.c > > test.c: In function 'custom_interruption': > > test.c:2:6: warning: 'custom_interruption' appears to be a > > misspelled > > signal handler > > void custom_interruption(void) {} > > ^~~ > > > > $ avr-gcc test.c -Wmisspelled-isr > > test.c: In function > > 'custom_interruption': > > test.c:2:6: warning: 'custom_interruption' > > appears to be a misspelled signal handler > > void > > custom_interruption(void) {} > > ^~~ > > > > $ avr-gcc test.c -Wno-misspelled-isr > > $ > > > > If OK, could someone commit please? I do not have commit access. > > > > Regards, > > Pitchumani > > > > gcc/ChangeLog > > > > 2016-06-10 Pitchumani Sivanupandi > > > Missing PR target/67353 > > > > * config/avr/avr.c (avr_set_current_function): Warn misspelled > > interrupt/ signal handler if warn_misspelled_isr flag is set. > > * config/avr/avr.opt (Wmisspelled-isr): New warning flag. > > Enabled > > by default to warn misspelled interrupt/ signal handler. > Shouldn't it also be documented in doc/invoke.texi? Thanks Johann. Updated the patch. Updated description for -nodevicelib option as well, device library should be lib.a. Regards, Pitchumani gcc/ChangeLog 2016-06-10 Pitchumani Sivanupandi PR target/67353 * config/avr/avr.c (avr_set_current_function): Warn misspelled interrupt/ signal handler if warn_misspelled_isr flag is set. * config/avr/avr.opt (Wmisspelled-isr): New warning flag. Enabled by default to warn misspelled interrupt/ signal handler. * doc/invoke.texi (AVR Options): Document it. Update description for -nodevicelib option.diff --git a/gcc/config/avr/avr.c b/gcc/config/avr/avr.c index ba5cd91..587bdbc 100644 --- a/gcc/config/avr/avr.c +++ b/gcc/config/avr/avr.c @@ -753,7 +753,7 @@ avr_set_current_function (tree decl) that the name of the function is "__vector_NN" so as to catch when the user misspells the vector name. */ - if (!STR_PREFIX_P (name, "__vector")) + if ((!STR_PREFIX_P (name, "__vector")) && (avr_warn_misspelled_isr)) warning_at (loc, 0, "%qs appears to be a misspelled %s handler", name, isr); } diff --git a/gcc/config/avr/avr.opt b/gcc/config/avr/avr.opt index 8809b9b..0703f5a 100644 --- a/gcc/config/avr/avr.opt +++ b/gcc/config/avr/avr.opt @@ -91,6 +91,10 @@ Waddr-space-convert Warning C Report Var(avr_warn_addr_space_convert) Init(0) Warn if the address space of an address is changed. +Wmisspelled-isr +Target Warning Report Var(avr_warn_misspelled_isr) Init(1) +Warn if the ISR is misspelled, i.e. without __vector prefix. Enabled by default. + mfract-convert-truncate Target Report Mask(FRACT_CONV_TRUNC) Allow to use truncation instead of rounding towards 0 for fractional int types. diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index aa11209..0bf39c5 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -640,7 +640,8 @@ Objective-C and Objective-C++ Dialects}. @emph{AVR Options} @gccoptlist{-mmcu=@var{mcu} -maccumulate-args -mbranch-cost=@var{cost} @gol -mcall-prologues -mint8 -mn_flash=@var{size} -mno-interrupts @gol --mrelax -mrmw -mstrict-X -mtiny-stack -nodevicelib -Waddr-space-convert} +-mrelax -mrmw -mstrict-X -mtiny-stack -nodevicelib -Waddr-space-convert @gol +-Wmisspelled-isr} @emph{Blackfin Options} @gccoptlist{-mcpu=@var{cpu}@r{[}-@var{sirevision}@r{]} @gol @@ -14554,12 +14555,17 @@ Only change the lower 8@tie{}bits of the stack pointer. @item -nodevicelib @opindex nodevicelib -Don't link against AVR-LibC's device specific library @code{libdev.a}. +Don't link against AVR-LibC's device specific library @code{lib.a}. @item -Waddr-space-convert @opindex Waddr-space-convert Warn about conversions between address spaces in the case where the resulting address space is not contained in the incoming address space. + +@item -Wmisspelled-isr +@opindex Wmisspelled-isr +Warn if the ISR is misspelled, i.e. without __vector prefix. +Enabled by default. @end table @subsubsection @code{EIND} and Devices with More Than 128 Ki Bytes of Flash
[patch, avr] Fix PR67353
Hi, This patch introduces new flags for warning 'misspelled interrupt/ signal handler'. Flag -Wmisspelled-isr is enabled by default and it will warn user if the interrupt/ signal handler is without '__vector' prefix. Flag -Wno-misspelled-isr shall be enabled by user to allow custom names, i.e. without __vector prefix. // avr-gcc -c test.c void custom_interruption(void) __attribute__((signal)); void custom_interruption(void) {} Behavior after applying this patch: $ avr-gcc test.c test.c: In function 'custom_interruption': test.c:2:6: warning: 'custom_interruption' appears to be a misspelled signal handler void custom_interruption(void) {} ^~~ $ avr-gcc test.c -Wmisspelled-isr test.c: In function 'custom_interruption': test.c:2:6: warning: 'custom_interruption' appears to be a misspelled signal handler void custom_interruption(void) {} ^~~ $ avr-gcc test.c -Wno-misspelled-isr $ If OK, could someone commit please? I do not have commit access. Regards, Pitchumani gcc/ChangeLog 2016-06-10 Pitchumani Sivanupandi * config/avr/avr.c (avr_set_current_function): Warn misspelled interrupt/ signal handler if warn_misspelled_isr flag is set. * config/avr/avr.opt (Wmisspelled-isr): New warning flag. Enabled by default to warn misspelled interrupt/ signal handler. diff --git a/gcc/config/avr/avr.c b/gcc/config/avr/avr.c index ba5cd91..587bdbc 100644 --- a/gcc/config/avr/avr.c +++ b/gcc/config/avr/avr.c @@ -753,7 +753,7 @@ avr_set_current_function (tree decl) that the name of the function is "__vector_NN" so as to catch when the user misspells the vector name. */ - if (!STR_PREFIX_P (name, "__vector")) + if ((!STR_PREFIX_P (name, "__vector")) && (avr_warn_misspelled_isr)) warning_at (loc, 0, "%qs appears to be a misspelled %s handler", name, isr); } diff --git a/gcc/config/avr/avr.opt b/gcc/config/avr/avr.opt index 8809b9b..0703f5a 100644 --- a/gcc/config/avr/avr.opt +++ b/gcc/config/avr/avr.opt @@ -91,6 +91,10 @@ Waddr-space-convert Warning C Report Var(avr_warn_addr_space_convert) Init(0) Warn if the address space of an address is changed. +Wmisspelled-isr +Target Warning Report Var(avr_warn_misspelled_isr) Init(1) +Warn if the ISR is misspelled, i.e. without __vector prefix. Enabled by default. + mfract-convert-truncate Target Report Mask(FRACT_CONV_TRUNC) Allow to use truncation instead of rounding towards 0 for fractional int types.
Ping: [patch, avr] Fix unrecognizable insn ICE for avr (PR71103)
Ping! Note: Removed the garbled characters and added ChangeLog -- avr-gcc crashes for following test as it couldn't recognize the instruction pattern. struct st { unsigned char uc1; unsigned int *ui1; }; unsigned int ui1; struct st foo () { struct st ret; ret.uc1 = 6; ret.ui1 = &ui1; return ret; } $ avr-gcc -mmcu=atmega328p -O1 test.c (-- snip --) test.c: In function 'foo': test.c:12:1: error: unrecognizable insn: } ^ (insn 6 5 7 2 (set (subreg:QI (reg:PSI 42 [ D.1499 ]) 1) (subreg:QI (symbol_ref:HI ("ui1") ) 0)) test.c:11 -1 (nil)) test.c:12:1: internal compiler error: in extract_insn, at recog.c:2287 0xd51195 _fatal_insn(char const*, rtx_def const*, char const*, int, char const*) /home/rudran/code/gcc/gcc/rtl-error.c:108 (--snip--) There is no valid pattern in avr to match the "subreg:QI (symbol_ref:HI)". Attached patch forces the symbol_ref of subreg operand to register so that it will become register operand and movqi pattern shall recognize it. Ran gcc regression test with internal simulators. No new regressions found. If Ok, could someone commit please? Regards, Pitchumani gcc/ChangeLog 2016-05-20 Pitchumani Sivanupandi PR target/71103 * config/avr/avr.md (define_expand "mov"): If the source operand is subreg (symbol_ref) then move the symbol ref to register. gcc/testsuite/ChangeLog 2016-05-20 Pitchumani Sivanupandi PR target/71103 * gcc.target/avr/pr71103.c: New test.diff --git a/gcc/config/avr/avr.md b/gcc/config/avr/avr.md index c988446..927bc69 100644 --- a/gcc/config/avr/avr.md +++ b/gcc/config/avr/avr.md @@ -638,6 +638,13 @@ rtx dest = operands[0]; rtx src = avr_eval_addr_attrib (operands[1]); +if (SUBREG_P(src) && (GET_CODE(XEXP(src,0)) == SYMBOL_REF) && +can_create_pseudo_p()) + { +rtx symbol_ref = XEXP(src, 0); +XEXP (src, 0) = copy_to_mode_reg (GET_MODE(symbol_ref), symbol_ref); + } + if (avr_mem_flash_p (dest)) DONE; diff --git a/gcc/testsuite/gcc.target/avr/pr71103.c b/gcc/testsuite/gcc.target/avr/pr71103.c new file mode 100644 index 000..43244d1 --- /dev/null +++ b/gcc/testsuite/gcc.target/avr/pr71103.c @@ -0,0 +1,16 @@ +/* { dg-do compile } */ +/* { dg-options "-O1" } */ + +struct ResponseStruct{ +unsigned char responseLength; +char *response; +}; + +static char response[5]; +struct ResponseStruct something(){ +struct ResponseStruct returnValue; +returnValue.responseLength = 5; +returnValue.response = response; +return returnValue; +} +
[patch, avr] Fix unrecognizable insn ICE for avr (PR71103)
avr-gcc crashes for following test as it couldn't recognize the instruction pattern. struct st { unsigned char uc1; unsigned int *ui1; }; unsigned int ui1; struct st foo () { struct st ret; ret.uc1 = 6; ret.ui1 = &ui1; return ret; } $ avr-gcc -mmcu=atmega328p -O1 test.c (-- snip --) test.c: In function 'foo': test.c:12:1: error: unrecognizable insn: } ^ (insn 6 5 7 2 (set (subreg:QI (reg:PSI 42 [ D.1499 ]) 1) (subreg:QI (symbol_ref:HI ("ui1") ) 0)) test.c:11 -1 (nil)) test.c:12:1: internal compiler error: in extract_insn, at recog.c:2287 0xd51195 _fatal_insn(char const*, rtx_def const*, char const*, int, char const*) /home/rudran/code/gcc/gcc/rtl-error.c:108 (-- snip --) There is no valid pattern in avr to match the "subreg:QI (symbol_ref:HI)". Attached patch forces the symbol_ref of subreg operand to register so that it will become register operand and movqi pattern shall recognize it. Ran gcc regression test with internal simulators. No new regressions found. If ok, could someone commit please? Regards, Pitchumanidiff --git a/gcc/config/avr/avr.md b/gcc/config/avr/avr.md index c988446..927bc69 100644 --- a/gcc/config/avr/avr.md +++ b/gcc/config/avr/avr.md @@ -638,6 +638,13 @@ rtx dest = operands[0]; rtx src = avr_eval_addr_attrib (operands[1]); +if (SUBREG_P(src) && (GET_CODE(XEXP(src,0)) == SYMBOL_REF) && +can_create_pseudo_p()) + { +rtx symbol_ref = XEXP(src, 0); +XEXP (src, 0) = copy_to_mode_reg (GET_MODE(symbol_ref), symbol_ref); + } + if (avr_mem_flash_p (dest)) DONE; diff --git a/gcc/testsuite/gcc.target/avr/pr71103.c b/gcc/testsuite/gcc.target/avr/pr71103.c new file mode 100644 index 000..43244d1 --- /dev/null +++ b/gcc/testsuite/gcc.target/avr/pr71103.c @@ -0,0 +1,16 @@ +/* { dg-do compile } */ +/* { dg-options "-O1" } */ + +struct ResponseStruct{ +unsigned char responseLength; +char *response; +}; + +static char response[5]; +struct ResponseStruct something(){ +struct ResponseStruct returnValue; +returnValue.responseLength = 5; +returnValue.response = response; +return returnValue; +} +
Re: [Patch, testsuite] Fix sra-13.c for 16 bit int
could someone please review and commit if it is OK? I don't have commit access. Regards, Pitchumani On 4/1/2013 4:30 PM, Pitchumani Sivanupandi wrote: Fix test case sra-13.c that assumed int is always 4 bytes. Regards, Pitchumani 2013-04-01 Pitchumani Sivanupandi testsuite * gcc.dg/tree-ssa/sra-13.c: Fix for 16 bit int --- gcc/testsuite/gcc.dg/tree-ssa/sra-13.c (revision 197081) +++ gcc/testsuite/gcc.dg/tree-ssa/sra-13.c (working copy) @@ -95,7 +95,7 @@ b = 0; gu1.b.l = 2000; s = bar (); - if (s != 2000) + if (s != (int)2000) __builtin_abort (); if (gu2.b.l != 2000) __builtin_abort ();
[Patch, testsuite] Fix sra-13.c for 16 bit int
Fix test case sra-13.c that assumed int is always 4 bytes. Regards, Pitchumani 2013-04-01 Pitchumani Sivanupandi testsuite * gcc.dg/tree-ssa/sra-13.c: Fix for 16 bit int --- gcc/testsuite/gcc.dg/tree-ssa/sra-13.c (revision 197081) +++ gcc/testsuite/gcc.dg/tree-ssa/sra-13.c (working copy) @@ -95,7 +95,7 @@ b = 0; gu1.b.l = 2000; s = bar (); - if (s != 2000) + if (s != (int)2000) __builtin_abort (); if (gu2.b.l != 2000) __builtin_abort ();