From patchwork Fri Nov 4 13:33:52 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Martin_Li=C5=A1ka?= X-Patchwork-Id: 80863 Delivered-To: patch@linaro.org Received: by 10.140.97.247 with SMTP id m110csp1155082qge; Fri, 4 Nov 2016 06:34:25 -0700 (PDT) X-Received: by 10.98.111.1 with SMTP id k1mr26759873pfc.164.1478266465196; Fri, 04 Nov 2016 06:34:25 -0700 (PDT) Return-Path: Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id u90si10346396pfk.38.2016.11.04.06.34.24 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 04 Nov 2016 06:34:25 -0700 (PDT) Received-SPF: pass (google.com: domain of gcc-patches-return-440443-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-440443-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) smtp.mailfrom=gcc-patches-return-440443-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 :subject:to:references:cc:from:message-id:date:mime-version :in-reply-to:content-type; q=dns; s=default; b=tw0EE1s/pUbOKI1BX V3YPmf8M+kfutNUT3y+P/IdM/3Sy+v32G4gfECkL1XhnfMVexqGxuI28TQi7ZmP7 FjRNJRKpLAAfl6qSRHsZgLYS0S+1+zy+Byly/LROdqiGZ8Z2K4y+ani6S1N63H4x 1tOeJmZmzFEQFtQIUXKlvkCtlE= 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 :subject:to:references:cc:from:message-id:date:mime-version :in-reply-to:content-type; s=default; bh=iD21fV6BRapT6X6XX4boFo4 +nkk=; b=gRF3m72is8WzG19dEzyJnaetbLEOt/8i+C7tcz6xayWzcutm8zKFCOI h8dvl1esVk16MuwLNJLKqLhA6R25t1eR3IjuA/3kruflvWlyI1/J6Vm/cqL17v99 qwzryFPTexLYgMx0fgCTb+5h3q/jHv1QrxpikhvVymb5E/8JRBa8= Received: (qmail 130128 invoked by alias); 4 Nov 2016 13:34:06 -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 130110 invoked by uid 89); 4 Nov 2016 13:34:05 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00, SPF_PASS autolearn=ham version=3.3.2 spammy=memchr, improved, informed, scans X-HELO: mx2.suse.de Received: from mx2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 04 Nov 2016 13:33:55 +0000 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id DBEA3ABF9; Fri, 4 Nov 2016 13:33:52 +0000 (UTC) Subject: Re: [PATCH] Convert character arrays to string csts To: Jan Hubicka , Bernd Schmidt References: <1a50afaa-6b8e-ba98-6cde-aae51c05a2c4@suse.cz> <1b1f844f-a20d-4702-27ea-59d718b8a99e@redhat.com> <20161103130057.GD52446@kam.mff.cuni.cz> Cc: GCC Patches From: =?UTF-8?Q?Martin_Li=c5=a1ka?= Message-ID: <749c0fce-2c35-7f02-e130-7f04a4260ebf@suse.cz> Date: Fri, 4 Nov 2016 14:33:52 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20161103130057.GD52446@kam.mff.cuni.cz> X-IsSubscribed: yes On 11/03/2016 02:00 PM, Jan Hubicka wrote: >> On 11/03/2016 01:12 PM, Martin Liška wrote: >>> + tree init = DECL_INITIAL (decl); >>> + if (init >>> + && TREE_READONLY (decl) >>> + && can_convert_ctor_to_string_cst (init)) >>> + DECL_INITIAL (decl) = build_string_cst_from_ctor (init); >> >> I'd merge these two new functions since they're only ever called >> together. We'd then have something like >> >> if (init && TREE_READONLY (decl)) >> init = convert_ctor_to_string_cst (init); >> if (init) >> DECL_INITIAL (decl) = init; Done. >> >> I'll defer to Jan on whether finalize_decl seems like a good place >> to do this. > > I think finalize_decl may be bit too early because frontends may expects the > ctors to be in a way they produced them. We only want to convert those arrays > seen by middle-end so I would move the logic to varpool_node::analyze > > Otherwise the patch seems fine to me. > > Honza >> >>> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple.c b/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple.c >>> index 283bd1c..b2d1fd5 100644 >>> --- a/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple.c >>> +++ b/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple.c >>> @@ -4,12 +4,15 @@ >>> char *buffer1; >>> char *buffer2; >>> >>> +const char global[] = {'a', 'b', 'c', 'd', '\0'}; >>> + >>> #define SIZE 1000 >>> >>> int >>> main (void) >>> { >>> const char* const foo1 = "hello world"; >>> + const char local[] = "abcd"; >>> >>> buffer1 = __builtin_malloc (SIZE); >>> __builtin_strcpy (buffer1, foo1); >>> @@ -45,6 +48,10 @@ main (void) >>> __builtin_abort (); >>> if (__builtin_memchr (foo1, null, 12) != foo1 + 11) >>> __builtin_abort (); >>> + if (__builtin_memchr (global, null, 5) == 0) >>> + __builtin_abort (); >>> + if (__builtin_memchr (local, null, 5) == 0) >>> + __builtin_abort (); >> >> How is that a meaningful test? This seems to work even with an >> unpatched gcc. I'd have expected something that shows a benefit for >> doing this conversion, and maybe also a test that shows it isn't >> done in cases where it's not allowed. It's meaningful as it scans that there's no __builtin_memchr in optimized dump. I'm adding new tests that does the opposite test. >> >>> tree >>> -build_string_literal (int len, const char *str) >>> +build_string_literal (int len, const char *str, bool build_addr_expr) >> >> New arguments should be documented in the function comment. Yep, improved. >> >>> +/* Return TRUE when CTOR can be converted to a string constant. */ >> >> "if", not "when". Done. >> >>> + unsigned HOST_WIDE_INT elements = CONSTRUCTOR_NELTS (ctor); >>> + FOR_EACH_CONSTRUCTOR_ELT (CONSTRUCTOR_ELTS (ctor), idx, key, value) >>> + { >>> + if (key == NULL_TREE >>> + || TREE_CODE (key) != INTEGER_CST >>> + || !tree_fits_uhwi_p (value) >>> + || !useless_type_conversion_p (TREE_TYPE (value), char_type_node)) >>> + return false; >> >> Shouldn't all elements have the same type, or do you really have to >> call useless_type_conversion in a loop? >> >>> + /* Allow zero character just at the end of a string. */ >>> + if (integer_zerop (value)) >>> + return idx == elements - 1; >> >> Don't you also have to explicitly check it's there? >> >> >> Bernd Patch can bootstrap on ppc64le-redhat-linux and survives regression tests. Martin >From 4a2bd43ad10cfb0467dde15ca0be0deba8194ea7 Mon Sep 17 00:00:00 2001 From: marxin Date: Mon, 17 Oct 2016 15:24:46 +0200 Subject: [PATCH] Convert character arrays to string csts gcc/testsuite/ChangeLog: 2016-11-04 Martin Liska * gcc.dg/tree-ssa/builtins-folding-gimple.c (main): Add new tests. * gcc.dg/tree-ssa/builtins-folding-gimple-ub.c (main): Likewise. gcc/ChangeLog: 2016-11-04 Martin Liska * gimplify.c (gimplify_decl_expr): Emit INIT_EXPR just if it cannot be converted to a string constant. (gimplify_init_constructor): Create string constant for local variables (if possible). * tree.c (convert_ctor_to_string_cst): New function. (build_string_literal): Add new argument. (can_convert_ctor_to_string_cst): New function. * tree.h: Declare new functions. * varpool.c (ctor_for_folding): Return ctor for local variables. (varpool_node::analyze): Convert character array ctors to a string constant (if possible). --- gcc/gimplify.c | 16 ++++- .../gcc.dg/tree-ssa/builtins-folding-gimple-ub.c | 20 +++++- .../gcc.dg/tree-ssa/builtins-folding-gimple.c | 7 ++ gcc/tree.c | 83 ++++++++++++++++++++-- gcc/tree.h | 5 +- gcc/varpool.c | 14 +++- 6 files changed, 134 insertions(+), 11 deletions(-) diff --git a/gcc/gimplify.c b/gcc/gimplify.c index 1531582..32f0909 100644 --- a/gcc/gimplify.c +++ b/gcc/gimplify.c @@ -1495,7 +1495,8 @@ gimplify_decl_expr (tree *stmt_p, gimple_seq *seq_p) { if (!TREE_STATIC (decl)) { - DECL_INITIAL (decl) = NULL_TREE; + if (!TREE_READONLY (decl) || TREE_CODE (init) != STRING_CST) + DECL_INITIAL (decl) = NULL_TREE; init = build2 (INIT_EXPR, void_type_node, decl, init); gimplify_and_add (init, seq_p); ggc_free (init); @@ -4438,6 +4439,19 @@ gimplify_init_constructor (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p, break; } + /* Replace a ctor with a string constant with possible. */ + if (TREE_READONLY (object) + && VAR_P (object)) + { + tree string_ctor = convert_ctor_to_string_cst (ctor); + if (string_ctor) + { + TREE_OPERAND (*expr_p, 1) = string_ctor; + DECL_INITIAL (object) = string_ctor; + break; + } + } + /* Fetch information about the constructor to direct later processing. We might want to make static versions of it in various cases, and can only do so if it known to be a valid constant initializer. */ diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple-ub.c b/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple-ub.c index e1658d1..ea73258 100644 --- a/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple-ub.c +++ b/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple-ub.c @@ -4,11 +4,18 @@ char *buffer1; char *buffer2; +const char global1[] = {'a', 'b', 'c', 'd'}; +const char global2[] = {'a', 'b', '\0', 'd', '\0'}; +const char global3[] = {'a', 'b', [3] = 'x', 'c', '\0'}; +const char global4[] = {'a', 'b', 'c', 'd', [5] = '\0'}; +char global5[] = {'a', 'b', 'c', 'd', '\0'}; + #define SIZE 1000 int main (void) { + char null = '\0'; const char* const foo1 = "hello world"; /* MEMCHR. */ @@ -17,6 +24,17 @@ main (void) if (__builtin_memchr (foo1, 'x', 1000)) /* Not folded away. */ __builtin_abort (); + if (__builtin_memchr (global1, null, 1) == foo1) /* Not folded away. */ + __builtin_abort (); + if (__builtin_memchr (global2, null, 1) == foo1) /* Not folded away. */ + __builtin_abort (); + if (__builtin_memchr (global3, null, 1) == foo1) /* Not folded away. */ + __builtin_abort (); + if (__builtin_memchr (global4, null, 1) == foo1) /* Not folded away. */ + __builtin_abort (); + if (__builtin_memchr (global5, null, 1) == foo1) /* Not folded away. */ + __builtin_abort (); + /* STRNCMP. */ if (strncmp ("a", "b", -1)) /* { dg-warning "implicit declaration of function" } */ __builtin_abort (); @@ -24,4 +42,4 @@ main (void) return 0; } -/* { dg-final { scan-tree-dump-times "__builtin_memchr" 2 "optimized" } } */ +/* { dg-final { scan-tree-dump-times "__builtin_memchr" 7 "optimized" } } */ diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple.c b/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple.c index 283bd1c..b2d1fd5 100644 --- a/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple.c +++ b/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple.c @@ -4,12 +4,15 @@ char *buffer1; char *buffer2; +const char global[] = {'a', 'b', 'c', 'd', '\0'}; + #define SIZE 1000 int main (void) { const char* const foo1 = "hello world"; + const char local[] = "abcd"; buffer1 = __builtin_malloc (SIZE); __builtin_strcpy (buffer1, foo1); @@ -45,6 +48,10 @@ main (void) __builtin_abort (); if (__builtin_memchr (foo1, null, 12) != foo1 + 11) __builtin_abort (); + if (__builtin_memchr (global, null, 5) == 0) + __builtin_abort (); + if (__builtin_memchr (local, null, 5) == 0) + __builtin_abort (); __builtin_memchr (foo1, x, 11); __builtin_memchr (buffer1, x, zero); diff --git a/gcc/tree.c b/gcc/tree.c index 434aff1..84e5774 100644 --- a/gcc/tree.c +++ b/gcc/tree.c @@ -1784,6 +1784,70 @@ build_vector_from_val (tree vectype, tree sc) } } +/* Return TRUE if CTOR can be converted to a string constant. */ + +static bool +can_convert_ctor_to_string_cst (tree ctor) +{ + unsigned HOST_WIDE_INT idx; + tree value, key; + + tree type = TREE_TYPE (ctor); + if (TREE_CODE (ctor) != CONSTRUCTOR + || TREE_CODE (type) != ARRAY_TYPE + || !tree_fits_uhwi_p (TYPE_SIZE_UNIT (type))) + return false; + + tree subtype = TREE_TYPE (type); + if (TYPE_MAIN_VARIANT (subtype) != char_type_node) + return false; + + unsigned HOST_WIDE_INT ctor_length = tree_to_uhwi (TYPE_SIZE_UNIT (type)); + + /* Skip constructors with a hole. */ + if (CONSTRUCTOR_NELTS (ctor) != ctor_length) + return false; + + FOR_EACH_CONSTRUCTOR_ELT (CONSTRUCTOR_ELTS (ctor), idx, key, value) + { + if (key == NULL_TREE + || !tree_fits_uhwi_p (key) + || !tree_fits_uhwi_p (value)) + return false; + + /* Allow zero character only at the end of a string. */ + if (integer_zerop (value)) + return idx == ctor_length - 1; + else if (!ISPRINT ((char)tree_to_uhwi (value))) + return false; + } + + return false; +} + +/* Build string constant from a constructor CTOR. */ + +tree +convert_ctor_to_string_cst (tree ctor) +{ + if (!can_convert_ctor_to_string_cst (ctor)) + return NULL_TREE; + + unsigned HOST_WIDE_INT idx; + tree value; + vec *elts = CONSTRUCTOR_ELTS (ctor); + char *str = XNEWVEC (char, elts->length ()); + + FOR_EACH_CONSTRUCTOR_VALUE (elts, idx, value) + str[idx] = (char)tree_to_uhwi (value); + + tree init = build_string_literal (elts->length (), + ggc_alloc_string (str, elts->length ()), + false); + free (str); + return init; +} + /* Something has messed with the elements of CONSTRUCTOR C after it was built; calculate TREE_CONSTANT and TREE_SIDE_EFFECTS. */ @@ -11359,9 +11423,12 @@ maybe_build_call_expr_loc (location_t loc, combined_fn fn, tree type, } /* Create a new constant string literal and return a char* pointer to it. - The STRING_CST value is the LEN characters at STR. */ + The STRING_CST value is the LEN characters at STR. If BUILD_ADDR_EXPR + is set to true, ADDR_EXPR of the newly created string constant is + returned. */ + tree -build_string_literal (int len, const char *str) +build_string_literal (int len, const char *str, bool build_addr_expr) { tree t, elem, index, type; @@ -11374,10 +11441,14 @@ build_string_literal (int len, const char *str) TREE_READONLY (t) = 1; TREE_STATIC (t) = 1; - type = build_pointer_type (elem); - t = build1 (ADDR_EXPR, type, - build4 (ARRAY_REF, elem, - t, integer_zero_node, NULL_TREE, NULL_TREE)); + if (build_addr_expr) + { + type = build_pointer_type (elem); + t = build1 (ADDR_EXPR, type, + build4 (ARRAY_REF, elem, + t, integer_zero_node, NULL_TREE, NULL_TREE)); + } + return t; } diff --git a/gcc/tree.h b/gcc/tree.h index 6a98b6e..10ee0d0 100644 --- a/gcc/tree.h +++ b/gcc/tree.h @@ -3975,6 +3975,8 @@ extern tree build_vector_stat (tree, tree * MEM_STAT_DECL); #define build_vector(t,v) build_vector_stat (t, v MEM_STAT_INFO) extern tree build_vector_from_ctor (tree, vec *); extern tree build_vector_from_val (tree, tree); +extern tree convert_ctor_to_string_cst (tree); +extern tree build_vector_from_val (tree, tree); extern void recompute_constructor_flags (tree); extern void verify_constructor_flags (tree); extern tree build_constructor (tree, vec *); @@ -4022,7 +4024,8 @@ extern tree build_call_expr_internal_loc_array (location_t, enum internal_fn, tree, int, const tree *); extern tree maybe_build_call_expr_loc (location_t, combined_fn, tree, int, ...); -extern tree build_string_literal (int, const char *); +extern tree build_string_literal (int len, const char *str, + bool build_addr_expr = true); /* Construct various nodes representing data types. */ diff --git a/gcc/varpool.c b/gcc/varpool.c index 78969d2..bb16c7b 100644 --- a/gcc/varpool.c +++ b/gcc/varpool.c @@ -412,11 +412,15 @@ ctor_for_folding (tree decl) return error_mark_node; /* Do not care about automatic variables. Those are never initialized - anyway, because gimplifier exapnds the code. */ + anyway, because gimplifier expands the code. */ if (!TREE_STATIC (decl) && !DECL_EXTERNAL (decl)) { gcc_assert (!TREE_PUBLIC (decl)); - return error_mark_node; + if (!TREE_READONLY (decl) || TREE_THIS_VOLATILE (decl)) + return error_mark_node; + + tree ctor = DECL_INITIAL (decl); + return ctor == NULL_TREE ? error_mark_node : ctor; } gcc_assert (VAR_P (decl)); @@ -525,6 +529,12 @@ varpool_node::analyze (void) /* Compute the alignment early so function body expanders are already informed about increased alignment. */ align_variable (decl, 0); + + tree init = DECL_INITIAL (decl); + if (init && TREE_READONLY (decl)) + init = convert_ctor_to_string_cst (init); + if (init) + DECL_INITIAL (decl) = init; } if (alias) resolve_alias (varpool_node::get (alias_target)); -- 2.10.1