PR 35232: Recent regression due to reload inheritance bug

[Ulrich Weigand]

Richard Sandiford wrote:

Sorry for the late review, I've unfortunately been quite busy
over the last couple of weeks, and this was a big patch ;-)

>The fix I've gone for is to introduce two new arrays,
>reload_reg_rtx_for_input and reload_reg_rtx_for_output.
>do_input_reload sets reload_reg_rtx_for_input[r] to the
>input form of rld[r].reg_rtx and do_output_reload sets
>reload_reg_rtx_for_output[r] to the output form.

OK, this definitely seems like the right thing to do.

>Er... well, that wasn't short.  I hope it doesn't put anyone off.
>I think this is actually a relatively straight-forward change as
>far as reload changes go.  I just wanted to describe it in even
>more painful detail than usual because of the late stage we're at.

Thanks for the detailed description of the changes, this was
very helpful.


The patch looks good to me, just a couple of questions and/or
suggestions:

emit_input_reload_insns has an initialization of reloadreg:
   rtx reloadreg = rl->reg_rtx;
which is now dead and should be removed.

>@@ -7194,7 +7162,7 @@ emit_output_reload_insns (struct insn_ch
> 		     made; leave new_spill_reg_store alone.  */
> 		  ;
> 		else if (s >= 0
>-			 && SET_SRC (set) == rl->reg_rtx
>+			 && SET_SRC (set) == rl_reg_rtx
> 			 && SET_DEST (set) == rld[s].reg_rtx)

What about cases where the secondary reload register also had
its mode adjusted?  Can this actually happen?

>   old = rl->out_reg;
>   if (old == 0
>-      || rl->reg_rtx == old
>-      || rl->reg_rtx == 0)
>+      || reg_rtx == old
>+      || reg_rtx == 0)
>     return;

I'm wondering whether == compares actually work here (and elsewhere),
as reload_adjust_reg_for_mode may allocate fresh rtx's ...


>@@ -7414,9 +7449,13 @@ emit_reload_insns (struct insn_chain *ch
> 
>   for (j = 0; j < n_reloads; j++)
>     {
>-      if (rld[j].reg_rtx
>-	  && REGNO (rld[j].reg_rtx) < FIRST_PSEUDO_REGISTER)
>-	new_spill_reg_store[REGNO (rld[j].reg_rtx)] = 0;
>+      if (rld[j].reg_rtx && HARD_REGISTER_P (rld[j].reg_rtx))
>+	{
>+	  unsigned int i;
>+
>+	  for (i = REGNO (rld[j].reg_rtx); i < END_REGNO (rld[j].reg_rtx); i++)
>+	    new_spill_reg_store[i] = 0;
>+	}

Maybe just memset'ing new_spill_reg_store to zero at the start
of emit_reload_insns would be clearer?


>+	  /* Maybe the spill reg contains a copy of reload_out.  */
>+	  if (rld[r].out != 0
>+	      && (REG_P (rld[r].out)
> #ifdef AUTO_INC_DEC
>-		      || ! rld[r].out_reg
>+		  || ! rld[r].out_reg
> #endif
>-		      || REG_P (rld[r].out_reg)))
>+		  || REG_P (rld[r].out_reg)))
>+	    {
>+	      rtx src;
>+	      enum machine_mode mode;
>+
>+	      src = reload_reg_rtx_for_output[r];
>+	      mode = GET_MODE (src);
>+	      i = REGNO (src);
>+	      nr = hard_regno_nregs[i][mode];

Redefining i and nr in the middle of this function strikes me as a
bit confusing, maybe it would be better to use different variable
names here ...

>+	  /* Maybe the spill reg contains a copy of reload_in.  Only do
>+	     something if there will not be an output reload for
>+	     the register being reloaded.  */
>+	  else if (rld[r].out_reg == 0
>+		   && rld[r].in != 0
>+		   && ((REG_P (rld[r].in)
>+			&& !HARD_REGISTER_P (rld[r].in)
>+			&& !REGNO_REG_SET_P (&reg_has_output_reload,
>+					     REGNO (rld[r].in)))
>+		       || (REG_P (rld[r].in_reg)
>+			   && !REGNO_REG_SET_P (&reg_has_output_reload,
>+						REGNO (rld[r].in_reg))))
>+		   && !reg_set_p (reload_reg_rtx_for_input[r], PATTERN (insn)))
>+	    {
>+	      rtx dest;
>+	      enum machine_mode mode;
>+
>+	      dest = reload_reg_rtx_for_input[r];

Why do you call the variable "dest" here and not "src" as above?
It is still the (potential) source of a future reload inheritance ...


>@@ -7687,17 +7718,18 @@ emit_reload_insns (struct insn_chain *ch
> 	 it thinks only about the original insn.  So invalidate it here.
> 	 Also do the same thing for RELOAD_OTHER constraints where the
> 	 output is discarded.  */
>-      if (i < 0 
>-	  && ((rld[r].out != 0
>-	       && (REG_P (rld[r].out)
>-		   || (MEM_P (rld[r].out)
>+      else if (i < 0
>+	       && ((rld[r].out != 0
>+		    && (REG_P (rld[r].out)
>+			|| (MEM_P (rld[r].out)
>+			    && REG_P (rld[r].out_reg))))

Why is the "else" necessary?


Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com