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

login
register
mail settings
Submitter Jakub Jelinek
Date Oct. 27, 2010, 1:55 p.m.
Message ID <20101027135511.GE29412@tyan-ft48-01.lab.bos.redhat.com>
Download mbox | patch
Permalink /patch/69358/
State New
Headers show

Comments

Jakub Jelinek - Oct. 27, 2010, 1:55 p.m.
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
Richard Henderson - Oct. 27, 2010, 6:23 p.m.
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~
Jakub Jelinek - Oct. 27, 2010, 6:37 p.m.
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
Richard Henderson - Oct. 27, 2010, 7:03 p.m.
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~

Patch

--- gcc/var-tracking.c.jj	2010-08-20 16:05:41.000000000 +0200
+++ gcc/var-tracking.c	2010-08-31 21:03:58.000000000 +0200
@@ -786,6 +786,10 @@  use_narrower_mode (rtx x, enum machine_m
     }
 }
 
+/* arg_pointer_rtx resp. frame_pointer_rtx if stack_pointer_rtx or
+   hard_frame_pointer_rtx is being mapped to it.  */
+static rtx cfa_base_rtx;
+
 /* Helper function for adjusting used MEMs.  */
 
 static rtx
@@ -803,11 +807,13 @@  adjust_mems (rtx loc, const_rtx old_rtx,
       if (amd->mem_mode == VOIDmode && amd->store)
 	return loc;
       if (loc == stack_pointer_rtx
-	  && !frame_pointer_needed)
+	  && !frame_pointer_needed
+	  && (!MAY_HAVE_DEBUG_STMTS || cfa_base_rtx))
 	return compute_cfa_pointer (amd->stack_adjust);
       else if (loc == hard_frame_pointer_rtx
 	       && frame_pointer_needed
-	       && hard_frame_pointer_adjustment != -1)
+	       && hard_frame_pointer_adjustment != -1
+	       && (!MAY_HAVE_DEBUG_STMTS || cfa_base_rtx))
 	return compute_cfa_pointer (hard_frame_pointer_adjustment);
       return loc;
     case MEM:
@@ -4784,10 +4790,6 @@  var_lowpart (enum machine_mode mode, rtx
   return gen_rtx_REG_offset (loc, mode, regno, offset);
 }
 
-/* arg_pointer_rtx resp. frame_pointer_rtx if stack_pointer_rtx or
-   hard_frame_pointer_rtx is being mapped to it.  */
-static rtx cfa_base_rtx;
-
 /* Carry information about uses and stores while walking rtx.  */
 
 struct count_use_info