[patch] Fix var-tracking with dynamic stack realignment on x86

[patch] Fix var-tracking with dynamic stack realignment on x86

[Eric Botcazou]

[-- Attachment #1: Type: text/plain, Size: 1924 bytes --]

var-tracking doesn't fully work with dynamic stack realignment on x86 when a 
DRAP register is used for a function: the value of the incoming parameters is 
very quickly wrong (as soon as the DRAP register is clobbered), in particular
in backtraces.  The reason is that var-tracking doesn't track parameters passed 
on the stack and, therefore, doesn't invalidate their location when the DRAP 
register is reused for something else.

Small Ada testcase attached, compile p1.adb with either -mforce-drap -O -g 
or -mforce-drap -O -g -fno-var-tracking-assignments, break on Diff and display 
a backtrace: the arguments for Func are wrong.

The fix is (1) to arrange for var-tracking to track parameters passed on the 
stack and (2) to invalidate their location where there is a DRAP register and 
it is clobbered.

Tested on x86_64-suse-linux and i586-suse-linux.  Is there an (unofficial) 
maintainter for the var-tracking code?  Jakub?


2011-05-22  Eric Botcazou  <ebotcazou@adacore.com>

	* var-tracking.c (var_regno_delete): Adjust prototype.
	(delete_mem_loc): New static function.
	(delete_mem_locs_on_entry): Likewise.
	(var_reg_delete_and_set): Add local variable REGNO.
	If the register is the DRAP register, invoke delete_mem_locs_on_entry.
	(var_reg_delete): Add local variable REGNO.
	If the register is the DRAP register and is also clobbered, invoke
	delete_mem_locs_on_entry.
	(var_regno_delete): Turn type of REGNO parameter to unsigned int.
	If the register is the DRAP register, invoke delete_mem_locs_on_entry.
	(drap_setter): New static function.
	(vt_init_drap_reg): Likewise.
	(vt_initialize): Set PROLOGUE_BB unconditionally.
	Use drap_setter to find out the SET that initializes the DRAP register
	in the prologue, if any, and call vt_init_drap_reg on it.
	Call vt_init_cfa_base only if FP_CFA_OFFSET isn't equal to -1.
	Call vt_add_function_parameters at the end of the prologue.


-- 
Eric Botcazou

[-- Attachment #2: q1.adb --]
[-- Type: text/x-adasrc, Size: 309 bytes --]

with R1; use R1;

package body Q1 is

  type Arr is array (1..4) of Long_Long_Integer;
  for Arr'alignment use 16;

  function Func (A : Integer; B : Integer) return Integer is
    C : Integer;
    My_Arr : Arr;
  begin
    My_Arr (1) := Dummy;
    C := Diff (A/2, B/2);
    return A + B + C;
  end;

end Q1;

[-- Attachment #3: q1.ads --]
[-- Type: text/x-adasrc, Size: 109 bytes --]

package Q1 is

  function Func (A: Integer; B : Integer) return Integer;
  pragma Export (C, Func);

end Q1;

[-- Attachment #4: r1.ads --]
[-- Type: text/x-adasrc, Size: 127 bytes --]

package R1 is

  function Diff (A : Integer; B : Integer) return Integer;

  function Dummy return Long_Long_Integer;

end R1;

[-- Attachment #5: p1.adb --]
[-- Type: text/x-adasrc, Size: 110 bytes --]

with Q1; use Q1;

procedure P1 is
begin
  if Func (10, 20) /= 25 then
    raise Program_Error;
  end if;
end;

[-- Attachment #6: r1.adb --]
[-- Type: text/x-adasrc, Size: 317 bytes --]

with System.Machine_Code; use System.Machine_Code;

package body R1 is

  function Diff (A : Integer; B : Integer) return Integer is
  begin
    return A - B;
  end;

  function Dummy return Long_Long_Integer is
  begin
    Asm ("mov %%esp, %%ecx", Clobber => "%ecx", Volatile => true);
    return 1;
  end;

end R1;

[-- Attachment #7: p.diff --]
[-- Type: text/x-diff, Size: 8167 bytes --]

Index: var-tracking.c
===================================================================
--- var-tracking.c	(revision 174027)
+++ var-tracking.c	(working copy)
@@ -429,7 +429,7 @@ static void var_reg_set (dataflow_set *,
 static void var_reg_delete_and_set (dataflow_set *, rtx, bool,
 				    enum var_init_status, rtx);
 static void var_reg_delete (dataflow_set *, rtx, bool);
-static void var_regno_delete (dataflow_set *, int);
+static void var_regno_delete (dataflow_set *, unsigned int);
 static void var_mem_set (dataflow_set *, rtx, enum var_init_status, rtx);
 static void var_mem_delete_and_set (dataflow_set *, rtx, bool,
 				    enum var_init_status, rtx);
@@ -1690,6 +1690,36 @@ get_init_value (dataflow_set *set, rtx l
   return ret_val;
 }
 
+/* Remove the one-part variable pointed to by SLOT from the set passed as
+   INFO if its location is a MEM.  */
+
+static int
+delete_mem_loc (void **slot, void *info)
+{
+  dataflow_set *set = (dataflow_set *) info;
+  variable var = (variable) *slot;
+
+  gcc_assert (var->n_var_parts == 1);
+
+  if (MEM_P (var->var_part[0].loc_chain->loc))
+    delete_variable_part (set, var->var_part[0].loc_chain->loc, var->dv,
+			  var->var_part[0].offset);
+
+  return 1;
+}
+
+/* Remove all one-part variables from the entry block still live in SET
+   and whose location is a MEM.  */
+
+static void
+delete_mem_locs_on_entry (dataflow_set *set)
+{
+  set->traversed_vars = VTI (ENTRY_BLOCK_PTR)->out.vars;
+  htab_traverse (shared_hash_htab (VTI (ENTRY_BLOCK_PTR)->out.vars),
+		 delete_mem_loc, set);
+  set->traversed_vars = NULL;
+}
+
 /* Delete current content of register LOC in dataflow set SET and set
    the register to contain REG_EXPR (LOC), REG_OFFSET (LOC).  If
    MODIFY is true, any other live copies of the same variable part are
@@ -1701,6 +1731,7 @@ static void
 var_reg_delete_and_set (dataflow_set *set, rtx loc, bool modify,
 			enum var_init_status initialized, rtx set_src)
 {
+  unsigned int regno = REGNO (loc);
   tree decl = REG_EXPR (loc);
   HOST_WIDE_INT offset = REG_OFFSET (loc);
   attrs node, next;
@@ -1711,7 +1742,7 @@ var_reg_delete_and_set (dataflow_set *se
   if (initialized == VAR_INIT_STATUS_UNKNOWN)
     initialized = get_init_value (set, loc, dv_from_decl (decl));
 
-  nextp = &set->regs[REGNO (loc)];
+  nextp = &set->regs[regno];
   for (node = *nextp; node; node = next)
     {
       next = node->next;
@@ -1729,6 +1760,12 @@ var_reg_delete_and_set (dataflow_set *se
     }
   if (modify)
     clobber_variable_part (set, loc, dv_from_decl (decl), offset, set_src);
+
+  /* If the DRAP register is set, then the incoming location of all parameters
+     not passed in registers is no longer valid.  */
+  if (stack_realign_drap && crtl->drap_reg && REGNO (crtl->drap_reg) == regno)
+    delete_mem_locs_on_entry (set);
+
   var_reg_set (set, loc, initialized, set_src);
 }
 
@@ -1740,7 +1777,8 @@ var_reg_delete_and_set (dataflow_set *se
 static void
 var_reg_delete (dataflow_set *set, rtx loc, bool clobber)
 {
-  attrs *nextp = &set->regs[REGNO (loc)];
+  unsigned int regno = REGNO (loc);
+  attrs *nextp = &set->regs[regno];
   attrs node, next;
 
   if (clobber)
@@ -1751,6 +1789,13 @@ var_reg_delete (dataflow_set *set, rtx l
       decl = var_debug_decl (decl);
 
       clobber_variable_part (set, NULL, dv_from_decl (decl), offset, NULL);
+
+      /* If the DRAP register is clobbered, then the incoming location of all
+	 parameters not passed in registers is no longer valid.  */
+      if (stack_realign_drap
+	  && crtl->drap_reg
+	  && REGNO (crtl->drap_reg) == regno)
+	delete_mem_locs_on_entry (set);
     }
 
   for (node = *nextp; node; node = next)
@@ -1770,7 +1815,7 @@ var_reg_delete (dataflow_set *set, rtx l
 /* Delete content of register with number REGNO in dataflow set SET.  */
 
 static void
-var_regno_delete (dataflow_set *set, int regno)
+var_regno_delete (dataflow_set *set, unsigned int regno)
 {
   attrs *reg = &set->regs[regno];
   attrs node, next;
@@ -1782,6 +1827,11 @@ var_regno_delete (dataflow_set *set, int
       pool_free (attrs_pool, node);
     }
   *reg = NULL;
+
+  /* If the contents of the DRAP register are changed, then the incoming
+     location of parameters not passed in registers is no longer valid.  */
+  if (stack_realign_drap && crtl->drap_reg && REGNO (crtl->drap_reg) == regno)
+    delete_mem_locs_on_entry (set);
 }
 
 /* Set the location of DV, OFFSET as the MEM LOC.  */
@@ -8658,13 +8708,61 @@ vt_init_cfa_base (void)
 		    0, NULL_RTX, INSERT);
 }
 
+/* Return the SET that initializes the DRAP register in INSN, if any.  */
+
+static rtx *
+drap_setter (rtx insn)
+{
+  const unsigned int drap_reg = REGNO (crtl->drap_reg);
+  rtx pat = PATTERN (insn);
+  if (GET_CODE (pat) == SET
+      && REG_P (SET_DEST (pat))
+      && REGNO (SET_DEST (pat)) == drap_reg)
+    return &PATTERN (insn);
+  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
+	    && REG_P (SET_DEST (XVECEXP (pat, 0, i)))
+	    && REGNO (SET_DEST (XVECEXP (pat, 0, i))) == drap_reg)
+	  return &XVECEXP (pat, 0, i);
+    }
+  return NULL;
+}
+
+/* Mask the SET that initializes the DRAP register and create a preserved VALUE
+   for it.  We need to do this because the incoming location of parameters not
+   passed in registers references the DRAP register _before_ the SET, since the
+   parameters are attached to the entry block.  */
+
+static void
+vt_init_drap_reg (rtx *set)
+{
+  cselib_val *val;
+
+  validate_change (NULL_RTX, set, gen_rtx_USE (VOIDmode, SET_SRC (*set)),
+		   true);
+
+  if (!MAY_HAVE_DEBUG_INSNS)
+    return;
+
+  val = cselib_lookup_from_insn (crtl->drap_reg, GET_MODE (crtl->drap_reg),
+				 1, VOIDmode, get_insns ());
+  preserve_value (val);
+  var_reg_decl_set (&VTI (ENTRY_BLOCK_PTR)->out, crtl->drap_reg,
+		    VAR_INIT_STATUS_INITIALIZED, dv_from_value (val->val_rtx),
+		    0, NULL_RTX, INSERT);
+}
+
 /* Allocate and initialize the data structures for variable tracking
    and parse the RTL to get the micro operations.  */
 
 static bool
 vt_initialize (void)
 {
-  basic_block bb, prologue_bb = NULL;
+  basic_block prologue_bb = single_succ (ENTRY_BLOCK_PTR), bb;
+  unsigned int drap_reg = INVALID_REGNUM;
   HOST_WIDE_INT fp_cfa_offset = -1;
 
   alloc_aux_for_blocks (sizeof (struct variable_tracking_info_def));
@@ -8764,8 +8862,6 @@ vt_initialize (void)
 	    }
 	  if (elim != hard_frame_pointer_rtx)
 	    fp_cfa_offset = -1;
-	  else
-	    prologue_bb = single_succ (ENTRY_BLOCK_PTR);
 	}
     }
   if (frame_pointer_needed)
@@ -8778,11 +8874,9 @@ vt_initialize (void)
 
   hard_frame_pointer_adjustment = -1;
 
-  vt_add_function_parameters ();
-
   FOR_EACH_BB (bb)
     {
-      rtx insn;
+      rtx insn, *set;
       HOST_WIDE_INT pre, post = 0;
       basic_block first_bb, last_bb;
 
@@ -8836,6 +8930,17 @@ vt_initialize (void)
 			}
 		    }
 
+		  if (bb == prologue_bb
+		      && stack_realign_drap
+		      && crtl->drap_reg
+		      && drap_reg == INVALID_REGNUM
+		      && RTX_FRAME_RELATED_P (insn)
+		      && ((set = drap_setter (insn)) != NULL))
+		    {
+		       vt_init_drap_reg (set);
+		       drap_reg = REGNO (crtl->drap_reg);
+		    }
+
 		  cselib_hook_called = false;
 		  adjust_insn (bb, insn);
 		  if (MAY_HAVE_DEBUG_INSNS)
@@ -8869,6 +8974,7 @@ vt_initialize (void)
 
 		  if (bb == prologue_bb
 		      && hard_frame_pointer_adjustment == -1
+		      && fp_cfa_offset != -1
 		      && RTX_FRAME_RELATED_P (insn)
 		      && fp_setter (insn))
 		    {
@@ -8876,6 +8982,14 @@ vt_initialize (void)
 		      hard_frame_pointer_adjustment = fp_cfa_offset;
 		    }
 		}
+
+	      /* Initialize the parameters right at the end of the prologue.
+		 We can do this neither earlier because the insn establishing
+		 the frame might invalidate all the previous MEMs, nor later
+		 because the DRAP register might be clobbered in between.  */
+	      else if (NOTE_P (insn)
+		       && NOTE_KIND (insn) == NOTE_INSN_PROLOGUE_END)
+		vt_add_function_parameters ();
 	    }
 	  gcc_assert (offset == VTI (bb)->out.stack_adjust);
 	}

Re: [patch] Fix var-tracking with dynamic stack realignment on x86

[Jakub Jelinek]

[-- Attachment #1: Type: text/plain, Size: 1986 bytes --]

On Sun, May 22, 2011 at 10:51:30PM +0200, Eric Botcazou wrote:
> var-tracking doesn't fully work with dynamic stack realignment on x86 when a 
> DRAP register is used for a function: the value of the incoming parameters is 
> very quickly wrong (as soon as the DRAP register is clobbered), in particular
> in backtraces.  The reason is that var-tracking doesn't track parameters passed 
> on the stack and, therefore, doesn't invalidate their location when the DRAP 
> register is reused for something else.
> 
> Small Ada testcase attached, compile p1.adb with either -mforce-drap -O -g 
> or -mforce-drap -O -g -fno-var-tracking-assignments, break on Diff and display 
> a backtrace: the arguments for Func are wrong.

Do you mean something like the attached one?  We don't have guality
testsuite for ada and I think ada testcases are quite unreadable for most
people.  Definitely to me.  Plus it is always better to have a single
file testcase over 5 files.

> The fix is (1) to arrange for var-tracking to track parameters passed on the 
> stack and (2) to invalidate their location where there is a DRAP register and 
> it is clobbered.

I guess this might be somewhat related to Alex'
http://gcc.gnu.org/ml/gcc-patches/2011-02/msg00981.html, but haven't tried
that patch.  But with tracking addresses as VALUEs var-tracking should
hopefully figure out that when %ecx as the DRAP register is saved into a
non-aliased stack location and then (maybe) clobbered during the call
that the addresses shouldn't use that hard reg afterwards, but need to
read it from the mem slot.  Anyway, IMHO the desired outcome is that the
parameters will have DW_OP_fbreg {0,4} as their location, so perhaps the
DRAP register needs to be remapped somehow during adjust_insn.

> Tested on x86_64-suse-linux and i586-suse-linux.  Is there an (unofficial) 
> maintainter for the var-tracking code?  Jakub?

I think Alex, who rewrote big chunks of var-tracking.c, is a better
unofficial reviewer.

	Jakub

[-- Attachment #2: draptest.c --]
[-- Type: text/plain, Size: 506 bytes --]

/* { dg-do run { target { i?86-*-* x86_64-*-* } } } */
/* { dg-options "-g -mforce-drap" } */

volatile int v;

__attribute__((noinline, noclone)) int
bar (int a, int b)
{
  int t = 0;
  asm volatile ("" : "+c" (t));
  return 0;
}

__attribute__((noinline, noclone)) int
foo (int a, int b)
{
  __attribute__((aligned (32))) int c = bar (a, b);
  v++;				/* { dg-final { gdb-test 18 "a" "5" } } */
  return a + b + c;		/* { dg-final { gdb-test 18 "b" "6" } } */
}

int
main ()
{
  foo (5, 6);
  return 0;
}

Re: [patch] Fix var-tracking with dynamic stack realignment on x86

[Eric Botcazou]

> Do you mean something like the attached one?  We don't have guality
> testsuite for ada and I think ada testcases are quite unreadable for most
> people.  Definitely to me.  Plus it is always better to have a single
> file testcase over 5 files.

Sure.  Last time I mentioned the guality testsuite, I was told that its results 
were random.  Thanks for the testcase in any case.

> I guess this might be somewhat related to Alex'
> http://gcc.gnu.org/ml/gcc-patches/2011-02/msg00981.html, but haven't tried
> that patch.  But with tracking addresses as VALUEs var-tracking should
> hopefully figure out that when %ecx as the DRAP register is saved into a
> non-aliased stack location and then (maybe) clobbered during the call
> that the addresses shouldn't use that hard reg afterwards, but need to
> read it from the mem slot.

Interesting, thanks for the pointer.

> Anyway, IMHO the desired outcome is that the 
> parameters will have DW_OP_fbreg {0,4} as their location, so perhaps the
> DRAP register needs to be remapped somehow during adjust_insn.

So you don't care about their location during the prologue?

-- 
Eric Botcazou

Re: [patch] Fix var-tracking with dynamic stack realignment on x86

[Jakub Jelinek]

On Mon, May 23, 2011 at 11:33:42AM +0200, Eric Botcazou wrote:
> > Do you mean something like the attached one?  We don't have guality
> > testsuite for ada and I think ada testcases are quite unreadable for most
> > people.  Definitely to me.  Plus it is always better to have a single
> > file testcase over 5 files.
> 
> Sure.  Last time I mentioned the guality testsuite, I was told that its results 
> were random.  Thanks for the testcase in any case.

They are far from random.  There is just an issue when using new debug info
features your debugger doesn't understand yet (if you are using recentish
gdb (like 7.2 or 7.3 that should be only a problem on the trunk), Jeff Law
posted a patch for it.  Other than that, the thing is just that not all
tests pass on all architectures or at all optimization levels, various
targets are very different and so while some var might be still available
in some place on one target somewhere, on another target there is no way
to compute its value without pessimizing generated code etc.  So, it is
always preferrable to compate test_summary output with earlier output from
the same architecture/gdb version.

> > Anyway, IMHO the desired outcome is that the 
> > parameters will have DW_OP_fbreg {0,4} as their location, so perhaps the
> > DRAP register needs to be remapped somehow during adjust_insn.
> 
> So you don't care about their location during the prologue?

I do, but DW_OP_fbreg should be precise all the time, on e.g. i386 it is
the address of the first parameter stack slot, that is a constant address
through the whole function.  And, .eh_frame is responsible for making sure
the CFA is correct at every instruction.

	Jakub

Re: [patch] Fix var-tracking with dynamic stack realignment on x86

[Jakub Jelinek]

On Mon, May 23, 2011 at 11:33:42AM +0200, Eric Botcazou wrote:
> > Anyway, IMHO the desired outcome is that the 
> > parameters will have DW_OP_fbreg {0,4} as their location, so perhaps the
> > DRAP register needs to be remapped somehow during adjust_insn.
> 
> So you don't care about their location during the prologue?

Here is an alternative, almost completely untested, patch, which
uses DW_OP_fbreg <N> for the arguments of dynamically realigned functions.
draptest.c now works...

2011-05-23  Jakub Jelinek  <jakub@redhat.com>

	* var-tracking.c (vt_add_function_parameter): Remap incoming
	MEMs with crtl->args.internal_arg_pointer based address
	if stack_realign_drap to arg_pointer_rtx.
	(vt_init_cfa_base): Add equate argument, don't equate cfa_base_rtx
	to hfp/sp if it is false.
	(vt_initialize): Adjust vt_init_cfa_base callers, don't call
	vt_init_cfa_base if frame_pointer_needed, but fp_cfa_offset is -1,
	set fp_cfa_offset to -1 if fp/argp hasn't been eliminated.

	* gcc.dg/guality/draptest.c: New test.

--- gcc/var-tracking.c.jj	2011-05-23 10:48:19.000000000 +0200
+++ gcc/var-tracking.c	2011-05-23 18:30:18.000000000 +0200
@@ -8449,6 +8449,8 @@ vt_get_decl_and_offset (rtx rtl, tree *d
   return false;
 }
 
+static void vt_init_cfa_base (bool);
+
 /* Insert function parameter PARM in IN and OUT sets of ENTRY_BLOCK.  */
 
 static void
@@ -8471,6 +8473,46 @@ vt_add_function_parameter (tree parm)
   if (GET_MODE (decl_rtl) == BLKmode || GET_MODE (incoming) == BLKmode)
     return;
 
+  if (MEM_P (incoming)
+      && stack_realign_drap
+      && frame_pointer_needed
+      && crtl->stack_realign_tried
+      && (XEXP (incoming, 0) == crtl->args.internal_arg_pointer
+	  || (GET_CODE (XEXP (incoming, 0)) == PLUS
+	      && XEXP (XEXP (incoming, 0), 0)
+		 == crtl->args.internal_arg_pointer
+	      && CONST_INT_P (XEXP (XEXP (incoming, 0), 1)))))
+    {
+      rtx new_addr;
+      if (cfa_base_rtx == NULL_RTX)
+	{
+	  rtx reg, elim;
+
+#ifdef FRAME_POINTER_CFA_OFFSET
+	  reg = frame_pointer_rtx;
+#else
+	  reg = arg_pointer_rtx;
+#endif
+	  elim = eliminate_regs (reg, VOIDmode, NULL_RTX);
+	  if (elim != reg)
+	    {
+	      if (GET_CODE (elim) == PLUS)
+		elim = XEXP (elim, 0);
+	      if (elim == hard_frame_pointer_rtx)
+		vt_init_cfa_base (false);
+	    }
+	}
+
+      if (cfa_base_rtx)
+	{
+	  HOST_WIDE_INT off = -FIRST_PARM_OFFSET (current_function_decl);
+	  new_addr = plus_constant (arg_pointer_rtx,
+				    (GET_CODE (XEXP (incoming, 0)) == PLUS
+				    ? INTVAL (XEXP (XEXP (incoming, 0), 1))
+				    : 0) + off);
+	  incoming = replace_equiv_address_nv (incoming, new_addr);
+	}
+    }
   if (!vt_get_decl_and_offset (incoming, &decl, &offset))
     {
       if (REG_P (incoming) || MEM_P (incoming))
@@ -8698,7 +8740,7 @@ note_register_arguments (rtx insn)
    has been eliminated.  */
 
 static void
-vt_init_cfa_base (void)
+vt_init_cfa_base (bool equate)
 {
   cselib_val *val;
 
@@ -8720,9 +8762,10 @@ vt_init_cfa_base (void)
 
   /* Tell alias analysis that cfa_base_rtx should share
      find_base_term value with stack pointer or hard frame pointer.  */
-  vt_equate_reg_base_value (cfa_base_rtx,
-			    frame_pointer_needed
-			    ? hard_frame_pointer_rtx : stack_pointer_rtx);
+  if (equate)
+    vt_equate_reg_base_value (cfa_base_rtx,
+			      frame_pointer_needed
+			      ? hard_frame_pointer_rtx : stack_pointer_rtx);
   val = cselib_lookup_from_insn (cfa_base_rtx, GET_MODE (cfa_base_rtx), 1,
 				 VOIDmode, get_insns ());
   preserve_value (val);
@@ -8814,7 +8857,7 @@ vt_initialize (void)
 	  if (GET_CODE (elim) == PLUS)
 	    elim = XEXP (elim, 0);
 	  if (elim == stack_pointer_rtx)
-	    vt_init_cfa_base ();
+	    vt_init_cfa_base (true);
 	}
     }
   else if (!crtl->stack_realign_tried)
@@ -8841,6 +8884,8 @@ vt_initialize (void)
 	  else
 	    prologue_bb = single_succ (ENTRY_BLOCK_PTR);
 	}
+      else
+	fp_cfa_offset = -1;
     }
   if (frame_pointer_needed)
     {
@@ -8944,9 +8989,10 @@ vt_initialize (void)
 		  if (bb == prologue_bb
 		      && hard_frame_pointer_adjustment == -1
 		      && RTX_FRAME_RELATED_P (insn)
-		      && fp_setter (insn))
+		      && fp_setter (insn)
+		      && fp_cfa_offset != -1)
 		    {
-		      vt_init_cfa_base ();
+		      vt_init_cfa_base (true);
 		      hard_frame_pointer_adjustment = fp_cfa_offset;
 		    }
 		}
--- gcc/testsuite/gcc.dg/guality/draptest.c.jj	2011-05-23 18:32:16.000000000 +0200
+++ gcc/testsuite/gcc.dg/guality/draptest.c	2011-05-23 14:55:25.000000000 +0200
@@ -0,0 +1,27 @@
+/* { dg-do run { target { i?86-*-* x86_64-*-* } } } */
+/* { dg-options "-g -mforce-drap" } */
+
+volatile int v;
+
+__attribute__((noinline, noclone)) int
+bar (int a, int b)
+{
+  int t = 0;
+  asm volatile ("" : "+c" (t));
+  return 0;
+}
+
+__attribute__((noinline, noclone)) int
+foo (int a, int b)
+{
+  __attribute__((aligned (32))) int c = bar (a, b);
+  v++;				/* { dg-final { gdb-test 18 "a" "5" } } */
+  return a + b + c;		/* { dg-final { gdb-test 18 "b" "6" } } */
+}
+
+int
+main ()
+{
+  foo (5, 6);
+  return 0;
+}


	Jakub

Re: [patch] Fix var-tracking with dynamic stack realignment on x86

[Eric Botcazou]

[-- Attachment #1: Type: text/plain, Size: 1733 bytes --]

> Here is an alternative, almost completely untested, patch, which
> uses DW_OP_fbreg <N> for the arguments of dynamically realigned functions.
> draptest.c now works...

Thanks for the tip.  I completely overlooked this CFA-based machinery and now 
agree that it's the most sensible approach.  For my defense, I would point out 
that it's quite lightly documented. :-)  I've attached a patch that adds a 
blurb about it (as well as contains the fp_cfa_offset adjustment we both have 
in our patches).  I'll install it as obvious if you don't have any objection.

Now on to a few questions:

> 2011-05-23  Jakub Jelinek  <jakub@redhat.com>
>
> 	* var-tracking.c (vt_add_function_parameter): Remap incoming
> 	MEMs with crtl->args.internal_arg_pointer based address
> 	if stack_realign_drap to arg_pointer_rtx.
> 	(vt_init_cfa_base): Add equate argument, don't equate cfa_base_rtx
> 	to hfp/sp if it is false.
> 	(vt_initialize): Adjust vt_init_cfa_base callers, don't call
> 	vt_init_cfa_base if frame_pointer_needed, but fp_cfa_offset is -1,
> 	set fp_cfa_offset to -1 if fp/argp hasn't been eliminated.

Why setting CFA_BASE_RTX here?  The remapping in vt_add_function_parameter 
doesn't seem to need it.  Is that necessary so that the processing of MEMs is 
short-circuited elsewhere?  If so, would this be correct in the whole function 
given that the MEMs aren't adjusted in this case (i.e. compute_cfa_pointer is 
never called)?


	* var-tracking.c (compute_cfa_pointer): Adjust head comment.
	(vt_initialize):  Set PROLOGUE_BB unconditionally.
	Add block comment about CFA_BASE_RTX machinery.
	Reset FP_CFA_OFFSET to -1 on all invalid paths.
	Call vt_init_cfa_base only if FP_CFA_OFFSET isn't equal to -1.


-- 
Eric Botcazou

[-- Attachment #2: p.diff --]
[-- Type: text/x-diff, Size: 2282 bytes --]

Index: var-tracking.c
===================================================================
--- var-tracking.c	(revision 174058)
+++ var-tracking.c	(working copy)
@@ -705,7 +705,8 @@ vt_stack_adjustments (void)
 static rtx cfa_base_rtx;
 static HOST_WIDE_INT cfa_base_offset;
 
-/* Compute a CFA-based value for the stack pointer.  */
+/* Compute a CFA-based value for an ADJUSTMENT made to stack_pointer_rtx
+   or hard_frame_pointer_rtx.  */
 
 static inline rtx
 compute_cfa_pointer (HOST_WIDE_INT adjustment)
@@ -8664,7 +8665,7 @@ vt_init_cfa_base (void)
 static bool
 vt_initialize (void)
 {
-  basic_block bb, prologue_bb = NULL;
+  basic_block bb, prologue_bb = single_succ (ENTRY_BLOCK_PTR);
   HOST_WIDE_INT fp_cfa_offset = -1;
 
   alloc_aux_for_blocks (sizeof (struct variable_tracking_info_def));
@@ -8722,6 +8723,15 @@ vt_initialize (void)
 
   CLEAR_HARD_REG_SET (argument_reg_set);
 
+  /* In order to shield ourselves from adjustments to the stack pointer or to
+     the hard frame pointer that aren't meant to change the base location of
+     variables, we're going to rewrite MEMs based on them into MEMs based on
+     the CFA by de-eliminating stack_pointer_rtx or hard_frame_pointer_rtx to
+     the virtual CFA pointer frame_pointer_rtx resp. arg_pointer_rtx.  We can
+     do this either when there is no frame pointer in the function and stack
+     adjustments are consistent for all basic blocks or when there is a frame
+     pointer and no stack realignment.  In any case, we first have to check
+     that frame_pointer_rtx resp. arg_pointer_rtx has been eliminated.  */
   if (!frame_pointer_needed)
     {
       rtx reg, elim;
@@ -8764,10 +8774,11 @@ vt_initialize (void)
 	    }
 	  if (elim != hard_frame_pointer_rtx)
 	    fp_cfa_offset = -1;
-	  else
-	    prologue_bb = single_succ (ENTRY_BLOCK_PTR);
 	}
+      else
+	fp_cfa_offset = -1;
     }
+
   if (frame_pointer_needed)
     {
       rtx insn;
@@ -8870,7 +8881,8 @@ vt_initialize (void)
 		  if (bb == prologue_bb
 		      && hard_frame_pointer_adjustment == -1
 		      && RTX_FRAME_RELATED_P (insn)
-		      && fp_setter (insn))
+		      && fp_setter (insn)
+		      && fp_cfa_offset != -1)
 		    {
 		      vt_init_cfa_base ();
 		      hard_frame_pointer_adjustment = fp_cfa_offset;

Re: [patch] Fix var-tracking with dynamic stack realignment on x86

[Jakub Jelinek]

On Tue, May 24, 2011 at 12:59:57PM +0200, Eric Botcazou wrote:
> Now on to a few questions:
> 
> > 2011-05-23  Jakub Jelinek  <jakub@redhat.com>
> >
> > 	* var-tracking.c (vt_add_function_parameter): Remap incoming
> > 	MEMs with crtl->args.internal_arg_pointer based address
> > 	if stack_realign_drap to arg_pointer_rtx.
> > 	(vt_init_cfa_base): Add equate argument, don't equate cfa_base_rtx
> > 	to hfp/sp if it is false.
> > 	(vt_initialize): Adjust vt_init_cfa_base callers, don't call
> > 	vt_init_cfa_base if frame_pointer_needed, but fp_cfa_offset is -1,
> > 	set fp_cfa_offset to -1 if fp/argp hasn't been eliminated.
> 
> Why setting CFA_BASE_RTX here?  The remapping in vt_add_function_parameter 
> doesn't seem to need it.  Is that necessary so that the processing of MEMs is 
> short-circuited elsewhere?  If so, would this be correct in the whole function 
> given that the MEMs aren't adjusted in this case (i.e. compute_cfa_pointer is 
> never called)?

That is so that the argp (or framep) will be properly handled during CSELIB,
as constant VALUE that is never changed through the function.  And
additionally to find out if we can do the replacement (we can't if
argp resp. framep isn't eliminated and could therefore occur in the real
code).

> -/* Compute a CFA-based value for the stack pointer.  */
> +/* Compute a CFA-based value for an ADJUSTMENT made to stack_pointer_rtx
> +   or hard_frame_pointer_rtx.  */

This is certainly fine.

>  static inline rtx
>  compute_cfa_pointer (HOST_WIDE_INT adjustment)
> @@ -8722,6 +8723,15 @@ vt_initialize (void)
>  
>    CLEAR_HARD_REG_SET (argument_reg_set);
>  
> +  /* In order to shield ourselves from adjustments to the stack pointer or to
> +     the hard frame pointer that aren't meant to change the base location of
> +     variables, we're going to rewrite MEMs based on them into MEMs based on
> +     the CFA by de-eliminating stack_pointer_rtx or hard_frame_pointer_rtx to
> +     the virtual CFA pointer frame_pointer_rtx resp. arg_pointer_rtx.  We can
> +     do this either when there is no frame pointer in the function and stack
> +     adjustments are consistent for all basic blocks or when there is a frame
> +     pointer and no stack realignment.  In any case, we first have to check
> +     that frame_pointer_rtx resp. arg_pointer_rtx has been eliminated.  */
>    if (!frame_pointer_needed)
>      {
>        rtx reg, elim;

This comment is not 100% accurate.  The reason we do that is to use the
much more compact DW_OP_fbreg* if possible compared to huge location lists,
especially for !frame_pointer_needed where the location list would need to
have another entry whenever sp changes.  dwarf2out.c rewrites argp/framep
if eliminated into DW_OP_fbreg (with some offset when needed).

> @@ -8664,7 +8665,7 @@ vt_init_cfa_base (void)
>  static bool
>  vt_initialize (void)
>  {
> -  basic_block bb, prologue_bb = NULL;
> +  basic_block bb, prologue_bb = single_succ (ENTRY_BLOCK_PTR);
>    HOST_WIDE_INT fp_cfa_offset = -1;
>  
>    alloc_aux_for_blocks (sizeof (struct variable_tracking_info_def));
> @@ -8764,10 +8774,11 @@ vt_initialize (void)
>  	    }
>  	  if (elim != hard_frame_pointer_rtx)
>  	    fp_cfa_offset = -1;
> -	  else
> -	    prologue_bb = single_succ (ENTRY_BLOCK_PTR);
>  	}
> +      else
> +	fp_cfa_offset = -1;
>      }
> +
>    if (frame_pointer_needed)
>      {
>        rtx insn;
> @@ -8870,7 +8881,8 @@ vt_initialize (void)
>  		  if (bb == prologue_bb
>  		      && hard_frame_pointer_adjustment == -1
>  		      && RTX_FRAME_RELATED_P (insn)
> -		      && fp_setter (insn))
> +		      && fp_setter (insn)
> +		      && fp_cfa_offset != -1)
>  		    {
>  		      vt_init_cfa_base ();
>  		      hard_frame_pointer_adjustment = fp_cfa_offset;

While I had part of this in my patch too, it is unnecessary, if prologue_bb
is set to non-NULL only when we want to use it, then we know fp_cfa_offset is not -1.
By computing prologue_bb always, it will call fp_setter unnecessarily
for every insn in the prologue bb for !frame_pointer_needed or for DRAP.
I guess adding a comment instead of this would be better.

	Jakub

Re: [patch] Fix var-tracking with dynamic stack realignment on x86

[Eric Botcazou]

[-- Attachment #1: Type: text/plain, Size: 1215 bytes --]

> That is so that the argp (or framep) will be properly handled during
> CSELIB, as constant VALUE that is never changed through the function.  And
> additionally to find out if we can do the replacement (we can't if argp
> resp. framep isn't eliminated and could therefore occur in the real code).

OK, thanks for the explanation.

> This comment is not 100% accurate.  The reason we do that is to use the
> much more compact DW_OP_fbreg* if possible compared to huge location lists,
> especially for !frame_pointer_needed where the location list would need to
> have another entry whenever sp changes.  dwarf2out.c rewrites argp/framep
> if eliminated into DW_OP_fbreg (with some offset when needed).

Adjusted.

> While I had part of this in my patch too, it is unnecessary, if prologue_bb
> is set to non-NULL only when we want to use it, then we know fp_cfa_offset
> is not -1. By computing prologue_bb always, it will call fp_setter
> unnecessarily for every insn in the prologue bb for !frame_pointer_needed
> or for DRAP. I guess adding a comment instead of this would be better.

The fp_cfa_offset != -1 was before the call to fp_setter in my first patch. :-)

Revised version attached.

-- 
Eric Botcazou

[-- Attachment #2: p.diff --]
[-- Type: text/x-diff, Size: 2259 bytes --]

Index: var-tracking.c
===================================================================
--- var-tracking.c	(revision 174058)
+++ var-tracking.c	(working copy)
@@ -705,7 +705,8 @@ vt_stack_adjustments (void)
 static rtx cfa_base_rtx;
 static HOST_WIDE_INT cfa_base_offset;
 
-/* Compute a CFA-based value for the stack pointer.  */
+/* Compute a CFA-based value for an ADJUSTMENT made to stack_pointer_rtx
+   or hard_frame_pointer_rtx.  */
 
 static inline rtx
 compute_cfa_pointer (HOST_WIDE_INT adjustment)
@@ -8664,7 +8665,7 @@ vt_init_cfa_base (void)
 static bool
 vt_initialize (void)
 {
-  basic_block bb, prologue_bb = NULL;
+  basic_block bb, prologue_bb = single_succ (ENTRY_BLOCK_PTR);
   HOST_WIDE_INT fp_cfa_offset = -1;
 
   alloc_aux_for_blocks (sizeof (struct variable_tracking_info_def));
@@ -8722,6 +8723,16 @@ vt_initialize (void)
 
   CLEAR_HARD_REG_SET (argument_reg_set);
 
+  /* In order to factor out the adjustments made to the stack pointer or to
+     the hard frame pointer and thus be able to use DW_OP_fbreg operations
+     instead of individual location lists, we're going to rewrite MEMs based
+     on them into MEMs based on the CFA by de-eliminating stack_pointer_rtx
+     or hard_frame_pointer_rtx to the virtual CFA pointer frame_pointer_rtx
+     resp. arg_pointer_rtx.  We can do this either when there is no frame
+     pointer in the function and stack adjustments are consistent for all
+     basic blocks or when there is a frame pointer and no stack realignment.
+     But we first have to check that frame_pointer_rtx resp. arg_pointer_rtx
+     has been eliminated.  */
   if (!frame_pointer_needed)
     {
       rtx reg, elim;
@@ -8764,10 +8775,11 @@ vt_initialize (void)
 	    }
 	  if (elim != hard_frame_pointer_rtx)
 	    fp_cfa_offset = -1;
-	  else
-	    prologue_bb = single_succ (ENTRY_BLOCK_PTR);
 	}
+      else
+	fp_cfa_offset = -1;
     }
+
   if (frame_pointer_needed)
     {
       rtx insn;
@@ -8867,7 +8879,8 @@ vt_initialize (void)
 		      VTI (bb)->out.stack_adjust += post;
 		    }
 
-		  if (bb == prologue_bb
+		  if (fp_cfa_offset != -1
+		      && bb == prologue_bb
 		      && hard_frame_pointer_adjustment == -1
 		      && RTX_FRAME_RELATED_P (insn)
 		      && fp_setter (insn))

Re: [patch] Fix var-tracking with dynamic stack realignment on x86

[Jakub Jelinek]

On Tue, May 24, 2011 at 01:42:11PM +0200, Eric Botcazou wrote:
> > That is so that the argp (or framep) will be properly handled during
> > CSELIB, as constant VALUE that is never changed through the function.  And
> > additionally to find out if we can do the replacement (we can't if argp
> > resp. framep isn't eliminated and could therefore occur in the real code).
> 
> OK, thanks for the explanation.

So, would you like me to redo my patch on top of your patch, test it and
submit?

> > This comment is not 100% accurate.  The reason we do that is to use the
> > much more compact DW_OP_fbreg* if possible compared to huge location lists,
> > especially for !frame_pointer_needed where the location list would need to
> > have another entry whenever sp changes.  dwarf2out.c rewrites argp/framep
> > if eliminated into DW_OP_fbreg (with some offset when needed).
> 
> Adjusted.

Ok.

> > While I had part of this in my patch too, it is unnecessary, if prologue_bb
> > is set to non-NULL only when we want to use it, then we know fp_cfa_offset
> > is not -1. By computing prologue_bb always, it will call fp_setter
> > unnecessarily for every insn in the prologue bb for !frame_pointer_needed
> > or for DRAP. I guess adding a comment instead of this would be better.
> 
> The fp_cfa_offset != -1 was before the call to fp_setter in my first patch. :-)

If you think it makes the code clearer, the extra comparison for at most
each stmt in the prologue_bb is probably noise compile time wise.
> 
> Revised version attached.

Fine with me, but I'm not a reviewer of this part of GCC...

	Jakub

Re: [patch] Fix var-tracking with dynamic stack realignment on x86

[Eric Botcazou]

> So, would you like me to redo my patch on top of your patch, test it and
> submit?

I'll take care of that.

> If you think it makes the code clearer, the extra comparison for at most
> each stmt in the prologue_bb is probably noise compile time wise.

No doubt about that.  Plus this may save the next hacker a dozen of minutes 
trying to understand why a seemingly innocuous code movement yields unexpected 
differences in the debug info...

> Fine with me, but I'm not a reviewer of this part of GCC...

Yes, but there is none officially, you wrote the code and the patch only tweaks 
comments and makes a minor code change.  Let's pretend that it's obvious.

Tested on i586-suse-linux, applied on the mainline.  Thanks again.

-- 
Eric Botcazou

Re: [patch] Fix var-tracking with dynamic stack realignment on x86

[Eric Botcazou]

[-- Attachment #1: Type: text/plain, Size: 777 bytes --]

> Here is an alternative, almost completely untested, patch, which
> uses DW_OP_fbreg <N> for the arguments of dynamically realigned functions.
> draptest.c now works...

I've attached a slightly revamped, IMO easier to understand version.  Stack 
realignment with DRAP is quite seldom on the mainline (because everything is 
ACCUMULATE_OUTGOING_ARGS now) so I don't think this will make any difference 
in practice.  Tested on i586 and x86-64.


	* var-tracking.c (vt_add_function_parameter): Remap incoming MEMs with
	crtl->args.internal_arg_pointer based address to arg_pointer_rtx if
	there is a DRAP register.
	(vt_init_cfa_base): Don't equate cfa_base_rtx if stack was realigned.
	(vt_initialize): Initialize cfa_base_rtx if there is a DRAP register.


-- 
Eric Botcazou

[-- Attachment #2: var-tracking.diff --]
[-- Type: text/x-diff, Size: 2938 bytes --]

Index: var-tracking.c
===================================================================
--- var-tracking.c	(revision 174377)
+++ var-tracking.c	(working copy)
@@ -8398,6 +8398,27 @@ vt_add_function_parameter (tree parm)
   if (GET_MODE (decl_rtl) == BLKmode || GET_MODE (incoming) == BLKmode)
     return;
 
+  /* If there is a DRAP register, rewrite the incoming location of parameters
+     passed on the stack into MEMs based on the argument pointer, as the DRAP
+     register can be reused for other purposes and we do not track locations
+     based on generic registers.  See also vt_initialize.  */
+  if (MEM_P (incoming)
+      && stack_realign_drap
+      && cfa_base_rtx
+      && (XEXP (incoming, 0) == crtl->args.internal_arg_pointer
+	  || (GET_CODE (XEXP (incoming, 0)) == PLUS
+	      && XEXP (XEXP (incoming, 0), 0)
+		 == crtl->args.internal_arg_pointer
+	      && CONST_INT_P (XEXP (XEXP (incoming, 0), 1)))))
+    {
+      HOST_WIDE_INT off = -FIRST_PARM_OFFSET (current_function_decl);
+      if (GET_CODE (XEXP (incoming, 0)) == PLUS)
+	off += INTVAL (XEXP (XEXP (incoming, 0), 1));
+      incoming
+	= replace_equiv_address_nv (incoming,
+				    plus_constant (arg_pointer_rtx, off));
+    }
+
   if (!vt_get_decl_and_offset (incoming, &decl, &offset))
     {
       if (REG_P (incoming) || MEM_P (incoming))
@@ -8647,9 +8668,11 @@ vt_init_cfa_base (void)
 
   /* Tell alias analysis that cfa_base_rtx should share
      find_base_term value with stack pointer or hard frame pointer.  */
-  vt_equate_reg_base_value (cfa_base_rtx,
-			    frame_pointer_needed
-			    ? hard_frame_pointer_rtx : stack_pointer_rtx);
+  if (!frame_pointer_needed)
+    vt_equate_reg_base_value (cfa_base_rtx, stack_pointer_rtx);
+  else if (!crtl->stack_realign_tried)
+    vt_equate_reg_base_value (cfa_base_rtx, hard_frame_pointer_rtx);
+
   val = cselib_lookup_from_insn (cfa_base_rtx, GET_MODE (cfa_base_rtx), 1,
 				 VOIDmode, get_insns ());
   preserve_value (val);
@@ -8780,6 +8803,31 @@ vt_initialize (void)
 	fp_cfa_offset = -1;
     }
 
+  /* If the stack is realigned and a DRAP register is used, we're going to
+     rewrite MEMs based on it representing incoming locations of parameters
+     passed on the stack into MEMs based on the argument pointer.  Although
+     we aren't going to rewrite other MEMs, we need to set up the virtual
+     CFA pointer in order to ensure that the argument pointer will be seen
+     as a constant throughout the function.  */
+  else if (stack_realign_drap)
+    {
+      rtx reg, elim;
+
+#ifdef FRAME_POINTER_CFA_OFFSET
+      reg = frame_pointer_rtx;
+#else
+      reg = arg_pointer_rtx;
+#endif
+      elim = eliminate_regs (reg, VOIDmode, NULL_RTX);
+      if (elim != reg)
+	{
+	  if (GET_CODE (elim) == PLUS)
+	    elim = XEXP (elim, 0);
+	  if (elim == hard_frame_pointer_rtx)
+	    vt_init_cfa_base ();
+	}
+    }
+
   if (frame_pointer_needed)
     {
       rtx insn;

Re: [patch] Fix var-tracking with dynamic stack realignment on x86

[Jakub Jelinek]

On Sun, May 29, 2011 at 10:57:34AM +0200, Eric Botcazou wrote:
> --- var-tracking.c	(revision 174377)
> +++ var-tracking.c	(working copy)
> @@ -8398,6 +8398,27 @@ vt_add_function_parameter (tree parm)
>    if (GET_MODE (decl_rtl) == BLKmode || GET_MODE (incoming) == BLKmode)
>      return;
>  
> +  /* If there is a DRAP register, rewrite the incoming location of parameters
> +     passed on the stack into MEMs based on the argument pointer, as the DRAP
> +     register can be reused for other purposes and we do not track locations
> +     based on generic registers.  See also vt_initialize.  */
> +  if (MEM_P (incoming)
> +      && stack_realign_drap
> +      && cfa_base_rtx
> +      && (XEXP (incoming, 0) == crtl->args.internal_arg_pointer
> +	  || (GET_CODE (XEXP (incoming, 0)) == PLUS
> +	      && XEXP (XEXP (incoming, 0), 0)
> +		 == crtl->args.internal_arg_pointer
> +	      && CONST_INT_P (XEXP (XEXP (incoming, 0), 1)))))
> +    {
> +      HOST_WIDE_INT off = -FIRST_PARM_OFFSET (current_function_decl);
> +      if (GET_CODE (XEXP (incoming, 0)) == PLUS)
> +	off += INTVAL (XEXP (XEXP (incoming, 0), 1));
> +      incoming
> +	= replace_equiv_address_nv (incoming,
> +				    plus_constant (arg_pointer_rtx, off));

This should be cfa_base_rtx instead of arg_pointer_rtx, or the condition
should test that cfa_base_rtx == arg_pointer_rtx.  We don't have a dynamic
stack realignment target which defines FRAME_POINTER_CFA_OFFSET right now,
but what if one day such target is added or just dynamic realignment
support is added.

And there should be a testcase added for it (e.g. the one I've posted in my
patch, I assume it works with your version of the patch too).

Otherwise it looks good to me, though I'm not a reviewer of this part of
GCC.

	Jakub

Re: [patch] Fix var-tracking with dynamic stack realignment on x86

[Alexandre Oliva]

On May 29, 2011, Eric Botcazou <ebotcazou@adacore.com> wrote:

> 	* var-tracking.c (vt_add_function_parameter): Remap incoming MEMs with
>       crtl-> args.internal_arg_pointer based address to arg_pointer_rtx if
> 	there is a DRAP register.
> 	(vt_init_cfa_base): Don't equate cfa_base_rtx if stack was realigned.
> 	(vt_initialize): Initialize cfa_base_rtx if there is a DRAP register.

This makes sense to me.  I don't think I can approve it, but if I could,
I would ;-)

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer

Re: [patch] Fix var-tracking with dynamic stack realignment on x86

[Bernd Schmidt]

On 05/30/2011 09:54 AM, Alexandre Oliva wrote:
> On May 29, 2011, Eric Botcazou <ebotcazou@adacore.com> wrote:
> 
>> 	* var-tracking.c (vt_add_function_parameter): Remap incoming MEMs with
>>       crtl-> args.internal_arg_pointer based address to arg_pointer_rtx if
>> 	there is a DRAP register.
>> 	(vt_init_cfa_base): Don't equate cfa_base_rtx if stack was realigned.
>> 	(vt_initialize): Initialize cfa_base_rtx if there is a DRAP register.
> 
> This makes sense to me.  I don't think I can approve it, but if I could,
> I would ;-)

Well as far as I'm concerned you and Jakub should be maintainers of this
stuff. Can you look at the changes Jakub suggested? If you agree with
them, I'll preapprove the changed patch.


Bernd

Re: [patch] Fix var-tracking with dynamic stack realignment on x86

[Eric Botcazou]

> This should be cfa_base_rtx instead of arg_pointer_rtx, or the condition
> should test that cfa_base_rtx == arg_pointer_rtx.  We don't have a dynamic
> stack realignment target which defines FRAME_POINTER_CFA_OFFSET right now,
> but what if one day such target is added or just dynamic realignment
> support is added.

OK, I wondered about that in the original patch.  Will adjust and re-test.

-- 
Eric Botcazou

Re: [patch] Fix var-tracking with dynamic stack realignment on x86

[Jakub Jelinek]

On Mon, May 30, 2011 at 01:16:13PM +0200, Eric Botcazou wrote:
> > This should be cfa_base_rtx instead of arg_pointer_rtx, or the condition
> > should test that cfa_base_rtx == arg_pointer_rtx.  We don't have a dynamic
> > stack realignment target which defines FRAME_POINTER_CFA_OFFSET right now,
> > but what if one day such target is added or just dynamic realignment
> > support is added.
> 
> OK, I wondered about that in the original patch.  Will adjust and re-test.

I think re-test is just waste of cycles, as you know cfa_base_rtx is
arg_pointer_rtx on both x86_64 and i686 (it is frame_pointer_rtx only on pa,
vax and rx).  IMHO just testing if var-tracking still compiles without
warnings ought to be enough.

	Jakub

Re: [patch] Fix var-tracking with dynamic stack realignment on x86

[Eric Botcazou]

[-- Attachment #1: Type: text/plain, Size: 1135 bytes --]

> I think re-test is just waste of cycles, as you know cfa_base_rtx is
> arg_pointer_rtx on both x86_64 and i686 (it is frame_pointer_rtx only on
> pa, vax and rx).  IMHO just testing if var-tracking still compiles without
> warnings ought to be enough.

Alright, quickstrapped and guality-regtested only.  Note that I tweaked the 
test as it didn't FAIL for me (it was UNSUPPORTED on i586 because of a "cannot 
access memory" error from within GDB and PASSed on x86-64).

Thanks for your help.


2011-05-30  Jakub Jelinek  <jakub@redhat.com>
            Eric Botcazou  <ebotcazou@adacore.com>

        * var-tracking.c (vt_add_function_parameter): Remap incoming MEMs with
        crtl->args.internal_arg_pointer based address to arg_pointer_rtx if
        there is a DRAP register and arg_pointer_rtx is the CFA pointer.
        (vt_init_cfa_base): Don't equate cfa_base_rtx if stack was realigned.
        (vt_initialize): Initialize cfa_base_rtx if there is a DRAP register.


2011-05-30  Jakub Jelinek  <jakub@redhat.com>
            Eric Botcazou  <ebotcazou@adacore.com>

	* gcc.dg/guality/drap.c: New test.


-- 
Eric Botcazou

[-- Attachment #2: var-tracking-2.diff --]
[-- Type: text/x-diff, Size: 3122 bytes --]

Index: var-tracking.c
===================================================================
--- var-tracking.c	(revision 174377)
+++ var-tracking.c	(working copy)
@@ -8398,6 +8398,28 @@ vt_add_function_parameter (tree parm)
   if (GET_MODE (decl_rtl) == BLKmode || GET_MODE (incoming) == BLKmode)
     return;
 
+  /* If there is a DRAP register, rewrite the incoming location of parameters
+     passed on the stack into MEMs based on the argument pointer, as the DRAP
+     register can be reused for other purposes and we do not track locations
+     based on generic registers.  But the prerequisite is that this argument
+     pointer be also the virtual CFA pointer, see vt_initialize.  */
+  if (MEM_P (incoming)
+      && stack_realign_drap
+      && arg_pointer_rtx == cfa_base_rtx
+      && (XEXP (incoming, 0) == crtl->args.internal_arg_pointer
+	  || (GET_CODE (XEXP (incoming, 0)) == PLUS
+	      && XEXP (XEXP (incoming, 0), 0)
+		 == crtl->args.internal_arg_pointer
+	      && CONST_INT_P (XEXP (XEXP (incoming, 0), 1)))))
+    {
+      HOST_WIDE_INT off = -FIRST_PARM_OFFSET (current_function_decl);
+      if (GET_CODE (XEXP (incoming, 0)) == PLUS)
+	off += INTVAL (XEXP (XEXP (incoming, 0), 1));
+      incoming
+	= replace_equiv_address_nv (incoming,
+				    plus_constant (arg_pointer_rtx, off));
+    }
+
   if (!vt_get_decl_and_offset (incoming, &decl, &offset))
     {
       if (REG_P (incoming) || MEM_P (incoming))
@@ -8647,9 +8669,11 @@ vt_init_cfa_base (void)
 
   /* Tell alias analysis that cfa_base_rtx should share
      find_base_term value with stack pointer or hard frame pointer.  */
-  vt_equate_reg_base_value (cfa_base_rtx,
-			    frame_pointer_needed
-			    ? hard_frame_pointer_rtx : stack_pointer_rtx);
+  if (!frame_pointer_needed)
+    vt_equate_reg_base_value (cfa_base_rtx, stack_pointer_rtx);
+  else if (!crtl->stack_realign_tried)
+    vt_equate_reg_base_value (cfa_base_rtx, hard_frame_pointer_rtx);
+
   val = cselib_lookup_from_insn (cfa_base_rtx, GET_MODE (cfa_base_rtx), 1,
 				 VOIDmode, get_insns ());
   preserve_value (val);
@@ -8780,6 +8804,33 @@ vt_initialize (void)
 	fp_cfa_offset = -1;
     }
 
+  /* If the stack is realigned and a DRAP register is used, we're going to
+     rewrite MEMs based on it representing incoming locations of parameters
+     passed on the stack into MEMs based on the argument pointer.  Although
+     we aren't going to rewrite other MEMs, we still need to initialize the
+     virtual CFA pointer in order to ensure that the argument pointer will
+     be seen as a constant throughout the function.
+
+     ??? This doesn't work if FRAME_POINTER_CFA_OFFSET is defined.  */
+  else if (stack_realign_drap)
+    {
+      rtx reg, elim;
+
+#ifdef FRAME_POINTER_CFA_OFFSET
+      reg = frame_pointer_rtx;
+#else
+      reg = arg_pointer_rtx;
+#endif
+      elim = eliminate_regs (reg, VOIDmode, NULL_RTX);
+      if (elim != reg)
+	{
+	  if (GET_CODE (elim) == PLUS)
+	    elim = XEXP (elim, 0);
+	  if (elim == hard_frame_pointer_rtx)
+	    vt_init_cfa_base ();
+	}
+    }
+
   if (frame_pointer_needed)
     {
       rtx insn;

[-- Attachment #3: drap.c --]
[-- Type: text/x-csrc, Size: 673 bytes --]

/* { dg-do run { target { i?86-*-* x86_64-*-* } } } */
/* { dg-options "-g -mforce-drap" } */

volatile int v;

__attribute__((noinline, noclone)) int
bar (int a, int b)
{
#ifdef __x86_64__
  asm volatile ("movq %%rsp, %%r10" : : : "r10");
#else
  asm volatile ("movl %%esp, %%ecx" : : : "ecx");
#endif
  return 0;
}

__attribute__((noinline, noclone)) int
foo (int v0, int v1, int v2, int v3, int v4, int v5, int a, int b)
{
  __attribute__((aligned (32))) int c = bar (a, b);
  v++;               /* { dg-final { gdb-test 21 "a" "5" } } */
  return a + b + c;  /* { dg-final { gdb-test 22 "b" "6" } } */
}

int
main (void)
{
  foo (0, 0, 0, 0, 0, 0, 5, 6);
  return 0;
}