Patchwork RFA: fix rtl-optimization/56833

login
register
mail settings
Submitter Joern Rennecke
Date May 22, 2013, 10:45 a.m.
Message ID <20130522064532.lk8zcp0dkowc8o4w-nzlynne@webmail.spamcop.net>
Download mbox | patch
Permalink /patch/245585/
State New
Headers show

Comments

Joern Rennecke - May 22, 2013, 10:45 a.m.
Quoting Eric Botcazou <ebotcazou@adacore.com>:

>> The problem was that we had some optimzations added to the
>> reload_cse_move2add pass that would attempt transformations with
>> multi-hard-register registers, without keeping track of the validity of the
>> values in all hard registers involved.
>
> That's not clear to me: for example in move2add_note_store, we reset the info
> about any multi hard-register; moveover 5 arrays are supposed to be   
> maintained
> for each hard-register:
>
>   for (i = FIRST_PSEUDO_REGISTER - 1; i >= 0; i--)
>     {
>       reg_set_luid[i] = 0;
>       reg_offset[i] = 0;
>       reg_base_reg[i] = 0;
>       reg_symbol_ref[i] = NULL_RTX;
>       reg_mode[i] = VOIDmode;
>     }
> so I'm not sure whether we really properly handle multi hard-registers.

Initializing all the values is nice to have defined contents, but
the values in reg_offset[i] / reg_base_reg[i] / reg_symbol_ref[i]
just tell what's in these registers if they are valid, not if they
are valid.
But I can see that there could be a problem with an earlier value
that used to be valid in a multi-hard-register sub register to be
considered to be still valid.
Setting the mode of every constituent register but the first one
(which has the new value recorded) to VOIDmode at the same time
as updating reg_set_luid should be sufficent to address this issue.

> So
> what about explicitly punting for multi hard-registers in   
> reload_cse_move2add?
> Do you think that this would pessimize too much in practice?

The pass was originally written with word_mode == Pmode targets like
the SH in mind, where multi-hard-register values are uninteresting.

But for targets like the avr, most or all of the interesting values
will be in multi-hard-register registers.
2013-05-22  Joern Rennecke <joern.rennecke@embecosm.com>

	PR rtl-optimization/56833
	* postreload.c (move2add_use_add2_insn): Update reg_set_luid
	and reg_mode for all affected hard registers.
	(move2add_use_add3_insn): Likewise.  Check that all source hard
	regs have been set by the same set.
	(reload_cse_move2add): Before concluding that we have a pre-existing
	value, check that all destination hard registers have been set by the
	same set.
Eric Botcazou - May 23, 2013, 3 p.m.
> But I can see that there could be a problem with an earlier value
> that used to be valid in a multi-hard-register sub register to be
> considered to be still valid.
> Setting the mode of every constituent register but the first one
> (which has the new value recorded) to VOIDmode at the same time
> as updating reg_set_luid should be sufficent to address this issue.

Agreed.

> The pass was originally written with word_mode == Pmode targets like
> the SH in mind, where multi-hard-register values are uninteresting.
> 
> But for targets like the avr, most or all of the interesting values
> will be in multi-hard-register registers.

The patch is OK on principle, but can you factor out the common code?  The 
endings of move2add_use_add2_insn and move2add_use_add3_insn are identical so 
it would be nice to have e.g. a record_reg_value helper function and call it 
from there.  Similarly, the 3 new checks look strictly identical.
Joern Rennecke - May 23, 2013, 8:33 p.m.
Quoting Eric Botcazou <ebotcazou@adacore.com>:

> The patch is OK on principle, but can you factor out the common code?  The
> endings of move2add_use_add2_insn and move2add_use_add3_insn are identical so
> it would be nice to have e.g. a record_reg_value helper function and call it
> from there.  Similarly, the 3 new checks look strictly identical.

Looking into sharing the code with sites that perform essentially the same
function but look somewhat different, I see there's a problem with using
only reg_set_luid to indicate the consistency of a multi-hard-reg-value
in these other contexts.
For values that are use a base register, the reg_set_luid is the same as
for the base register; for constants, it is the same for all constants
set since the last label.

Say we have reg size 8 bit, base r0, and then
(set reg:HI 2 (plus:SI (reg:HI 0) (const_int 500))
...
(set reg:HI 3 (plus:SI (reg:HI 0) (const_int 500))

Now how do we tell that the value in r2 is no longer valid?

As the example shows, trying to replicate the recorded value across all hard
regs is pointless, as we still need to make sure that we have a still-valid
start register.
OTOH, this ties in nicely with setting the mode of subsequent registers
to VOIDmode.  We can verify the mode to make sure there was no more recent
set of any constituent register.  The check of the extra luids thus becomes
superflous, as becomes the set.
This logic relies on multi-hard register regs to be allocated contigously...
But if we'd want to change that, there'd be a lot more code that would
need changing.

Patch

Index: postreload.c
===================================================================
--- postreload.c	(revision 199190)
+++ postreload.c	(working copy)
@@ -1749,7 +1749,13 @@  move2add_use_add2_insn (rtx reg, rtx sym
 	    }
 	}
     }
-  reg_set_luid[regno] = move2add_luid;
+  int i = hard_regno_nregs[regno][GET_MODE (reg)] -1;
+  do
+    {
+      reg_set_luid[regno + i] = move2add_luid;
+      reg_mode[regno + i] = VOIDmode;
+    }
+  while (--i >= 0);
   reg_base_reg[regno] = -1;
   reg_mode[regno] = GET_MODE (reg);
   reg_symbol_ref[regno] = sym;
@@ -1793,6 +1799,14 @@  move2add_use_add3_insn (rtx reg, rtx sym
 	&& reg_symbol_ref[i] != NULL_RTX
 	&& rtx_equal_p (sym, reg_symbol_ref[i]))
       {
+	int j;
+
+	for (j = hard_regno_nregs[i][reg_mode[i]] - 1;
+	     j > 0 && reg_set_luid[i+j] == reg_set_luid[i];)
+	  j--;
+	if (j != 0)
+	  continue;
+
 	rtx new_src = gen_int_mode (INTVAL (off) - reg_offset[i],
 				    GET_MODE (reg));
 	/* (set (reg) (plus (reg) (const_int 0))) is not canonical;
@@ -1835,7 +1849,13 @@  move2add_use_add3_insn (rtx reg, rtx sym
       if (validate_change (insn, &SET_SRC (pat), tem, 0))
 	changed = true;
     }
-  reg_set_luid[regno] = move2add_luid;
+  i = hard_regno_nregs[regno][GET_MODE (reg)] -1;
+  do
+    {
+      reg_set_luid[regno + i] = move2add_luid;
+      reg_mode[regno + i] = VOIDmode;
+    }
+  while (--i >= 0);
   reg_base_reg[regno] = -1;
   reg_mode[regno] = GET_MODE (reg);
   reg_symbol_ref[regno] = sym;
@@ -1890,7 +1910,10 @@  reload_cse_move2add (rtx first)
 
 	  /* Check if we have valid information on the contents of this
 	     register in the mode of REG.  */
-	  if (reg_set_luid[regno] > move2add_last_label_luid
+	  for (i = hard_regno_nregs[regno][GET_MODE (reg)] - 1;
+	       i > 0 && reg_set_luid[regno + i] == reg_set_luid[regno];)
+	    i--;
+	  if (i == 0 && reg_set_luid[regno] > move2add_last_label_luid
 	      && MODES_OK_FOR_MOVE2ADD (GET_MODE (reg), reg_mode[regno])
               && dbg_cnt (cse2_move2add))
 	    {
@@ -2021,7 +2044,10 @@  reload_cse_move2add (rtx first)
 
 	      /* If the reg already contains the value which is sum of
 		 sym and some constant value, we can use an add2 insn.  */
-	      if (reg_set_luid[regno] > move2add_last_label_luid
+	      for (i = hard_regno_nregs[regno][GET_MODE (reg)] - 1;
+		   i > 0 && reg_set_luid[regno + i] == reg_set_luid[regno];)
+		i--;
+	      if (i == 0 && reg_set_luid[regno] > move2add_last_label_luid
 		  && MODES_OK_FOR_MOVE2ADD (GET_MODE (reg), reg_mode[regno])
 		  && reg_base_reg[regno] < 0
 		  && reg_symbol_ref[regno] != NULL_RTX