From patchwork Thu Nov 24 13:57:38 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kyrill Tkachov X-Patchwork-Id: 83901 Delivered-To: patch@linaro.org Received: by 10.140.20.101 with SMTP id 92csp125946qgi; Thu, 24 Nov 2016 05:58:09 -0800 (PST) X-Received: by 10.98.83.193 with SMTP id h184mr2395162pfb.175.1479995889859; Thu, 24 Nov 2016 05:58:09 -0800 (PST) Return-Path: Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id 33si10995282plo.92.2016.11.24.05.58.09 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 24 Nov 2016 05:58:09 -0800 (PST) Received-SPF: pass (google.com: domain of gcc-patches-return-442555-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) client-ip=209.132.180.131; Authentication-Results: mx.google.com; dkim=pass header.i=@gcc.gnu.org; spf=pass (google.com: domain of gcc-patches-return-442555-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) smtp.mailfrom=gcc-patches-return-442555-patch=linaro.org@gcc.gnu.org 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=d/niozxm9w1BEBXm7xKZjWhvrxlYP7FgdtJIwcfvGaJO+c BPAfqnvs+YkCr85xNhyrQM/uE8i6mleKkIMhMxBJpjG8I+R0cvzqm0MDNEIfQJM/ GJuOXOeRuznp7dwDgmX6comJ2AUD1ZF/EYkiGWcqoVw1Wh5K0P0uRo4upecnE= 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=sS29BkVZ9qcxkjmmEicgqEmF5uU=; b=aWiEvzCG2+s3zsIZcOSo MwmSzEvlCMDt7L93xxwbx/SRhNy2OdazAm2GNjNlsZjxKnQHTpVQNZ38LKUrCnSl kf//5dmuUg8ueZh6X8MTE9s0SVkbv277vAQmN9G1rQApg4n/wqohR5rYsdgvOPlX HgA23t+msShqfkdl96DNx2Q= Received: (qmail 102448 invoked by alias); 24 Nov 2016 13:57:52 -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 102124 invoked by uid 89); 24 Nov 2016 13:57:52 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.9 required=5.0 tests=BAYES_00, KAM_LAZY_DOMAIN_SECURITY, RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=bsi, XCNEWVEC, xcnewvec, 2467 X-HELO: foss.arm.com Received: from foss.arm.com (HELO foss.arm.com) (217.140.101.70) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 24 Nov 2016 13:57:41 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id D57831515; Thu, 24 Nov 2016 05:57:39 -0800 (PST) Received: from [10.2.207.77] (e100706-lin.cambridge.arm.com [10.2.207.77]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 85D723F24D for ; Thu, 24 Nov 2016 05:57:39 -0800 (PST) Message-ID: <5836F1D2.8090407@foss.arm.com> Date: Thu, 24 Nov 2016 13:57:38 +0000 From: Kyrill Tkachov User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 MIME-Version: 1.0 To: GCC Patches Subject: [PATCH][TER] PR 48863: Don't replace expressions across local register variable definitions Hi all, In this bug we have TER during out-of-ssa misbehaving. A minimal example is: void foo(unsigned a, float b) { unsigned c = (unsigned) b; // 1 register unsigned r0 __asm__("r0") = a; // 2 register unsigned r1 __asm__("r1") = c; // 3 __asm__ volatile( "str %[r1], [%[r0]]\n" : : [r0] "r" (r0), [r1] "r" (r1)); } Statement 1 can produce a libcall to convert float b into an int and TER moves it into statement 3. But the libcall clobbers r0, which we want set to 'a' in statement 2. So r0 gets clobbered by the argument to the conversion libcall. TER already has code to avoid substituting across function calls and ideally we'd teach it to not substitute expressions that can perform a libcall across register variable definitions where the register can be clobbered in a libcall, but that information is not easy to get hold off in a general way at this level. So this patch disallows replacement across any local register variable definition. It does this by keeping track of local register definitions encountered in a similar way to which calls are counted for TER purposes. I hope this is not too big a hammer as local register variables are not very widely used and we only disable replacement across them and it allows us to fix the wrong-code bug on some inline-asm usage scenarios where gcc currently miscompiles code following its documented advice [1] Bootstrapped and tested on arm-none-linux-gnueabihf, aarch64-none-linux-gnu, x86_64. Is this approach acceptable? Thanks, Kyrill [1] https://gcc.gnu.org/onlinedocs/gcc/Local-Register-Variables.html#Local-Register-Variables 2016-11-24 Kyrylo Tkachov PR target/48863 PR inline-asm/70184 * tree-ssa-ter.c (temp_expr_table): Add reg_vars_cnt field. (new_temp_expr_table): Initialise reg_vars_cnt. (free_temp_expr_table): Release reg_vars_cnt. (process_replaceable): Add reg_vars_cnt argument, set reg_vars_cnt field of TAB. (find_replaceable_in_bb): Use the above to record register variable write occurrences and cancel replacement across them. 2016-11-24 Kyrylo Tkachov PR target/48863 PR inline-asm/70184 * gcc.target/arm/pr48863.c: New test. commit c53373216f7954e89893c1c7067e053fe6b819e8 Author: Kyrylo Tkachov Date: Wed Nov 23 16:44:47 2016 +0000 [TER] PR 48863: Don't replace expressions across local register variable definitions diff --git a/gcc/testsuite/gcc.target/arm/pr48863.c b/gcc/testsuite/gcc.target/arm/pr48863.c new file mode 100644 index 0000000..33bc7a4 --- /dev/null +++ b/gcc/testsuite/gcc.target/arm/pr48863.c @@ -0,0 +1,35 @@ +/* PR target/48863. */ +/* { dg-do run } */ +/* { dg-options "-O2" } */ + +/* Check that Temporary Expression Replacement does not move a + libcall-producing expression across a statement initialising a local + register variable. */ + +static inline int +dosvc (int fd, unsigned long high, unsigned low) +{ + register int r0 asm("r0") = fd; + register int r2 asm("r2") = high; + register int r3 asm("r3") = low; + + asm volatile("" : "=r"(r0) : "0"(r0), "r"(r2), "r"(r3)); + return r0; +} + +struct s +{ + int fd; + long long length; +} s = { 2, 0 }, *p = &s; + +int +main (void) +{ + unsigned low = p->length & 0xffffffff; + unsigned high = p->length / 23; + + if (dosvc (p->fd, high, low) != 2) + __builtin_abort (); + return 0; +} diff --git a/gcc/tree-ssa-ter.c b/gcc/tree-ssa-ter.c index c7d8b7e..af5d91c 100644 --- a/gcc/tree-ssa-ter.c +++ b/gcc/tree-ssa-ter.c @@ -169,6 +169,8 @@ struct temp_expr_table bitmap new_replaceable_dependencies; /* Holding place for pending dep's. */ int *num_in_part; /* # of ssa_names in a partition. */ int *call_cnt; /* Call count at definition. */ + int *reg_vars_cnt; /* Number of register variable + definitions encountered. */ }; /* Used to indicate a dependency on VDEFs. */ @@ -211,6 +213,7 @@ new_temp_expr_table (var_map map) t->num_in_part[p]++; } t->call_cnt = XCNEWVEC (int, num_ssa_names + 1); + t->reg_vars_cnt = XCNEWVEC (int, num_ssa_names + 1); return t; } @@ -243,6 +246,7 @@ free_temp_expr_table (temp_expr_table *t) free (t->partition_dependencies); free (t->num_in_part); free (t->call_cnt); + free (t->reg_vars_cnt); if (t->replaceable_expressions) ret = t->replaceable_expressions; @@ -435,7 +439,8 @@ ter_is_replaceable_p (gimple *stmt) /* Create an expression entry for a replaceable expression. */ static void -process_replaceable (temp_expr_table *tab, gimple *stmt, int call_cnt) +process_replaceable (temp_expr_table *tab, gimple *stmt, int call_cnt, + int reg_vars_cnt) { tree var, def, basevar; int version; @@ -477,6 +482,7 @@ process_replaceable (temp_expr_table *tab, gimple *stmt, int call_cnt) } tab->call_cnt[version] = call_cnt; + tab->reg_vars_cnt[version] = reg_vars_cnt; } @@ -573,6 +579,7 @@ find_replaceable_in_bb (temp_expr_table *tab, basic_block bb) ssa_op_iter iter; bool stmt_replaceable; int cur_call_cnt = 0; + int cur_reg_vars_cnt = 0; for (bsi = gsi_start_bb (bb); !gsi_end_p (bsi); gsi_next (&bsi)) { @@ -649,11 +656,14 @@ find_replaceable_in_bb (temp_expr_table *tab, basic_block bb) /* Mark expression as replaceable unless stmt is volatile, or the def variable has the same root variable as something in the substitution list, or the def and use span a call such that - we'll expand lifetimes across a call. */ + we'll expand lifetimes across a call. We also don't want to + replace across these expressions that may call libcalls that + clobber the register involved. See PR 70184. */ if (gimple_has_volatile_ops (stmt) || same_root_var || (tab->call_cnt[ver] != cur_call_cnt && SINGLE_SSA_USE_OPERAND (SSA_NAME_DEF_STMT (use), SSA_OP_USE) - == NULL_USE_OPERAND_P)) + == NULL_USE_OPERAND_P) + || tab->reg_vars_cnt[ver] != cur_reg_vars_cnt) finished_with_expr (tab, ver, true); else mark_replaceable (tab, use, stmt_replaceable); @@ -676,9 +686,16 @@ find_replaceable_in_bb (temp_expr_table *tab, basic_block bb) && DECL_BUILT_IN (fndecl))) cur_call_cnt++; + /* Increment counter if this statement sets a local + register variable. */ + if (gimple_assign_single_p (stmt) + && (TREE_CODE (gimple_assign_lhs (stmt)) == VAR_DECL + && DECL_HARD_REGISTER (gimple_assign_lhs (stmt)))) + cur_reg_vars_cnt++; + /* Now see if we are creating a new expression or not. */ if (stmt_replaceable) - process_replaceable (tab, stmt, cur_call_cnt); + process_replaceable (tab, stmt, cur_call_cnt, cur_reg_vars_cnt); /* Free any unused dependency lists. */ bitmap_clear (tab->new_replaceable_dependencies);