From patchwork Tue Nov 29 08:42:07 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jakub Jelinek X-Patchwork-Id: 128237 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) by ozlabs.org (Postfix) with SMTP id A0B2BB6F65 for ; Tue, 29 Nov 2011 19:42:58 +1100 (EST) Received: (qmail 5604 invoked by alias); 29 Nov 2011 08:42:55 -0000 Received: (qmail 5592 invoked by uid 22791); 29 Nov 2011 08:42:53 -0000 X-SWARE-Spam-Status: No, hits=-7.2 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, SPF_HELO_PASS X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 29 Nov 2011 08:42:25 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id pAT8g9Yo022164 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 29 Nov 2011 03:42:09 -0500 Received: from tyan-ft48-01.lab.bos.redhat.com (tyan-ft48-01.lab.bos.redhat.com [10.16.42.4]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id pAT8g8Wj022322 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 29 Nov 2011 03:42:08 -0500 Received: from tyan-ft48-01.lab.bos.redhat.com (tyan-ft48-01.lab.bos.redhat.com [127.0.0.1]) by tyan-ft48-01.lab.bos.redhat.com (8.14.4/8.14.4) with ESMTP id pAT8g7Lj008139; Tue, 29 Nov 2011 09:42:08 +0100 Received: (from jakub@localhost) by tyan-ft48-01.lab.bos.redhat.com (8.14.4/8.14.4/Submit) id pAT8g79R008137; Tue, 29 Nov 2011 09:42:07 +0100 Date: Tue, 29 Nov 2011 09:42:07 +0100 From: Jakub Jelinek To: Eric Botcazou Cc: gcc-patches@gcc.gnu.org Subject: [PATCH] Make sibcall argument overlap check less pessimistic (PR middle-end/50074, take 2) Message-ID: <20111129084207.GD27242@tyan-ft48-01.lab.bos.redhat.com> Reply-To: Jakub Jelinek References: <20111125184850.GQ27242@tyan-ft48-01.lab.bos.redhat.com> <201111282310.57159.ebotcazou@adacore.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <201111282310.57159.ebotcazou@adacore.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org 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. Jakub --- 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, internal_arg_pointer_exp_state.cache, + idx, val); + } + } + if (NEXT_INSN (insn) == NULL_RTX) + scan_start = insn; + insn = NEXT_INSN (insn); + } + + /* Reenable scanning. */ + internal_arg_pointer_exp_state.scan_start = scan_start; +} + +/* If EXP is based on crtl->args.internal_arg_pointer, return either + a CONST_INT offset from crtl->args.internal_arg_pointer if + offset from it is known constant, or PC if the offset is unknown. + Return NULL_RTX if EXP isn't based on crtl->args.internal_arg_pointer. */ + +static rtx +internal_arg_pointer_based_exp (rtx rtl) +{ + 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)); + if (val == NULL_RTX || val == pc_rtx) + return val; + return plus_constant (val, INTVAL (XEXP (rtl, 1))); + } + + /* When not called recursively, scan pseudo assignments in between the + last scanned instruction in the tail call sequence and the latest insn + in that sequence. */ + if (internal_arg_pointer_exp_state.scan_start != pc_rtx) + 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); + else + 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 +1800,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); + 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 +3418,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 { --- gcc/testsuite/gcc.c-torture/execute/pr51323.c.jj 2011-11-29 09:07:52.362695896 +0100 +++ gcc/testsuite/gcc.c-torture/execute/pr51323.c 2011-11-29 09:07:52.362695896 +0100 @@ -0,0 +1,35 @@ +/* PR middle-end/51323 */ + +extern void abort (void); +struct S { int a, b, c; }; +int v; + +__attribute__((noinline, noclone)) void +foo (int x, int y, int z) +{ + if (x != v || y != 0 || z != 9) + abort (); +} + +static inline int +baz (const struct S *p) +{ + return p->b; +} + +__attribute__((noinline, noclone)) void +bar (int x, struct S y) +{ + foo (baz (&y), 0, x); +} + +int +main () +{ + struct S s; + v = 3; s.a = v - 1; s.b = v; s.c = v + 1; + bar (9, s); + v = 17; s.a = v - 1; s.b = v; s.c = v + 1; + bar (9, s); + return 0; +}