Re: [patch] Fix PR rtl-optimization/54290

["Ulrich Weigand" <uweigand at de dot ibm dot com>]

Eric Botcazou wrote:

> Reload 1 of insn #85 inherits the reload reg from reload 1 of insn #84, the
> bug being that the same reload reg is also used for reload 0 of insn #85.
> 
> This is supposed to work like so: the inheritance code in choose_reload_regs
> calls free_for_value_p with IGNORE_ADDRESS_RELOADS set to 1 to bypass the
> conflict with reload 0 because there is special code in the same function to
> discard reload 0 in this case:
> 
> 	  /* If we can inherit a RELOAD_FOR_INPUT, or can use a
> 	     reload_override_in, then we do not need its related
> 	     RELOAD_FOR_INPUT_ADDRESS / RELOAD_FOR_INPADDR_ADDRESS reloads;
> 	     likewise for other reload types.
> 	     We handle this by removing a reload when its only replacement
> 	     is mentioned in reload_in of the reload we are going to inherit.
> 	     A special case are auto_inc expressions; even if the input is
> 	     inherited, we still need the address for the output.  We can
> 	     recognize them because they have RELOAD_OUT set to RELOAD_IN.
>             If we succeeded removing some reload and we are doing a preliminary
> 	     pass just to remove such reloads, make another pass, since the
> 	     removal of one reload might allow us to inherit another one.  */
> 	  else if (rld[r].in
> 		   && rld[r].out != rld[r].in
> 		   && remove_address_replacements (rld[r].in) && pass)
> 	    pass = 2;
> 
> But it is called with rld[r].in equal to:
> (gdb) p debug_rtx(in_rtx)
>   (reg:SI 40 %f8 [orig:109 prephitmp.12 ] [109])
> 
> and the following replacement for reload 0:
> (gdb) p debug_rtx(*replacements[0].where)
>   (plus:SI (reg/f:SI 30 %fp)
>         (const_int -6140 [0xffffffffffffe804]))
> 
> so loc_mentioned_in_p returns false and reload 0 isn't discarded.
> 
> The problem is that reload 0 is pushed because of SECONDARY_MEMORY_NEEDED,
> i.e. (reg:SI 40 %f8) is spilled to memory at the address
> (gdb) p debug_rtx(secondary_memlocs_elim[11][0])
>   (mem/c:SI (plus:SI (reg/f:SI 30 %fp)
>         (const_int -6140 [0xffffffffffffe804])) [0 S4 A32])
> 
> and replacements[0].where above points to within this MEM instead of IN_RTX.

This analysis looks correct to me.

> The attached patch fixes the problem (on the 4.6 branch) by also invoking 
> remove_address_replacements on the result of get_secondary_mem if a secondary 
> memory is needed.  The condition to detect it has been copied from push_reload 
> on the 4.6 branch, but it has already slightly changed on the mainline and I'm 
> not sure how to adjust it.

It is unfortunate that we need to re-do the test over and over ... longer term
it might be better to add a predicate to directly *check* whether we decided we
needed a secondary memory slot for the operand.

In any case, the change in the condition you noticed was introduced by a recent
patch by Bernd: http://gcc.gnu.org/ml/gcc-patches/2012-08/msg00171.html

It seems that we ought to use a similar test to what Bernd introduced in
gen_reload, that is, use the "replaced_subreg" routine on rld[r].in, check
whether the result is a hard register and use its REGNO_REG_CLASS.

Bernd, given that you worked on this recently, any other thoughts?

Bye,
Ulrich

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