Patchwork [1/3] SH-2A FDPIC: New pattern: use_initial_val

login
register
mail settings
Submitter Andrew Stubbs
Date Aug. 19, 2010, 4:08 p.m.
Message ID <4C6D56F2.3060204@codesourcery.com>
Download mbox | patch
Permalink /patch/62190/
State New
Headers show

Comments

Andrew Stubbs - Aug. 19, 2010, 4:08 p.m.
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.

This feature is required by our SH-2A FDPIC (to follow), but has been 
split out here because it is target independent.

OK to commit?

Andrew Stubbs
Richard Henderson - Aug. 19, 2010, 6:18 p.m.
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~
Bernd Schmidt - Aug. 19, 2010, 6:30 p.m.
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
Richard Henderson - Aug. 19, 2010, 6:42 p.m.
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~
Bernd Schmidt - Aug. 19, 2010, 6:55 p.m.
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
Daniel Jacobowitz - Aug. 19, 2010, 7:33 p.m.
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.
Richard Henderson - Aug. 19, 2010, 7:34 p.m.
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~
Richard Henderson - Aug. 19, 2010, 7:42 p.m.
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~

Patch

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;
 }