Re: [PATCH] Make sibcall argument overlap check less pessimistic (PR middle-end/50074, take 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. :-) -- Eric Botcazou
Re: [PATCH] Make sibcall argument overlap check less pessimistic (PR middle-end/50074, take 2)
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
Re: [PATCH] Make sibcall argument overlap check less pessimistic (PR middle-end/50074, take 2)
> 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.
--
Eric Botcazou
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, inte
[PATCH] Make sibcall argument overlap check less pessimistic (PR middle-end/50074, take 2)
On Mon, Nov 28, 2011 at 11:10:56PM +0100, Eric Botcazou wrote:
> > Here is an attempt to make the check more complete (e.g.
> > the change wouldn't see overlap if addr was PLUS of two REGs,
> > where one of the REGs was based on internal_arg_pointer, etc.)
> > and less pessimistic. As tree-tailcall.c doesn't allow tail calls
> > from functions that have address of any of the caller's parameters
> > taken, IMHO it is enough to look for internal_arg_pointer based
> > pseudos initialized in the tail call sequence.
> > This patch scans the tail call sequence and notes which pseudos
> > are based on internal_arg_pointer (and what offset from
> > that pointer they have) and uses that in
> > mem_overlaps_already_clobbered_arg_p.
>
> This looks reasonable, but the logic is a bit hard to follow, especially the
> double usage of internal_arg_pointer_based_reg depending on SCAN's value.
> Would it be possible to split it into 2 functions that recursively call each
> other?
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.
2011-11-29 Jakub Jelinek
PR middle-end/51323
PR middle-end/50074
* calls.c (internal_arg_pointer_exp_state): New variable.
(internal_arg_pointer_based_exp_1,
internal_arg_pointer_exp_scan): New functions.
(internal_arg_pointer_based_exp): New function.
(mem_overlaps_already_clobbered_arg_p): Use it.
(expand_call): Free internal_arg_pointer_exp_state.cache vector
and clear internal_arg_pointer_exp_state.scan_start.
* gcc.c-torture/execute/pr51323.c: New test.
--- gcc/calls.c.jj 2011-11-29 08:58:50.164030662 +0100
+++ gcc/calls.c 2011-11-29 09:29:21.355613795 +0100
@@ -1658,6 +1658,139 @@ rtx_for_function_call (tree fndecl, tree
return funexp;
}
+/* Internal state for internal_arg_pointer_based_exp function and its
+ helpers. */
+static struct
+{
+ /* Last insn that has been already scanned by
+ internal_arg_pointer_based_exp_scan, or NULL_RTX if none has been
+ scanned yet and scan should start at get_insns (), or pc_rtx if
+ internal_arg_pointer_based_exp is called from within
+ internal_arg_pointer_based_exp_scan and scanning shouldn't
+ be performed. */
+ 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);
+
+/* Helper function for internal_arg_pointer_based_exp, called through
+ for_each_rtx. Return 1 if a crtl->args.internal_arg_pointer based
+ register is seen anywhere. Return -1 if it is not based on it and
+ subexpressions of *LOC should not be examined. */
+
+static int
+internal_arg_pointer_based_exp_1 (rtx *loc, void *data ATTRIBUTE_UNUSED)
+{
+ if (REG_P (*loc) && internal_arg_pointer_based_exp (*loc) != NULL_RTX)
+return 1;
+ if (MEM_P (*loc))
+return -1;
+ return 0;
+}
+
+/* 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 LHS pseudo whether it is based on
+ crtl->args.internal_arg_pointer or not and what offset from
+ 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);
+
+ /* Disable scanning in the recursive internal_arg_pointer_based_exp
+ calls. */
+ internal_arg_pointer_exp_state.scan_start = pc_rtx;
+
+ 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));
+ if (val != NULL_RTX)
+ {
+ VEC_safe_grow_cleared (rtx, heap,
+internal_arg_pointer_exp_state.cache,
+idx + 1);
+ VEC_replace (rtx,
