Patchwork Perform frame_pointer_needed hfp -> cfa replacements even if fp setter is not in the first bb (PR debug/54402)

login
register
mail settings
Submitter Jakub Jelinek
Date Oct. 29, 2012, 8:43 p.m.
Message ID <20121029204328.GM1752@tucnak.redhat.com>
Download mbox | patch
Permalink /patch/195137/
State New
Headers show

Comments

Jakub Jelinek - Oct. 29, 2012, 8:43 p.m.
Hi!

The following patch should fix the var-tracking slowness on the
reflect/check testcases where one of the test routines was compiling for
almost 50 minutes.  The problem was that there is a huge routine that is
shrink wrapped (or perhaps just the split stack prologue is multi-bb),
and I was trying to be too conservative when adding frame_pointer_needed
hard_frame_pointer_rtx VTA replacements.  Without those replacements we
weren't using DW_OP_fbreg for the hard frame pointer based mem accesses (so
unnecessarily large debug info), but what's worse is that on the hfp VALUE
we got almost 1200 reverse ops in the locs chain, so find_base_term as well
as get_addr has been quite costly.

Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
trunk?

2012-10-29  Jakub Jelinek  <jakub@redhat.com>

	PR debug/54402
	* var-tracking.c (fp_setter): Return false if there is REG_CFA_RESTORE
	hfp note.
	(vt_initialize): Look for fp_setter in any bb, not just successor of
	entry bb.


	Jakub
Steven Bosscher - Oct. 29, 2012, 8:46 p.m.
On Mon, Oct 29, 2012 at 9:43 PM, Jakub Jelinek wrote:
> The following patch should fix the var-tracking slowness on the
> reflect/check testcases where one of the test routines was compiling for
> almost 50 minutes.

Thanks for working on this.
Will you also add the test case?

Ciao!
Steven
Jakub Jelinek - Oct. 29, 2012, 8:52 p.m.
On Mon, Oct 29, 2012 at 09:46:24PM +0100, Steven Bosscher wrote:
> Will you also add the test case?

There already is, libgo make reflect/check.  I don't see what other kind of
testcase should I provide for that, unless you mean scanning -dA assembly
for DW_OP_fbreg (which would be inherently target specific).

BTW, only the go testcase has been worked on, I haven't been able to
reproduce any significant slowness on the #c0 testcase (perhaps I've been
trying wrong target or options), and the clobber_overlapping_mems
routine, while certainly desirable for correct debug info (without it
debug info might say something is available in a mem where it isn't
available), is still quite expensive and it would be nice to find some
ways to speed it up.

	Jakub
Ian Taylor - Oct. 29, 2012, 10:32 p.m.
On Mon, Oct 29, 2012 at 1:43 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>
> The following patch should fix the var-tracking slowness on the
> reflect/check testcases where one of the test routines was compiling for
> almost 50 minutes.  The problem was that there is a huge routine that is
> shrink wrapped (or perhaps just the split stack prologue is multi-bb),
> and I was trying to be too conservative when adding frame_pointer_needed
> hard_frame_pointer_rtx VTA replacements.  Without those replacements we
> weren't using DW_OP_fbreg for the hard frame pointer based mem accesses (so
> unnecessarily large debug info), but what's worse is that on the hfp VALUE
> we got almost 1200 reverse ops in the locs chain, so find_base_term as well
> as get_addr has been quite costly.

Thanks for working on this.

The patch looks fine to me but I don't know this code well.

Ian
Alexandre Oliva - Nov. 4, 2012, 11:10 p.m.
On Oct 29, 2012, Jakub Jelinek <jakub@redhat.com> wrote:

> 	PR debug/54402
> 	* var-tracking.c (fp_setter): Return false if there is REG_CFA_RESTORE
> 	hfp note.
> 	(vt_initialize): Look for fp_setter in any bb, not just successor of
> 	entry bb.

This looks reasonable, except that...

> --- gcc/var-tracking.c.jj	2012-10-29 12:16:23.000000000 +0100
> +++ gcc/var-tracking.c	2012-10-29 17:58:06.629025339 +0100
> @@ -9535,24 +9535,33 @@ vt_add_function_parameters (void)
>  static bool
>  fp_setter (rtx insn)
>  {
> -  rtx pat = PATTERN (insn);
> +  rtx pat = PATTERN (insn), expr;
>    if (RTX_FRAME_RELATED_P (insn))
>      {
> -      rtx expr = find_reg_note (insn, REG_FRAME_RELATED_EXPR, NULL_RTX);
> +      expr = find_reg_note (insn, REG_FRAME_RELATED_EXPR, NULL_RTX);
>        if (expr)
>  	pat = XEXP (expr, 0);

... I don't get why you moved expr out of the only block in which it's
used.  Ok with or without this change.  Thanks,

Patch

--- gcc/var-tracking.c.jj	2012-10-29 12:16:23.000000000 +0100
+++ gcc/var-tracking.c	2012-10-29 17:58:06.629025339 +0100
@@ -9535,24 +9535,33 @@  vt_add_function_parameters (void)
 static bool
 fp_setter (rtx insn)
 {
-  rtx pat = PATTERN (insn);
+  rtx pat = PATTERN (insn), expr;
   if (RTX_FRAME_RELATED_P (insn))
     {
-      rtx expr = find_reg_note (insn, REG_FRAME_RELATED_EXPR, NULL_RTX);
+      expr = find_reg_note (insn, REG_FRAME_RELATED_EXPR, NULL_RTX);
       if (expr)
 	pat = XEXP (expr, 0);
     }
   if (GET_CODE (pat) == SET)
-    return SET_DEST (pat) == hard_frame_pointer_rtx;
+    {
+      if (SET_DEST (pat) != hard_frame_pointer_rtx)
+	return false;
+    }
   else if (GET_CODE (pat) == PARALLEL)
     {
       int i;
       for (i = XVECLEN (pat, 0) - 1; i >= 0; i--)
 	if (GET_CODE (XVECEXP (pat, 0, i)) == SET
 	    && SET_DEST (XVECEXP (pat, 0, i)) == hard_frame_pointer_rtx)
-	  return true;
+	  break;
+      if (i < 0)
+	return false;
     }
-  return false;
+  else
+    return false;
+  if (find_reg_note (insn, REG_CFA_RESTORE, hard_frame_pointer_rtx))
+    return false;
+  return true;
 }
 
 /* Initialize cfa_base_rtx, create a preserved VALUE for it and
@@ -9600,7 +9609,7 @@  vt_init_cfa_base (void)
 static bool
 vt_initialize (void)
 {
-  basic_block bb, prologue_bb = single_succ (ENTRY_BLOCK_PTR);
+  basic_block bb;
   HOST_WIDE_INT fp_cfa_offset = -1;
 
   alloc_aux_for_blocks (sizeof (struct variable_tracking_info_def));
@@ -9858,8 +9867,7 @@  vt_initialize (void)
 		      VTI (bb)->out.stack_adjust += post;
 		    }
 
-		  if (bb == prologue_bb
-		      && fp_cfa_offset != -1
+		  if (fp_cfa_offset != -1
 		      && hard_frame_pointer_adjustment == -1
 		      && RTX_FRAME_RELATED_P (insn)
 		      && fp_setter (insn))