| Message ID | 4C6D56F2.3060204@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
On 08/19/2010 09:08 AM, Andrew Stubbs wrote: > This patch creates a new instruction pattern "use_initial_val". > > The purpose of this patch is to allow late optimization passes to use > re-use existing pseudo-registers, even if the use of the register has > been optimized away in earlier passes. What is a target intended to do in this use_initial_val pattern? Looking forward to 3/3, it seems you emit a fancy USE insn, whose purpose appears to be simply to extend the lifetime of the pseudo, without actually using it in a specific pattern. Without looking deeper, it seems this is a mistake. Can you explain further when and why this is required? r~
On 08/19/2010 08:18 PM, Richard Henderson wrote: > On 08/19/2010 09:08 AM, Andrew Stubbs wrote: >> This patch creates a new instruction pattern "use_initial_val". >> >> The purpose of this patch is to allow late optimization passes to use >> re-use existing pseudo-registers, even if the use of the register has >> been optimized away in earlier passes. > > What is a target intended to do in this use_initial_val pattern? > > Looking forward to 3/3, it seems you emit a fancy USE insn, whose > purpose appears to be simply to extend the lifetime of the pseudo, > without actually using it in a specific pattern. Yes. It's an unspec which later gets turned into a no-op. > Without looking deeper, it seems this is a mistake. > Can you explain further when and why this is required? We get a pseudo for an initial value during rtl expansion. All uses of the pseudo are optimized away. Later, in optimize_mode_switching, we introduce new references to the pseudo, but the initializing insn has been deleted. Bernd
On 08/19/2010 11:30 AM, Bernd Schmidt wrote: >> Looking forward to 3/3, it seems you emit a fancy USE insn, whose >> purpose appears to be simply to extend the lifetime of the pseudo, >> without actually using it in a specific pattern. > > Yes. It's an unspec which later gets turned into a no-op. Looking at the split, it really does turn in to a nop insn, not being optimized away entirely. Is that really intended? If so, I think that deserves a comment. What advantage does this unspec have over a (USE reg) insn? I'm somewhat surprised that a free-floating use or unspec does what you want better than actually using the pseudo in some insn pattern that actually requires it. E.g. by adding it to the CALL_INSN_FUNCTION_USAGE where optimize_mode_switching can find it. Can you tell me which part of mode switching ends up requiring the pseudo? Scanning the 3/3 patch, it's non-obvious. r~
On 08/19/2010 08:42 PM, Richard Henderson wrote: > On 08/19/2010 11:30 AM, Bernd Schmidt wrote: >>> Looking forward to 3/3, it seems you emit a fancy USE insn, whose >>> purpose appears to be simply to extend the lifetime of the pseudo, >>> without actually using it in a specific pattern. >> >> Yes. It's an unspec which later gets turned into a no-op. > > Looking at the split, it really does turn in to a nop insn, not > being optimized away entirely. Is that really intended? If so, > I think that deserves a comment. AFAIR no code is output for it. If that's not true, then that at least needs to be revisited. Does the splitting code handle the case where we return no insn at all? > What advantage does this unspec have over a (USE reg) insn? I wasn't certain a USE wouldn't just get deleted, now or in the future, if it was the only remaining use of a pseudo. > I'm somewhat surprised that a free-floating use or unspec does > what you want better than actually using the pseudo in some > insn pattern that actually requires it. E.g. by adding it to > the CALL_INSN_FUNCTION_USAGE where optimize_mode_switching can > find it. > > Can you tell me which part of mode switching ends up requiring > the pseudo? Scanning the 3/3 patch, it's non-obvious. I don't have the build trees anymore, and I don't recall all the details - but I think it was generating a reference to a symbol, which required the PIC register. I believe it was __fpscr_values - see emit_fpu_switch in sh.c. For FD-PIC, the incoming PIC reg is loaded into a pseudo with the initial values machinery. The problem occurs if during expand we already had something that required the PIC register, so the pseudo was set up, but all the uses (and hence the initializing insn) were optimized away. So there really isn't anywhere to put it anymore in this case. Bernd
On Thu, Aug 19, 2010 at 08:55:20PM +0200, Bernd Schmidt wrote: > For FD-PIC, the incoming PIC reg is loaded into a pseudo with the > initial values machinery. The problem occurs if during expand we > already had something that required the PIC register, so the pseudo was > set up, but all the uses (and hence the initializing insn) were > optimized away. So there really isn't anywhere to put it anymore in > this case. FWIW, I once tried to make the MIPS pic base into a pseudo, and encountered this same problem - I eventually gave up, but maybe it can be done the same way.
On 08/19/2010 11:55 AM, Bernd Schmidt wrote: > AFAIR no code is output for it. If that's not true, then that at least > needs to be revisited. Does the splitting code handle the case where we > return no insn at all? Yes. Lots of other ports do that regularly. I think the correct fix is (define_insn_and_split "use_initial_val" [(unspec [(match_operand 0 "pseudo_register_operand" "r")] UNSPEC_INITVAL)] "" "#" "reload_completed" [(const_int 0)]) + { DONE; }) Otherwise you'll split to (const_int 0), which is defined elsewhere as the "nop" pattern. You can't elide the [] block from the split pattern though; you have to have something there. >> What advantage does this unspec have over a (USE reg) insn? > > I wasn't certain a USE wouldn't just get deleted, now or in the future, > if it was the only remaining use of a pseudo. I'm pretty sure they get deleted during/after reload, but they hang around until then. > I don't have the build trees anymore, and I don't recall all the details > - but I think it was generating a reference to a symbol, which required > the PIC register. I believe it was __fpscr_values - see emit_fpu_switch > in sh.c. That does seem like a likely candidate. Which means that the new use can occur from essentially any random FP insn. I agree that you'd not want to annotate each and every one of those. I wonder if a better solution might be to enhance df_get_exit_block_use_set to keep your pseudo alive for the entire function? We currently have EPILOGUE_USES, but that's limited to hard registers. I wonder if the macro could be replaced by a hook that, instead of returning true for any specific regno, sets bits in a bitset. You'd want to have the hook disable the forced use sometime just reload, presumably, in order to let the pseudo die properly. I don't know enough about the current state of the transition to df-scan to know if you'd need to rebuild REG_DEAD notes before reload, or what. r~
On 08/19/2010 12:33 PM, Daniel Jacobowitz wrote: > On Thu, Aug 19, 2010 at 08:55:20PM +0200, Bernd Schmidt wrote: >> For FD-PIC, the incoming PIC reg is loaded into a pseudo with the >> initial values machinery. The problem occurs if during expand we >> already had something that required the PIC register, so the pseudo was >> set up, but all the uses (and hence the initializing insn) were >> optimized away. So there really isn't anywhere to put it anymore in >> this case. > > FWIW, I once tried to make the MIPS pic base into a pseudo, and > encountered this same problem - I eventually gave up, but maybe it can > be done the same way. Yeah, same with Alpha. In the end I left it as a fixed register, and use peep2's to avoid reloading it after calls when it's dead. Indeed, I don't even expose the use of the PIC register before reload is complete; the optimizers are much happier with a bare (symbol_ref "sdata_sym") than they are with an expression that involves the pic register. SH has only 16 registers though, so I can understand the strong desire to retain the register allocation freedom. r~
2010-08-19 Bernd Schmidt <bernds@codesourcery.com> gcc/ * Makefile.in: (mode-switching.o): Depend on integrate.h. * doc/md.texi (Standard Pattern Names For Generation): Document use_initial_val. * integrate.c (initial_value_pair): Add new member "initialized". (get_hard_reg_initial_val): Set initialized. (emit_initial_value_sets): Emit result of gen_use_initial_val. * mode-switching.c: Include integrate.h. (rest_of_handle_mode_switching): Call emit_initial_value_sets. --- src/gcc-mainline/gcc/Makefile.in | 3 ++- src/gcc-mainline/gcc/doc/md.texi | 7 +++++++ src/gcc-mainline/gcc/integrate.c | 23 ++++++++++++++++++----- src/gcc-mainline/gcc/mode-switching.c | 2 ++ 4 files changed, 29 insertions(+), 6 deletions(-) diff --git a/src/gcc-mainline/gcc/Makefile.in b/src/gcc-mainline/gcc/Makefile.in index 49b0634..4c035a7 100644 --- a/src/gcc-mainline/gcc/Makefile.in +++ b/src/gcc-mainline/gcc/Makefile.in @@ -3119,7 +3119,8 @@ lcm.o : lcm.c $(CONFIG_H) $(SYSTEM_H) coretypes.h $(TM_H) $(RTL_H) $(REGS_H) \ mode-switching.o : mode-switching.c $(CONFIG_H) $(SYSTEM_H) coretypes.h \ $(TM_H) $(RTL_H) $(REGS_H) hard-reg-set.h $(FLAGS_H) insn-config.h \ $(INSN_ATTR_H) $(RECOG_H) $(BASIC_BLOCK_H) $(TM_P_H) $(FUNCTION_H) \ - output.h $(TREE_PASS_H) $(TIMEVAR_H) $(DF_H) $(TARGET_H) $(EMIT_RTL_H) + output.h $(TREE_PASS_H) $(TIMEVAR_H) $(DF_H) $(TARGET_H) $(EMIT_RTL_H) \ + integrate.h tree-ssa-dce.o : tree-ssa-dce.c $(CONFIG_H) $(SYSTEM_H) $(TREE_H) \ $(TREE_FLOW_H) $(DIAGNOSTIC_H) $(TIMEVAR_H) $(TM_H) \ coretypes.h $(TREE_DUMP_H) $(TREE_PASS_H) $(FLAGS_H) $(BASIC_BLOCK_H) \ diff --git a/src/gcc-mainline/gcc/doc/md.texi b/src/gcc-mainline/gcc/doc/md.texi index fd8423a..bc488eb 100644 --- a/src/gcc-mainline/gcc/doc/md.texi +++ b/src/gcc-mainline/gcc/doc/md.texi @@ -5196,6 +5196,13 @@ The @code{sibcall_epilogue} pattern must not clobber any arguments used for parameter passing or any stack slots for arguments passed to the current function. +@cindex @code{use_initial_val} instruction pattern +@item @samp{use_initial_val} +This pattern, if defined, emits an insn that shows use a pseudo register +created by @code{get_hard_reg_initial_val}. This is useful if a port may +introduce additional uses of this pseudo in late optimization passes, when +the initial ones may already have been optimized away. + @cindex @code{trap} instruction pattern @item @samp{trap} This pattern, if defined, signals an error, typically by causing some diff --git a/src/gcc-mainline/gcc/integrate.c b/src/gcc-mainline/gcc/integrate.c index dd75758..9a5c364 100644 --- a/src/gcc-mainline/gcc/integrate.c +++ b/src/gcc-mainline/gcc/integrate.c @@ -56,6 +56,7 @@ along with GCC; see the file COPYING3. If not see typedef struct GTY(()) initial_value_pair { rtx hard_reg; rtx pseudo; + bool initialized; } initial_value_pair; typedef struct GTY(()) initial_value_struct { int num_entries; @@ -239,6 +240,7 @@ get_hard_reg_initial_val (enum machine_mode mode, unsigned int regno) { struct initial_value_struct *ivs; rtx rv; + int entry; rv = has_hard_reg_initial_val (mode, regno); if (rv) @@ -254,17 +256,19 @@ get_hard_reg_initial_val (enum machine_mode mode, unsigned int regno) crtl->hard_reg_initial_vals = ivs; } - if (ivs->num_entries >= ivs->max_entries) + entry = ivs->num_entries++; + if (entry >= ivs->max_entries) { ivs->max_entries += 5; ivs->entries = GGC_RESIZEVEC (initial_value_pair, ivs->entries, ivs->max_entries); } - ivs->entries[ivs->num_entries].hard_reg = gen_rtx_REG (mode, regno); - ivs->entries[ivs->num_entries].pseudo = gen_reg_rtx (mode); + ivs->entries[entry].hard_reg = gen_rtx_REG (mode, regno); + ivs->entries[entry].pseudo = gen_reg_rtx (mode); + ivs->entries[entry].initialized = false; - return ivs->entries[ivs->num_entries++].pseudo; + return ivs->entries[entry].pseudo; } /* See if get_hard_reg_initial_val has been used to create a pseudo @@ -299,7 +303,16 @@ emit_initial_value_sets (void) start_sequence (); for (i = 0; i < ivs->num_entries; i++) - emit_move_insn (ivs->entries[i].pseudo, ivs->entries[i].hard_reg); + { + if (ivs->entries[i].initialized) + continue; + ivs->entries[i].initialized = true; + emit_move_insn (ivs->entries[i].pseudo, ivs->entries[i].hard_reg); +#ifdef HAVE_use_initial_val + if (HAVE_use_initial_val) + emit_insn (gen_use_initial_val (ivs->entries[i].pseudo)); +#endif + } seq = get_insns (); end_sequence (); diff --git a/src/gcc-mainline/gcc/mode-switching.c b/src/gcc-mainline/gcc/mode-switching.c index 306fb5d..ce7e4d1 100644 --- a/src/gcc-mainline/gcc/mode-switching.c +++ b/src/gcc-mainline/gcc/mode-switching.c @@ -33,6 +33,7 @@ along with GCC; see the file COPYING3. If not see #include "output.h" #include "tm_p.h" #include "function.h" +#include "integrate.h" #include "tree-pass.h" #include "timevar.h" #include "df.h" @@ -753,6 +754,7 @@ rest_of_handle_mode_switching (void) { #ifdef OPTIMIZE_MODE_SWITCHING optimize_mode_switching (); + emit_initial_value_sets (); #endif /* OPTIMIZE_MODE_SWITCHING */ return 0; }