From patchwork Wed Oct 16 17:53:59 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Bernd Schmidt X-Patchwork-Id: 284017 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]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 97B7E2C0084 for ; Thu, 17 Oct 2013 04:54:16 +1100 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:subject:content-type; q= dns; s=default; b=Ei62bzvga8PPnpVjcVhH8K6MJqk1AkpLGBiPjtsXH3GYSF 3TRyV8xjLpka6nXMgBZBorDJnZFOpWQplD6vhtbpwWVSZT/bGuPPnHmzqubTJwNw URutgpXmepQiNkVvJsRD1cIwhWS8Qd7BsSwp7v82cxkN6WHhZ+m+ocv/Vzj6U= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:subject:content-type; s= default; bh=oqTcKTnskspUL/0s1OmcwMv0/FA=; b=ZXsMEPrJuwmmW3EjdHyD c5IpK0rSk+EomtqTXr6cs4ym18baxHaAlsnxRkyu1wcWWKIhCVOW2n+0Myka5Ihd oIdeltpd+wrDGfzMAKugl1QI26HOZA75M4ggKCJnVOtcPxOzMhbBKmtG1tR6DZT9 y4z1u3q85ySQx5bpXl54bVw= Received: (qmail 8419 invoked by alias); 16 Oct 2013 17:54:10 -0000 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 Received: (qmail 8407 invoked by uid 89); 16 Oct 2013 17:54:10 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=AWL, BAYES_00 autolearn=ham version=3.3.2 X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 16 Oct 2013 17:54:07 +0000 Received: from svr-orw-exc-10.mgc.mentorg.com ([147.34.98.58]) by relay1.mentorg.com with esmtp id 1VWVIJ-0002rN-In from Bernd_Schmidt@mentor.com for gcc-patches@gcc.gnu.org; Wed, 16 Oct 2013 10:54:03 -0700 Received: from SVR-IES-FEM-01.mgc.mentorg.com ([137.202.0.104]) by SVR-ORW-EXC-10.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Wed, 16 Oct 2013 10:54:03 -0700 Received: from [127.0.0.1] (137.202.0.76) by SVR-IES-FEM-01.mgc.mentorg.com (137.202.0.104) with Microsoft SMTP Server id 14.2.247.3; Wed, 16 Oct 2013 18:54:01 +0100 Message-ID: <525ED2B7.3030801@codesourcery.com> Date: Wed, 16 Oct 2013 19:53:59 +0200 From: Bernd Schmidt User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130926 Thunderbird/17.0.9 MIME-Version: 1.0 To: GCC Patches Subject: alias fix for PR58685 The sequence of events here can be summarized as "shrink-wrapping causes the i386 backend to do something that confuses alias analysis". The miscompilation is that two instructions are swapped by the scheduler when they shouldn't be, due to an incorrect reg_base_value. The miscompiled function has the following parts: * a loop which uses %rdi as an incoming argument register * following that, a decrement of the stack pointer (shrink-wrapped to this point rather than the start of the function) * the rest of the function which has one set of %rdi to (symbol_ref LC0). The argument register %rdi is dead by the point where we decrement the stack pointer. The i386 backend splits the sub into a sequence of two insns, a clobber of %rdi and a push of it (which register is chosen appears to be somewhat random). When called from the scheduler, init_alias_analysis incorrectly deduces that %rdi has a base value of (symbol_ref LC0). Although it tries to track which registers are argument registers that may hold a pointer, this information is lost when we encounter the clobber. The main part of the following patch is to modify that piece of code to also set reg_seen for argument registers when encountering a clobber. There are other problems in this area, one of which showed up while testing an earlier patch. i386/pr57003.c demonstrates that return values of FUNCTION_ARG_REGNO are not constant across the whole compilation; they are affected by the ms_abi attribute. This necessitates changing from a static_reg_base_value array to a per-function one. Once that is done, it's better to look at DECL_ARGUMENTS in a similar way to what combine.c does to identify the arguments of the specific function we're compiling rather than using the less specific FUNCTION_ARG_REGNO. Lastly, I modified a test for Pmode to use the valid_pointer_mode hook which should be a little more correct. Bootstrapped and tested on x86_64-linux. Ok? Bernd PR rtl-optimization/58685 * alias.c (static_reg_base_value): Delete macro. (reg_base_value_initial): New macro to replace it. All uses changed. (record_set): For a clobber of an arg reg, set reg_seen. (setup_initial_base_values): Renamed from init_alias_target and made static. Check the DECL_INCOMING_RTL of the DECL_ARGUMENTS. Record that the array was set up for the current function. (init_alias_analysis): Call setup_initial_base_values if necessary. * function.h (struct emit_status): Add fields initial_reg_base_value and initial_reg_base_value_setup. * rtl.h (struct target_rtl): Remove x_static_reg_base_value. (init_alias_target): Don't declare. * toplev.c (backend_init_target): Don't call it. PR rtl-optimization/58685 * gcc.c-torture/execute/pr58685.c: New test. diff --git a/gcc/alias.c b/gcc/alias.c index a48bb51..b1189b7 100644 --- a/gcc/alias.c +++ b/gcc/alias.c @@ -229,8 +229,7 @@ static GTY((deletable)) vec *old_reg_base_value; #define UNIQUE_BASE_VALUE_FP -3 #define UNIQUE_BASE_VALUE_HFP -4 -#define static_reg_base_value \ - (this_target_rtl->x_static_reg_base_value) +#define reg_base_value_initial (crtl->emit.initial_reg_base_value) #define REG_BASE_VALUE(X) \ (REGNO (X) < vec_safe_length (reg_base_value) \ @@ -1306,6 +1305,20 @@ record_set (rtx dest, const_rtx set, void *data ATTRIBUTE_UNUSED) set). */ if (GET_CODE (set) == CLOBBER) { + if (new_reg_base_value[regno] != 0 + && regno != HARD_FRAME_POINTER_REGNUM + && regno != FRAME_POINTER_REGNUM + && regno != STACK_POINTER_REGNUM + && regno != ARG_POINTER_REGNUM) + { + if (!bitmap_bit_p (reg_seen, regno)) + { + /* We have to make an exception here for an argument + register, in case it is used before the clobber. */ + gcc_assert (new_reg_base_value[regno] == arg_base_value); + bitmap_set_bit (reg_seen, regno); + } + } new_reg_base_value[regno] = 0; return; } @@ -1693,7 +1706,7 @@ find_base_term (rtx x) return ret; if (cselib_sp_based_value_p (val)) - return static_reg_base_value[STACK_POINTER_REGNUM]; + return reg_base_value_initial[STACK_POINTER_REGNUM]; f = val->locs; /* Temporarily reset val->locs to avoid infinite recursion. */ @@ -1795,7 +1808,7 @@ bool may_be_sp_based_p (rtx x) { rtx base = find_base_term (x); - return !base || base == static_reg_base_value[STACK_POINTER_REGNUM]; + return !base || base == reg_base_value_initial[STACK_POINTER_REGNUM]; } /* Return 0 if the addresses X and Y are known to point to different @@ -2809,34 +2822,34 @@ may_alias_p (const_rtx mem, const_rtx x) return rtx_refs_may_alias_p (x, mem, false); } -void -init_alias_target (void) +static void +setup_initial_base_values (void) { - int i; - if (!arg_base_value) arg_base_value = gen_rtx_ADDRESS (VOIDmode, 0); - memset (static_reg_base_value, 0, sizeof static_reg_base_value); + memset (reg_base_value_initial, 0, sizeof reg_base_value_initial); - for (i = 0; i < FIRST_PSEUDO_REGISTER; i++) - /* Check whether this register can hold an incoming pointer - argument. FUNCTION_ARG_REGNO_P tests outgoing register - numbers, so translate if necessary due to register windows. */ - if (FUNCTION_ARG_REGNO_P (OUTGOING_REGNO (i)) - && HARD_REGNO_MODE_OK (i, Pmode)) - static_reg_base_value[i] = arg_base_value; + for (tree arg = DECL_ARGUMENTS (current_function_decl); arg; + arg = DECL_CHAIN (arg)) + { + rtx reg = DECL_INCOMING_RTL (arg); + if (REG_P (reg) && targetm.valid_pointer_mode (GET_MODE (reg))) + reg_base_value_initial[REGNO (reg)] = arg_base_value; + } - static_reg_base_value[STACK_POINTER_REGNUM] + reg_base_value_initial[STACK_POINTER_REGNUM] = unique_base_value (UNIQUE_BASE_VALUE_SP); - static_reg_base_value[ARG_POINTER_REGNUM] + reg_base_value_initial[ARG_POINTER_REGNUM] = unique_base_value (UNIQUE_BASE_VALUE_ARGP); - static_reg_base_value[FRAME_POINTER_REGNUM] + reg_base_value_initial[FRAME_POINTER_REGNUM] = unique_base_value (UNIQUE_BASE_VALUE_FP); #if !HARD_FRAME_POINTER_IS_FRAME_POINTER - static_reg_base_value[HARD_FRAME_POINTER_REGNUM] + reg_base_value_initial[HARD_FRAME_POINTER_REGNUM] = unique_base_value (UNIQUE_BASE_VALUE_HFP); #endif + + crtl->emit.initial_reg_base_value_setup = true; } /* Set MEMORY_MODIFIED when X modifies DATA (that is assumed @@ -2914,6 +2927,9 @@ init_alias_analysis (void) timevar_push (TV_ALIAS_ANALYSIS); + if (!crtl->emit.initial_reg_base_value_setup) + setup_initial_base_values (); + vec_safe_grow_cleared (reg_known_value, maxreg - FIRST_PSEUDO_REGISTER); reg_known_equiv_p = sbitmap_alloc (maxreg - FIRST_PSEUDO_REGISTER); bitmap_clear (reg_known_equiv_p); @@ -2983,7 +2999,7 @@ init_alias_analysis (void) The address expression is VOIDmode for an argument and Pmode for other registers. */ - memcpy (new_reg_base_value, static_reg_base_value, + memcpy (new_reg_base_value, reg_base_value_initial, FIRST_PSEUDO_REGISTER * sizeof (rtx)); /* Walk the insns adding values to the new_reg_base_value array. */ diff --git a/gcc/function.h b/gcc/function.h index d1f4ffc..5cd16a5 100644 --- a/gcc/function.h +++ b/gcc/function.h @@ -79,6 +79,13 @@ struct GTY(()) emit_status { for that pseudo (if REG_POINTER is set in x_regno_reg_rtx). Allocated in parallel with x_regno_reg_rtx. */ unsigned char * GTY((skip)) regno_pointer_align; + + /* Set up by init_alias_analysis and reused for successive invocations for + the same function. Contains information about whether a register can + be used to hold a pointer argument. */ + rtx initial_reg_base_value[FIRST_PSEUDO_REGISTER]; + /* True if the preceding array has been initialized for this function. */ + bool initial_reg_base_value_setup; }; diff --git a/gcc/rtl.h b/gcc/rtl.h index 247a0d0..7c92e9d 100644 --- a/gcc/rtl.h +++ b/gcc/rtl.h @@ -2296,10 +2296,6 @@ struct GTY(()) target_rtl { /* A sample (mem:M stack_pointer_rtx) rtx for each mode M. */ rtx x_top_of_stack[MAX_MACHINE_MODE]; - /* Static hunks of RTL used by the aliasing code; these are treated - as persistent to avoid unnecessary RTL allocations. */ - rtx x_static_reg_base_value[FIRST_PSEUDO_REGISTER]; - /* The default memory attributes for each mode. */ struct mem_attrs *x_mode_mem_attrs[(int) MAX_MACHINE_MODE]; }; @@ -2713,7 +2709,6 @@ extern int canon_anti_dependence (const_rtx, bool, const_rtx, enum machine_mode, rtx); extern int output_dependence (const_rtx, const_rtx); extern int may_alias_p (const_rtx, const_rtx); -extern void init_alias_target (void); extern void init_alias_analysis (void); extern void end_alias_analysis (void); extern void vt_equate_reg_base_value (const_rtx, const_rtx); diff --git a/gcc/testsuite/gcc.c-torture/execute/pr58685.c b/gcc/testsuite/gcc.c-torture/execute/pr58685.c new file mode 100644 index 0000000..36e770c --- /dev/null +++ b/gcc/testsuite/gcc.c-torture/execute/pr58685.c @@ -0,0 +1,144 @@ +/* This is a fragile testcase, requiring a random choice of push register + from the x86 backend to fail, hence the multiple copies of an identical + function. */ +extern void abort (); +extern void exit (int); +int a, b, c = 1, d, e; +int *f = &e, *g = &e, *i; + +char *p; +__attribute__((noinline,noclone)) void fail (char *f) +{ + abort (); +} + +void +fn1 () +{ + if (a) + i = 0; +} + + +int +fn3 (int *p1) +{ + for (b = 0; b >= 0; b--) + { + int *h = &e; + *h = 0; + if (*p1) + continue; + e = 1; + } + fn1 (); + c ? (void) 0 : fail (""); + return 0; +} + +int +fn3b (int *p1) +{ + for (b = 0; b >= 0; b--) + { + int *h = &e; + *h = 0; + if (*p1) + continue; + e = 1; + } + fn1 (); + c ? (void) 0 : fail (""); + return 0; +} +int +fn3c (int *p1) +{ + for (b = 0; b >= 0; b--) + { + int *h = &e; + *h = 0; + if (*p1) + continue; + e = 1; + } + fn1 (); + c ? (void) 0 : fail (""); + return 0; +} +int +fn3d (int *p1) +{ + for (b = 0; b >= 0; b--) + { + int *h = &e; + *h = 0; + if (*p1) + continue; + e = 1; + } + fn1 (); + c ? (void) 0 : fail (""); + return 0; +} +int +fn3e (int *p1) +{ + for (b = 0; b >= 0; b--) + { + int *h = &e; + *h = 0; + if (*p1) + continue; + e = 1; + } + fn1 (); + c ? (void) 0 : fail (""); + return 0; +} +int +fn3f (int *p1) +{ + for (b = 0; b >= 0; b--) + { + int *h = &e; + *h = 0; + if (*p1) + continue; + e = 1; + } + fn1 (); + c ? (void) 0 : fail (""); + return 0; +} + +int +main () +{ + e = 1; + fn3 (g); + if (!*f) + abort (); + e = 1; + fn3b (g); + if (!*f) + abort (); + e = 1; + fn3c (g); + if (!*f) + abort (); + e = 1; + fn3d (g); + if (!*f) + abort (); + e = 1; + fn3e (g); + if (!*f) + abort (); + e = 1; + fn3f (g); + if (!*f) + abort (); + exit (0); +} + diff --git a/gcc/toplev.c b/gcc/toplev.c index feba051..76ce292 100644 --- a/gcc/toplev.c +++ b/gcc/toplev.c @@ -1599,10 +1599,6 @@ backend_init_target (void) /* This depends on stack_pointer_rtx. */ init_fake_stack_mems (); - /* Sets static_base_value[HARD_FRAME_POINTER_REGNUM], which is - mode-dependent. */ - init_alias_target (); - /* Depends on HARD_FRAME_POINTER_REGNUM. */ init_reload ();