This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [Patch]: PR49868: Named address space support for AVR
- From: Georg-Johann Lay <avr at gjlay dot de>
- To: Denis Chertykov <chertykov at gmail dot com>
- Cc: gcc-patches at gcc dot gnu dot org, Eric Weddington <eric dot weddington at atmel dot com>, Joerg Wunsch <joerg_wunsch at uriah dot heep dot sax dot de>, Anatoly Sokolov <aesok at post dot ru>
- Date: Mon, 07 Nov 2011 12:10:02 +0100
- Subject: Re: [Patch]: PR49868: Named address space support for AVR
- References: <4E8DC2C0.8040703@gjlay.de> <4EAA8450.9080207@gjlay.de> <CADOs=za2MdLeOif-DFZK6TQFMCJj8Kg=Azhxh5BUiCZWTTr7qQ@mail.gmail.com> <4EB6D44C.2020609@gjlay.de>
Georg-Johann Lay wrote:
> Denis Chertykov wrote:
>> 2011/10/28 Georg-Johann Lay <????>:
>>> Georg-Johann Lay schrieb:
>>>
>>>> This patch adds named address space support to read data from flash (aka.
>>>> progmem) to target AVR.
>>>>
>>>> The patch has two parts:
>>>>
>>>> The first part is a repost of Ulrich's work from
>>>> http://gcc.gnu.org/ml/gcc/2011-08/msg00131.html
>>>> with the needed changes to ./gcc and ./gcc/doc
>>>>
>>>> This patch is needed because the target hooks MODE_CODE_BASE_REG_CLASS and
>>>> REGNO_MODE_CODE_OK_FOR_BASE_P don't distinguish between different address
>>>> spaces. Ulrich's patch adds respective support to these hooks.
>>>>
>>>> The second part is the AVR dependent part that adds __pgm as address space
>>>> qualifier for address space AS1.
>>>>
>>>> The AVR part is just the worker code. If there is agreement that AS support
>>>> for AVR is okay in principle and Ulrich's work will go into GCC, I will supply
>>>> test programs and updates to the user manual, of course.
>>>>
>>>> The major drawbacks of the current AS implementation are:
>>>>
>>>> - It works only for C.
>>>> For C++, a language extension would be needed as indicated in
>>>> ISO/IEC DTR 18037
>>>> Annex F - C++ Compatibility and Migration issues
>>>> F.2 Multiple Address Spaces Support
>>>>
>>>> - Register allocation does not a good job. AS1 can only be addressed
>>>> byte-wise by one single address register (Z) as per *Z or *Z++.
>>> This flaw from register allocator are filed as PR50775 now.
>>>
>>>> The AVR part does several things:
>>>>
>>>> - It locates data in AS1 into appropriate section, i.e. somewhere in
>>>> .progmem
>>>>
>>>> - It does early sanity checks to ensure that __pgm is always accompanied
>>>> with const so that writing to AS1 in not possible.
>>>>
>>>> - It prints LPM instructions to access flash memory.
>>> The attached patch is an update merge so that it fits without conflicts.
>>>
>>> The patch requires Ulrich's works which is still in review
>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50775
>>>
>>> The regression tests run with this patch and the new ChangeLog enttry si
>>> written as if Ulrich's patch was applied.
>>>
>>> Tests pass without regression.
>>>
>>> Besides the update to a nor up-to-date SVN version, the patch sets a built-in
>>> define __PGM so that it is easy for users to test if or if not the feature is
>>> available.
>>>
>>> Documentation and test cases will follow in separate patch.
>>>
>>> Ok for trunk after Ulrich's work has been approved?
>>>
>>> Johann
>>>
>>> PR target/49868
>>> * config/avr/avr.h (ADDR_SPACE_PGM): New define for address space AS1.
>>> (REGISTER_TARGET_PRAGMAS): New define.
>>> * config/avr/avr-protos.h (avr_mem_pgm_p): New prototype.
>>> (avr_register_target_pragmas): New prototype.
>>> (avr_log_t): Add field "progmem". Order alphabetically.
>>> * config/avr/avr-log.c (avr_log_set_avr_log): Set avr_log.progmem.
>>> * config/avr/avr-c.c (langhooks.h): New include.
>>> (avr_register_target_pragmas): New function. Register address
>>> space AS1 as "__pgm".
>>> (avr_cpu_cpp_builtins): Add built-in define __PGM.
>>> * config/avr/avr.c: Include "c-family/c-common.h".
>>> (TARGET_LEGITIMATE_ADDRESS_P): Remove define.
>>> (TARGET_LEGITIMIZE_ADDRESS): Remove define.
>>> (TARGET_ADDR_SPACE_SUBSET_P): Define to...
>>> (avr_addr_space_subset_p): ...this new static function.
>>> (TARGET_ADDR_SPACE_CONVERT): Define to...
>>> (avr_addr_space_convert): ...this new static function.
>>> (TARGET_ADDR_SPACE_ADDRESS_MODE): Define to...
>>> (avr_addr_space_address_mode): ...this new static function.
>>> (TARGET_ADDR_SPACE_POINTER_MODE): Define to...
>>> (avr_addr_space_pointer_mode): ...this new static function.
>>> (TARGET_ADDR_SPACE_LEGITIMATE_ADDRESS_P): Define to...
>>> (avr_addr_space_legitimate_address_p): ...this new static function.
>>> (TARGET_ADDR_SPACE_LEGITIMIZE_ADDRESS): Define to...
>>> (avr_addr_space_legitimize_address): ...this new static function.
>>> (avr_mode_code_base_reg_class): Handle AS1.
>>> (avr_regno_mode_code_ok_for_base_p): Handle AS1.
>>> (lpm_addr_reg_rtx, lpm_reg_rtx): New static GTYed variables.
>>> (avr_decl_pgm_p): New static function.
>>> (avr_mem_pgm_p): New function.
>>> (avr_asm_len): Return always "" instead of void.
>>> (avr_out_lpm_no_lpmx): New static function.
>>> (avr_out_lpm): New static function.
>>> (output_movqi, output_movhi, output_movsisf): Call avr_out_lpm to
>>> handle loads from progmem.
>>> (avr_progmem_p): Test if decl is in AS1.
>>> (avr_pgm_pointer_const_p): New static function.
>>> (avr_pgm_check_var_decl): New static function.
>>> (avr_insert_attributes): Use it. Change error message to report
>>> cause (progmem or AS1) when code wants to write to AS1.
>>> (avr_section_type_flags): Unset section flag SECTION_BSS for
>>> data in progmem.
>>> * config/avr/avr.md (LPM_REGNO): New define_constants.
>>> (movqi, movhi, movsi, movsf): Skip if code would write to AS1.
>>> (movmemhi): Ditto. Propagate address space information to newly
>>> created MEM.
>>> (split-lpmx): New split.
>>>
>> Approved.
>>
>> Denis.
>
> The patch from above is still not upstream because it needs the following work
> which is still pending review:
>
> http://gcc.gnu.org/ml/gcc-patches/2011-10/msg01874.html
>
> The patch you find in this message contains the above patch with the following
> minor changes:
>
> * The patch is rebased to a newer revision
> * avr_out_lpm_no_lpmx and avr_out_lpm can also load the new 3-byte PSImode
> that got upstream meanwhile.
> * If there is no LMPX instruction and there are more than 2 bytes to load,
> a libgcc function is called.
>
> Moreover, the patch introduces the following major changes:
>
> * There is support for a new 24-bit address space called __pgmx
> * There is support for new 16-bit address spaces that represent
> the different 64k flash segments. Their names are __pgm2 .. __pgm5
> * There are libgcc functions that load from flash for the expensive
> cases, i.e. there is no ELPMX.
>
> The reason why the __pgm<n> address spaces are there is because there is very
> little to do to support them and there is no need for 24-bit pointers.
>
> The work is not yet complete, but I'd like to post it before stage 1 is over.
> Sorry for the inconvenience for just another review of the matter...
>
> What's missing is selection of sections for the numbered address spaces.
>
> The libgcc is independent of the not-yet-reviewed patch from Ulrich.
>
> Is the libgcc part ok for trunk?
> Is the gcc part ok provided Ulrich had been reviewed?
>
> I'd like to do the remaining as follow-up patches:
>
> * Release Notes
> * Documentation
> * Test cases
> * Section handling for numbered address spaces
>
> Johann
>
> gcc/
> PR target/49868
> * config/avr/avr.h (base_arch_s): Add field n_segments.
> (ADDR_SPACE_PGM, ADDR_SPACE_PGM1, ADDR_SPACE_PGM2,
> ADDR_SPACE_PGM3, ADDR_SPACE_PGM4, ADDR_SPACE_PGM5,
> ADDR_SPACE_PGMX): New address spaces.
> (AVR_HAVE_ELPM, AVR_HAVE_ELPMX): New defines.
> (REGISTER_TARGET_PRAGMAS): New define.
> * config/avr/avr-protos.h (avr_mem_pgm_p, avr_mem_pgmx_p): New.
> (avr_out_xload, avr_xload_libgcc_p): New.
> (asm_output_external_libcall): Remove.
> (avr_register_target_pragmas): New.
> (avr_log_t): Add field "progmem". Order alphabetically.
> * config/avr/avr-log.c (avr_log_set_avr_log): Set avr_log.progmem.
> * config/avr/avr-c.c (langhooks.h): New include.
> (avr_register_target_pragmas): New function. Register address
> spaces __pgm, __pgm2, __pgm3, __pgm4 __pgm5, __pgmx.
> (avr_cpu_cpp_builtins): Add built-in defines __PGM, __PGM1,
> __PGM2, __PGM3, __PGM4, __PGM5, __PGMX.
> * config/avr/avr-devices.c (avr_arch_types): Set field n_segments.
>
> * config/avr/avr.c: Include "c-family/c-common.h".
> (TARGET_LEGITIMATE_ADDRESS_P): Remove define.
> (TARGET_LEGITIMIZE_ADDRESS): Remove define.
> (TARGET_ADDR_SPACE_SUBSET_P): Define to...
> (avr_addr_space_subset_p): ...this new static function.
> (TARGET_ADDR_SPACE_CONVERT): Define to...
> (avr_addr_space_convert): ...this new static function.
> (TARGET_ADDR_SPACE_ADDRESS_MODE): Define to...
> (avr_addr_space_address_mode): ...this new static function.
> (TARGET_ADDR_SPACE_POINTER_MODE): Define to...
> (avr_addr_space_pointer_mode): ...this new static function.
> (TARGET_ADDR_SPACE_LEGITIMATE_ADDRESS_P): Define to...
> (avr_addr_space_legitimate_address_p): ...this new static function.
> (TARGET_ADDR_SPACE_LEGITIMIZE_ADDRESS): Define to...
> (avr_addr_space_legitimize_address): ...this new static function.
> (avr_mode_code_base_reg_class): Handle address spaces.
> (avr_regno_mode_code_ok_for_base_p): Ditto.
> (lpm_addr_reg_rtx, lpm_reg_rtx, xstring_empty, xstring_e,
> all_regs_rtx): New static GTYed variables.
> (avr_option_override): Initialize them.
> (avr_pgm_segment): New static function.
> (avr_decl_pgm_p, avr_decl_pgmx_p): New static functions.
> (avr_mem_pgm_p, avr_mem_pgmx_p): New functions.
> (avr_asm_len): Return always "" instead of void.
> (avr_out_lpm, avr_out_lpm_no_lpmx, avr_out_xload,
> avr_find_unused_d_reg): New static functions.
> (output_movqi, output_movhi, output_movsisf, avr_out_movpsi): Call
> avr_out_lpm to handle loads from progmem.
> (print_operand): Hande CONST_STRING.
> (avr_xload_libgcc_p): New static function.
> (avr_progmem_p): Test if decl is in flash.
> (avr_pgm_pointer_const_p): New static function.
> (avr_nonconst_pointer_addrspace): New static function.
> (avr_pgm_check_var_decl): New static function.
> (avr_insert_attributes): Use it. Change error message to report
> cause (progmem or address space) when code wants to write to flash.
> (avr_section_type_flags): Unset section flag SECTION_BSS for
> data in progmem.
> (adjust_insn_length): Handle ADJUST_LEN_XLOAD.
> (avr_const_address_lo16): New static function.
> (avr_assemble_integer): Use it to handle 3-byte integers.
> (avr_file_star): Print set for __RAMPZ__.
> (avr_emit_move): New function.
>
> * config/avr/predicates.md (hh8_operand): New predicate.
> (nop_general_operand): New predicate.
> (nox_general_operand): New predicate.
> * config/avr/avr.md (LPM_REGNO): New define_constants.
> (adjust_len): Add xload.
> (load<mode>_libgcc): New expander.
> (xload8_A, xload<mode>_A, *xload.<mode>): New insns.
> (*load.<mode>.libgcc, *xload.qi, *xload.<mode>.libgcc): New
> insn-and-splits.
> (mov<mode>): Call avr_emit_move to expand.
> (movmemhi): Ditto. Propagate address space information to newly
> created MEM.
> (movqi_insn, *movhi, *movpsi, *movsi, *movsf): Change predicate #1
> to nox_general_operand.
> (ashrqi3, ashrhi3, ashrsi3): Change predicate #1 to nop_general_operand.
> (ashlqi3, *ashlqi3, ashlhi3, ashlsi3): Ditto.
> (lshrqi3, *lshrqi3, lshrhi3, lshrsi3): Ditto.
> (split-lpmx): New split.
> (*ashlhi3_const, *ashlsi3_const, *ashrhi3_const, *ashrsi3_const,
> *lshrhi3_const, *lshrsi3_const): Indent, unquote C.
> (n_extendhipsi2): New insn-and-split.
>
> libgcc/
> PR target/49868
> * config/avr/t-avr (LIB1ASMFUNCS): Add _load_3, _load_4,
> _xload_2 _xload_3 _xload_4.
> * config/avr/lib1funcs.S (__load_3, __load_4, __xload_2,
> __xload_3, __xload_4): New functions.
This is a follow-up patch atop
http://gcc.gnu.org/ml/gcc-patches/2011-11/msg00806.html
Changes are:
* avr_out_lpm_no_lpmx must not reset *plen
* n_extendhipsi2 is rewritten so that hh8_operand is not needed any more
* fix an assertion in avr_out_lpm
* Rewrite of avr_find_unused_d_reg as follows
Johann
--
static rtx
avr_find_unused_d_reg (rtx insn, rtx exclude)
{
int regno;
bool isr_p = (interrupt_function_p (current_function_decl)
|| signal_function_p (current_function_decl));
for (regno = 16; regno < 32; regno++)
{
rtx reg = all_regs_rtx[regno];
if ((exclude
&& reg_overlap_mentioned_p (exclude, reg))
|| fixed_regs[regno])
{
continue;
}
/* Try non-live register */
if (!df_regs_ever_live_p (regno)
&& (TREE_THIS_VOLATILE (current_function_decl)
|| cfun->machine->is_OS_task
|| cfun->machine->is_OS_main
|| (!isr_p && call_used_regs[regno])))
{
return reg;
}
/* Any live register can be used if it is unused after.
Prologue/epilogue will care for it as needed. */
if (df_regs_ever_live_p (regno)
&& reg_unused_after (insn, reg))
{
return reg;
}
}
return NULL_RTX;
}
diff --git a/config/avr/avr.c b/config/avr/avr.c
index 8a942c1..c12234e 100644
--- a/config/avr/avr.c
+++ b/config/avr/avr.c
@@ -2300,28 +2300,58 @@ avr_xload_libgcc_p (enum machine_mode mode)
}
+/* Find an unused d-register to be used as scratch in INSN.
+ EXCLUDE is either NULL_RTX or some register. In the case where EXCLUDE
+ is a register, skip all possible return values that overlap EXCLUDE.
+ The policy for the returned register is similar to that of
+ `reg_unused_after', i.e. the returned register may overlap the SET_DEST
+ of INSN.
+
+ Return a QImode d-register or NULL_RTX if nothing found. */
+
static rtx
avr_find_unused_d_reg (rtx insn, rtx exclude)
{
int regno;
+ bool isr_p = (interrupt_function_p (current_function_decl)
+ || signal_function_p (current_function_decl));
- for (regno = 16; regno < 32; regno ++)
+ for (regno = 16; regno < 32; regno++)
{
rtx reg = all_regs_rtx[regno];
- if (exclude
- && reg_overlap_mentioned_p (exclude, reg))
+ if ((exclude
+ && reg_overlap_mentioned_p (exclude, reg))
+ || fixed_regs[regno])
{
continue;
}
- if (reg_unused_after (insn, reg))
- return reg;
+ /* Try non-live register */
+
+ if (!df_regs_ever_live_p (regno)
+ && (TREE_THIS_VOLATILE (current_function_decl)
+ || cfun->machine->is_OS_task
+ || cfun->machine->is_OS_main
+ || (!isr_p && call_used_regs[regno])))
+ {
+ return reg;
+ }
+
+ /* Any live register can be used if it is unused after.
+ Prologue/epilogue will care for it as needed. */
+
+ if (df_regs_ever_live_p (regno)
+ && reg_unused_after (insn, reg))
+ {
+ return reg;
+ }
}
return NULL_RTX;
}
+
/* Helper function for the next function in the case where only restricted
version of LPM instruction is available. */
@@ -2354,7 +2384,7 @@ avr_out_lpm_no_lpmx (rtx insn, rtx *xop, int *plen)
case 1:
return avr_asm_len ("%4lpm" CR_TAB
- "mov %0,%3", xop, plen, -2);
+ "mov %0,%3", xop, plen, 2);
case 2:
if (REGNO (dest) == REG_Z)
@@ -2363,14 +2393,14 @@ avr_out_lpm_no_lpmx (rtx insn, rtx *xop, int *plen)
"adiw %2,1" CR_TAB
"%4lpm" CR_TAB
"mov %B0,%3" CR_TAB
- "pop %A0", xop, plen, -6);
+ "pop %A0", xop, plen, 6);
else
{
avr_asm_len ("%4lpm" CR_TAB
"mov %A0,%3" CR_TAB
"adiw %2,1" CR_TAB
"%4lpm" CR_TAB
- "mov %B0,%3", xop, plen, -5);
+ "mov %B0,%3", xop, plen, 5);
if (!reg_unused_after (insn, addr))
avr_asm_len ("sbiw %2,1", xop, plen, 1);
@@ -2386,7 +2416,7 @@ avr_out_lpm_no_lpmx (rtx insn, rtx *xop, int *plen)
"mov %B0,%3" CR_TAB
"adiw %2,1" CR_TAB
"%4lpm" CR_TAB
- "mov %C0,%3", xop, plen, -8);
+ "mov %C0,%3", xop, plen, 8);
if (REGNO (dest) != REG_Z - 2
|| !reg_unused_after (insn, addr))
@@ -2402,7 +2432,7 @@ avr_out_lpm_no_lpmx (rtx insn, rtx *xop, int *plen)
"adiw %2,1" CR_TAB
"%4lpm" CR_TAB
"mov %B0,%3" CR_TAB
- "adiw %2,1", xop, plen, -6);
+ "adiw %2,1", xop, plen, 6);
if (REGNO (dest) == REG_Z - 2)
return avr_asm_len ("%4lpm" CR_TAB
@@ -2435,7 +2465,7 @@ avr_out_lpm_no_lpmx (rtx insn, rtx *xop, int *plen)
avr_asm_len ("%4lpm" CR_TAB
"mov %A0,%3" CR_TAB
- "adiw %2,1", xop, plen, -3);
+ "adiw %2,1", xop, plen, 3);
if (n_bytes >= 2)
avr_asm_len ("%4lpm" CR_TAB
@@ -2492,7 +2522,8 @@ avr_out_lpm (rtx insn, rtx *op, int *plen)
segment = avr_pgm_segment (MEM_ADDR_SPACE (src));
gcc_assert (REG_P (dest)
- && ((REG_P (addr) && segment >= 0)
+ && ((segment >= 0
+ && (REG_P (addr) || POST_INC == GET_CODE (addr)))
|| (GET_CODE (addr) == LO_SUM && segment == -1)));
if (segment == -1)
@@ -2515,6 +2546,8 @@ avr_out_lpm (rtx insn, rtx *op, int *plen)
segment %= avr_current_arch->n_segments;
+ /* Set RAMPZ as needed. */
+
if (segment == 0)
{
xop[4] = xstring_empty;
@@ -9430,7 +9463,7 @@ avr_reg_ok_for_pgm_addr (rtx reg, bool strict)
/* Avoid combine to propagate hard regs. */
- if (can_create_pseude_p()
+ if (can_create_pseudo_p()
&& REGNO (reg) < REG_Z)
{
return false;
@@ -9555,17 +9588,24 @@ avr_addr_space_convert (rtx src, tree type_from, tree type_to)
if (as_from != ADDR_SPACE_PGMX
&& as_to == ADDR_SPACE_PGMX)
{
+ int n_segments = avr_current_arch->n_segments;
+
src = force_reg (Pmode, src);
if (ADDR_SPACE_GENERIC_P (as_from)
- || as_from == ADDR_SPACE_PGM)
+ || as_from == ADDR_SPACE_PGM
+ || n_segments == 1)
{
return gen_rtx_ZERO_EXTEND (PSImode, src);
}
else
{
- int hh8 = avr_pgm_segment (as_from) << 16;
- src = SET_SRC (gen_n_extendhipsi2 (src, src, GEN_INT (hh8)));
+ rtx new_src = gen_reg_rtx (PSImode);
+ int segment = avr_pgm_segment (as_from) % n_segments;
+
+ emit_insn (gen_n_extendhipsi2 (new_src, GEN_INT (segment), src));
+
+ return new_src;
}
}
diff --git a/config/avr/avr.md b/config/avr/avr.md
index 3656eef..d341755 100644
--- a/config/avr/avr.md
+++ b/config/avr/avr.md
@@ -3845,18 +3845,24 @@
})
(define_insn_and_split "n_extendhipsi2"
- [(set (match_operand:PSI 0 "register_operand" "=d")
- (ior:PSI (zero_extend:PSI (match_operand:HI 1 "register_operand" "r"))
- (match_operand:PSI 2 "hh8_operand" "n")))]
+ [(set (match_operand:PSI 0 "register_operand" "=r,r,d,r")
+ (lo_sum:PSI (match_operand:QI 1 "const_int_operand" "L,P,n,n")
+ (match_operand:HI 2 "register_operand" "r,r,r,r")))
+ (clobber (match_scratch:QI 3 "=X,X,X,&d"))]
""
"#"
"reload_completed"
- [(set (match_dup 3) (match_dup 1))
- (set (match_dup 4) (match_dup 5))]
+ [(set (match_dup 4) (match_dup 2))
+ (set (match_dup 3) (match_dup 6))
+ ; no-op move in the case where no scratch is needed
+ (set (match_dup 5) (match_dup 3))]
{
- operands[3] = simplify_gen_subreg (HImode, operands[0], PSImode, 0);
- operands[4] = simplify_gen_subreg (QImode, operands[0], PSImode, 2);
- operands[5] = simplify_gen_subreg (QImode, operands[2], PSImode, 2);
+ operands[4] = simplify_gen_subreg (HImode, operands[0], PSImode, 0);
+ operands[5] = simplify_gen_subreg (QImode, operands[0], PSImode, 2);
+ operands[6] = operands[1];
+
+ if (GET_CODE (operands[3]) == SCRATCH)
+ operands[3] = operands[5];
})
(define_insn_and_split "zero_extendhisi2"
diff --git a/config/avr/predicates.md b/config/avr/predicates.md
index f2a6a70..2db6f3b 100644
--- a/config/avr/predicates.md
+++ b/config/avr/predicates.md
@@ -249,8 +249,3 @@
(define_predicate "o16_operand"
(and (match_code "const_int")
(match_test "IN_RANGE (INTVAL (op), -(1<<16), -1)")))
-
-;; CONST_INT were ony sub-byte #2 may have non-zero value.
-(define_predicate "hh8_operand"
- (and (match_code "const_int")
- (match_test "IN_RANGE (INTVAL (op), 0x00010000, 0x00ff0000)")))