On targets without setup for cfa_base_rtx don't try to replace sp/hfp (PR target/45250)

Hi!

On PA fp was the same hard reg as hfp and argp, so trying to replace sp with
hfp if the hfp register was actually used for something else resulted in
ICEs.  While PA has been changed since then to have a new, artificial, hard
reg for argp, this patch should fix it for any other possible targets that
have similar setup.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2010-10-27  Jakub Jelinek  <jakub@redhat.com>

	PR target/45250
	* var-tracking.c (cfa_base_rtx): Move definition earlier in the file.
	(adjust_mems): If MAY_HAVE_DEBUG_STMTS, don't do any sp or hfp
	replacements if cfa_base_rtx is NULL.


	Jakub

On 10/27/2010 09:55 AM, Jakub Jelinek wrote:
> +	  && !frame_pointer_needed
> +	  && (!MAY_HAVE_DEBUG_STMTS || cfa_base_rtx))

It seems to me that one test of MAY_HAVE_DEBUG_STMTS is wrong.
It's either wrong here, in that cfa_base_reg should never be
null when compute_cfa_pointer is invoked, or it's wrong in
vt_init_cfa_base in that it's tested too late.

Also, it seems to me that compute_cfa_pointer is wrong in
that it should not effectively re-compute cfa_base_rtx, but
should use that existing decision.

I guess that's not a particularly directed review, except to
say that I think the logic here is distinctly unclear.


r~

On Wed, Oct 27, 2010 at 02:23:00PM -0400, Richard Henderson wrote:
> On 10/27/2010 09:55 AM, Jakub Jelinek wrote:
> > +	  && !frame_pointer_needed
> > +	  && (!MAY_HAVE_DEBUG_STMTS || cfa_base_rtx))
> 
> It seems to me that one test of MAY_HAVE_DEBUG_STMTS is wrong.

I think we can drop !MAY_HAVE_DEBUG_STMTS || check here, I believe we
can without it generate wrong debug info for -fno-var-tracking-assignments,
on the other side that's what the code was doing before and I didn't want to
affect those ports in that case too much.  But I'm fine with
just using && cfa_base_rtx, targets that will care about the debug
info quality will want a setup where cfa_base_rtx is non-NULL (i.e.
some virtual hard register that is always eliminated, like most of the
targets do already).

> It's either wrong here, in that cfa_base_reg should never be
> null when compute_cfa_pointer is invoked, or it's wrong in

cfa_base_rtx is NULL either if the target has weird argp/fp/hfp setup
and thus fp resp. argp can actually appear in the code, or
if frame_pointer_needed and we are still before initializing the
hfp register.

> Also, it seems to me that compute_cfa_pointer is wrong in
> that it should not effectively re-compute cfa_base_rtx, but
> should use that existing decision.

You mean using cfa_base_rtx instead of {frame,arg}_pointer_rtx
or also just remembering the adjustment bias in vta_init_cfa_base
in HWI static global and using it in compute_cfa_pointer?
Just the former would still mean we'd need to have #ifdef...

	Jakub

On 10/27/2010 02:37 PM, Jakub Jelinek wrote:
> I think we can drop !MAY_HAVE_DEBUG_STMTS || check here, I believe we
> can without it generate wrong debug info for -fno-var-tracking-assignments,
> on the other side that's what the code was doing before ...

Ok, let's do that then.

>> Also, it seems to me that compute_cfa_pointer is wrong in
>> that it should not effectively re-compute cfa_base_rtx, but
>> should use that existing decision.
> 
> You mean using cfa_base_rtx instead of {frame,arg}_pointer_rtx
> or also just remembering the adjustment bias in vta_init_cfa_base
> in HWI static global and using it in compute_cfa_pointer?
> Just the former would still mean we'd need to have #ifdef...

Then let's use the later so that we consolidate the ifdef
into a single place.


r~