diff mbox

Make sibcall argument overlap check less pessimistic (PR middle-end/50074, take 2)

Message ID 201112042153.42684.ebotcazou@adacore.com
State New
Headers show

Commit Message

Eric Botcazou Dec. 4, 2011, 8:53 p.m. UTC
> What about this way?  I've groupped the two variables into a structure
> to make it clear it is internal internal_arg_pointer_based_exp* state,
> scanning is done in a separate function and the SCAN argument is gone,
> instead the internal_arg_pointer_based_exp_scan function disables scanning
> during recursion by tweaking the internal state.

Thanks.  I think this isn't exactly equivalent to the previous version though, 
as the recursive call made through internal_arg_pointer_based_exp_1 will now 
scan as well, won't it?  OK, my fault, the argument was probably better then.
But I think that we can keep the structure and the 2 functions for clarity.

Adjusted patch attached, same ChangeLog.  Please double-check, make the changes 
you deem necessary and install.

Comments

Jakub Jelinek Dec. 4, 2011, 9:13 p.m. UTC | #1
On Sun, Dec 04, 2011 at 09:53:42PM +0100, Eric Botcazou wrote:
> > What about this way?  I've groupped the two variables into a structure
> > to make it clear it is internal internal_arg_pointer_based_exp* state,
> > scanning is done in a separate function and the SCAN argument is gone,
> > instead the internal_arg_pointer_based_exp_scan function disables scanning
> > during recursion by tweaking the internal state.
> 
> Thanks.  I think this isn't exactly equivalent to the previous version though, 
> as the recursive call made through internal_arg_pointer_based_exp_1 will now 
> scan as well, won't it?  OK, my fault, the argument was probably better then.

I think it is.  Those called during internal_arg_pointer_based_exp_scan
will see scan_start equal to pc_rtx and won't scan, and for the calls after
it, while scan_start won't be pc_rtx, as it is after scan, it is either
NULL_RTX with no insns in the sequence, or some insn whose NEXT_INSN is
NULL, therefore it will attempt to scan, but won't scan a single insn.
But surely, if you prefer the explicit argument, I can test that version
too.

	Jakub
Eric Botcazou Dec. 4, 2011, 9:40 p.m. UTC | #2
> I think it is.  Those called during internal_arg_pointer_based_exp_scan
> will see scan_start equal to pc_rtx and won't scan, and for the calls after
> it, while scan_start won't be pc_rtx, as it is after scan, it is either
> NULL_RTX with no insns in the sequence, or some insn whose NEXT_INSN is
> NULL, therefore it will attempt to scan, but won't scan a single insn.

Right, internal_arg_pointer_based_exp_scan will be invoked for nothing.

> But surely, if you prefer the explicit argument, I can test that version
> too.

Yes, I think it is better in the end. :-)
diff mbox

Patch

Index: calls.c
===================================================================
--- calls.c	(revision 181902)
+++ calls.c	(working copy)
@@ -1658,6 +1658,129 @@  rtx_for_function_call (tree fndecl, tree
   return funexp;
 }
 
+/* Internal state for internal_arg_pointer_based_exp and its helpers.  */
+static struct
+{
+  /* Last insn that has been scanned by internal_arg_pointer_based_exp_scan,
+     or NULL_RTX if none has been scanned yet.  */
+  rtx scan_start;
+  /* Vector indexed by REGNO - FIRST_PSEUDO_REGISTER, recording if a pseudo is
+     based on crtl->args.internal_arg_pointer.  The element is NULL_RTX if the
+     pseudo isn't based on it, a CONST_INT offset if the pseudo is based on it
+     with fixed offset, or PC if this is with variable or unknown offset.  */
+  VEC(rtx, heap) *cache;
+} internal_arg_pointer_exp_state;
+
+static rtx internal_arg_pointer_based_exp (rtx, bool);
+
+/* Helper function for internal_arg_pointer_based_exp.  Scan insns in
+   the tail call sequence, starting with first insn that hasn't been
+   scanned yet, and note for each pseudo on the LHS whether it is based
+   on crtl->args.internal_arg_pointer or not, and what offset from that
+   that pointer it has.  */
+
+static void
+internal_arg_pointer_based_exp_scan (void)
+{
+  rtx insn, scan_start = internal_arg_pointer_exp_state.scan_start;
+
+  if (scan_start == NULL_RTX)
+    insn = get_insns ();
+  else
+    insn = NEXT_INSN (scan_start);
+
+  while (insn)
+    {
+      rtx set = single_set (insn);
+      if (set && REG_P (SET_DEST (set)) && !HARD_REGISTER_P (SET_DEST (set)))
+	{
+	  rtx val = NULL_RTX;
+	  unsigned int idx = REGNO (SET_DEST (set)) - FIRST_PSEUDO_REGISTER;
+	  /* Punt on pseudos set multiple times.  */
+	  if (idx < VEC_length (rtx, internal_arg_pointer_exp_state.cache)
+	      && (VEC_index (rtx, internal_arg_pointer_exp_state.cache, idx)
+		  != NULL_RTX))
+	    val = pc_rtx;
+	  else
+	    val = internal_arg_pointer_based_exp (SET_SRC (set), false);
+	  if (val != NULL_RTX)
+	    {
+	      VEC_safe_grow_cleared (rtx, heap,
+				     internal_arg_pointer_exp_state.cache,
+				     idx + 1);
+	      VEC_replace (rtx, internal_arg_pointer_exp_state.cache,
+			   idx, val);
+	    }
+	}
+      if (NEXT_INSN (insn) == NULL_RTX)
+	scan_start = insn;
+      insn = NEXT_INSN (insn);
+    }
+
+  internal_arg_pointer_exp_state.scan_start = scan_start;
+}
+
+/* Helper function for internal_arg_pointer_based_exp, called through
+   for_each_rtx.  Return 1 if *LOC is a register based on
+   crtl->args.internal_arg_pointer.  Return -1 if *LOC is not based on it
+   and the subexpressions need not be examined.  Otherwise return 0.  */
+
+static int
+internal_arg_pointer_based_exp_1 (rtx *loc, void *data ATTRIBUTE_UNUSED)
+{
+  if (REG_P (*loc) && internal_arg_pointer_based_exp (*loc, false) != NULL_RTX)
+    return 1;
+  if (MEM_P (*loc))
+    return -1;
+  return 0;
+}
+
+/* Compute whether RTL is based on crtl->args.internal_arg_pointer.  Return
+   NULL_RTX if RTL isn't based on it, a CONST_INT offset if RTL is based on
+   it with fixed offset, or PC if this is with variable or unknown offset.
+   TOPLEVEL is true if the function is invoked at the topmost level.  */
+
+static rtx
+internal_arg_pointer_based_exp (rtx rtl, bool toplevel)
+{
+  if (CONSTANT_P (rtl))
+    return NULL_RTX;
+
+  if (rtl == crtl->args.internal_arg_pointer)
+    return const0_rtx;
+
+  if (REG_P (rtl) && HARD_REGISTER_P (rtl))
+    return NULL_RTX;
+
+  if (GET_CODE (rtl) == PLUS && CONST_INT_P (XEXP (rtl, 1)))
+    {
+      rtx val = internal_arg_pointer_based_exp (XEXP (rtl, 0), toplevel);
+      if (val == NULL_RTX || val == pc_rtx)
+	return val;
+      return plus_constant (val, INTVAL (XEXP (rtl, 1)));
+    }
+
+  /* When called at the topmost level, scan pseudo assignments in between the
+     last scanned instruction in the tail call sequence and the latest insn
+     in that sequence.  */
+  if (toplevel)
+    internal_arg_pointer_based_exp_scan ();
+
+  if (REG_P (rtl))
+    {
+      unsigned int idx = REGNO (rtl) - FIRST_PSEUDO_REGISTER;
+      if (idx < VEC_length (rtx, internal_arg_pointer_exp_state.cache))
+	return VEC_index (rtx, internal_arg_pointer_exp_state.cache, idx);
+
+      return NULL_RTX;
+    }
+
+  if (for_each_rtx (&rtl, internal_arg_pointer_based_exp_1, NULL))
+    return pc_rtx;
+
+  return NULL_RTX;
+}
+
 /* Return true if and only if SIZE storage units (usually bytes)
    starting from address ADDR overlap with already clobbered argument
    area.  This function is used to determine if we should give up a
@@ -1667,26 +1790,17 @@  static bool
 mem_overlaps_already_clobbered_arg_p (rtx addr, unsigned HOST_WIDE_INT size)
 {
   HOST_WIDE_INT i;
+  rtx val;
 
   if (sbitmap_empty_p (stored_args_map))
     return false;
-  if (addr == crtl->args.internal_arg_pointer)
-    i = 0;
-  else if (GET_CODE (addr) == PLUS
-	   && XEXP (addr, 0) == crtl->args.internal_arg_pointer
-	   && CONST_INT_P (XEXP (addr, 1)))
-    i = INTVAL (XEXP (addr, 1));
-  /* Return true for arg pointer based indexed addressing.  */
-  else if (GET_CODE (addr) == PLUS
-	   && (XEXP (addr, 0) == crtl->args.internal_arg_pointer
-	       || XEXP (addr, 1) == crtl->args.internal_arg_pointer))
-    return true;
-  /* If the address comes in a register, we have no idea of its origin so
-     give up and conservatively return true.  */
-  else if (REG_P(addr))
+  val = internal_arg_pointer_based_exp (addr, true);
+  if (val == NULL_RTX)
+    return false;
+  else if (val == pc_rtx)
     return true;
   else
-    return false;
+    i = INTVAL (val);
 
 #ifdef ARGS_GROW_DOWNWARD
   i = -i - size;
@@ -3294,6 +3408,8 @@  expand_call (tree exp, rtx target, int i
 	    }
 
 	  sbitmap_free (stored_args_map);
+	  internal_arg_pointer_exp_state.scan_start = NULL_RTX;
+	  VEC_free (rtx, heap, internal_arg_pointer_exp_state.cache);
 	}
       else
 	{