Re: [sh PATCH] PR/27717, sh backend lies to reload about index registers

[Ian Lance Taylor <iant at google dot com>]

Paolo Bonzini <paolo.bonzini@lu.unisi.ch> writes:

> The audit trail includes a comment from Joern, which is the author of
> this patch, about what this patch really does.  My only change to this
> patch was to fix a typo which is also mentioned in the audit trail.
> 
> The patch was tested by Kaz Kojima:
> > I've tested Joern's patch with #7 tweak on i686-pc-linux-gnu,
> > x86 cross sh{64,4}-unknown-linux-gnu and x86 cross sh64-elf targets.
> > There are no bootstrap or build problems and the results of
> > regression tests look fine on all targets.
> >
> ... and I am in the progress of retesting it on i686-pc-linux-gnu
> (where it should actually have no effect whatsoever).  Ok to apply?
> 
> Paolo
> 
> :ADDPATCH sh+reload:
> 2006-04-12  Joern Rennecke  <joern.rennecke@st.com>
> 	    Paolo Bonzini  <bonzini@gnu.org>
> 
> 	* doc/tm.texi: Document new macro INDEX_REG_CLASS_FOR_MODE.
> 	* defaults.h: Provide definition using INDEX_REG_CLASS.
> 	* regrename.c (scan_rtx_reg, scan_rtx_address): Replace uses of
> 	INDEX_REG_CLASS with INDEX_REG_CLASS_FOR_MODE.
> 	(replace_oldest_value_reg, replace_oldest_value_addr): Likewise.
> 	* postreload.c (reload_cse_simplify_operands): Likewise.
> 	(reload_combine): Likewise.
> 	* reload.c (find_reloads_address, find_reloads_address_1): Likewise.
> 	* regclass.c (record_address_regs): Likewise.
> 	* config/sh/sh.h (INDEX_REG_CLASS): Replace with:
> 	(INDEX_REG_CLASS_FOR_MODE).

I'm confused by this patch.  The PR implies that the problem has to do
with different pointer sizes.  That is, INDEX_REG_CLASS_FOR_MODE needs
to use the mode in the pointers are being computed.

> @@ -607,7 +608,8 @@ scan_rtx_address (rtx insn, rtx *loc, en
>  	  }
>  
>  	if (locI)
> -	  scan_rtx_address (insn, locI, INDEX_REG_CLASS, action, mode);
> +	  scan_rtx_address (insn, locI, INDEX_REG_CLASS_FOR_MODE (GET_MODE (x)),
> +			    action, mode);
>  	if (locB)
>  	  scan_rtx_address (insn, locB, base_reg_class (mode, PLUS, index_code),
>  			    action, mode);

But here GET_MODE (x) is not the mode in which the address is being
computed.  It is the mode in which the memory is being accessed.  And
that is not what I under Joern's comment in the PR to mean.

So, what am I missing?

If this patch goes forward, I think the documentation needs to be
cleaned up, as I find it to be somewhat unclear.  Avoid subclauses
like "if it different from NO_REGS".  Instead add a sentence to the
end saying something like "you don't need to define this if there are
no index registers."

Thanks.

:REVIEWMAIL:

Ian