From patchwork Fri May 13 15:24:03 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Martin Jambor X-Patchwork-Id: 622059 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.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3r5tv23SrZz9t77 for ; Sat, 14 May 2016 01:24:29 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=C1by/bA5; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; q=dns; s=default; b=MVNFqDtT8/NzDqT5H cFzgxtKp+xLSO85l9nPM2uJ8DFwwxk5YI5i0H2/P7krGispoymZLICuA82x7DxT6 Zm6TS8Sg+snLnTWQf//KuW318myjpxZnHzSjLrFHksf0sEx+q1zGL90shrEG5upF ewiYzj1aTbUI2UmNEBFVRvN1/w= 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:date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=default; bh=fMFqngv+fM7EgDkSF0SYCjF tZ08=; b=C1by/bA5TJoQsui8RW/eykyyWFrOCThJ8zVO0xSnhEz6V4VmvI9MNan 51f6qhh5V74I9Jwq0IX4OSf7AVpQubJCvkSfqLXvpTe+FPt7zqRE7bcZ3Zv2kOa1 L3xr6VTER/cuvKaoUZnjir9yaZr+pdK8qxbE6abbpn53X2Cv45ts= Received: (qmail 77873 invoked by alias); 13 May 2016 15:24:20 -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 77291 invoked by uid 89); 13 May 2016 15:24:18 -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=decides, moments, sk:candida 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 (CAMELLIA256-SHA encrypted) ESMTPS; Fri, 13 May 2016 15:24:07 +0000 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 035E8AABC; Fri, 13 May 2016 15:24:03 +0000 (UTC) Date: Fri, 13 May 2016 17:24:03 +0200 From: Martin Jambor To: Eric Botcazou Cc: Richard Biener , gcc-patches@gcc.gnu.org, alan.lawrence@arm.com Subject: Re: [patch] Fix PR tree-optimization/70884 Message-ID: <20160513152403.GL5580@virgil.suse.cz> Mail-Followup-To: Eric Botcazou , Richard Biener , gcc-patches@gcc.gnu.org, alan.lawrence@arm.com References: <6226269.FBBDIS7nhY@polaris> <20609673.FxQTIz62nb@polaris> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20609673.FxQTIz62nb@polaris> User-Agent: Mutt/1.6.0 (2016-04-01) X-IsSubscribed: yes Hi, On Fri, May 13, 2016 at 01:01:50PM +0200, Eric Botcazou wrote: > > Hmm, the patch looks obvious if it was the intent to allow constant > > pool replacements > > _not_ only when the whole constant pool entry may go away. But I > > think the intent was > > to not do this otherwise it will generate worse code by forcing all > > loads from the constant pool to appear at > > function start. > > Do you mean when the whole constant pool entry is scalarized as opposed to > partially scalarized? > > > So - the "real" issue may be a missing > > should_scalarize_away_bitmap/cannot_scalarize_away_bitmap > > check somewhere. > > This seems to work: > > Index: tree-sra.c > =================================================================== > --- tree-sra.c (revision 236195) > +++ tree-sra.c (working copy) > @@ -2680,6 +2680,10 @@ analyze_all_variable_accesses (void) > EXECUTE_IF_SET_IN_BITMAP (tmp, 0, i, bi) > { > tree var = candidate (i); > + if (constant_decl_p (var) > + && (!bitmap_bit_p (should_scalarize_away_bitmap, i) > + || bitmap_bit_p (cannot_scalarize_away_bitmap, i))) > + continue; > struct access *access; > > access = sort_and_splice_var_accesses (var); > > but I have no idea whether this is correct or not. This would skip creation of access trees for the variables without disqualifying them and while it may "work," it is certainly ugly, causing lookups to traverse uninitialized structures. > > Martin, are we sure to disable scalarization of constant_decl_p variables not > covered by initialize_constant_pool_replacements that way? Overall, I think that removal of bitmap tests in initialize_constant_pool_replacements is certainly correct, if SRA decides to create replacements for a constant pool decl, it has to initialize them. Whether we want to perform such scalarization is another matter. SRA creates replacement for all parts of an aggregate that is loaded as a scalar multiple times. If moving (multiple) scalar loads from the constant pool to the beginning of the function is a bad idea, then I would propose the fix below which disables that heuristics for them. Effectively, the patch prevents late-SRA from doing anything for both testcases (PR 70884 and PR 70919). I have started a bootstrap and testing on x86_64 and i686 only a few moments ago but it would be great if someone also tried on an architecture for which the constant-pool SRA enhancement was intended, just to be sure. Thanks, Martin 2016-05-13 Martin Jambor PR tree-optimization/70884 * tree-sra.c (initialize_constant_pool_replacements): Do not check should_scalarize_away_bitmap and cannot_scalarize_away_bitmap bits. (sort_and_splice_var_accesses): Do not consider multiple scalar reads of constant pool data as a reason for scalarization. testsuite/ * gcc.dg/tree-ssa/pr70919.c: New test. --- gcc/testsuite/gcc.dg/tree-ssa/pr70919.c | 46 ++++++++++++++++++++++++++++ gcc/tree-sra.c | 54 ++++++++++++++++----------------- 2 files changed, 73 insertions(+), 27 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr70919.c diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr70919.c b/gcc/testsuite/gcc.dg/tree-ssa/pr70919.c new file mode 100644 index 0000000..bed0ab3 --- /dev/null +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr70919.c @@ -0,0 +1,46 @@ +/* { dg-do run } */ +/* { dg-options "-O" } */ + +#pragma pack(1) +struct S0 +{ + int f0:24; +}; + +struct S1 +{ + int f1; +} a; + +int b, c; + +char +fn1 (struct S1 p1) +{ + return 0; +} + +int +main () +{ + c = fn1 (a); + if (b) + { + struct S0 f[3][9] = + { { { 0 }, { 0 }, { 1 }, { 1 }, { 0 }, { 0 }, { 0 }, { 1 }, { 1 } }, + { { 0 }, { 0 }, { 1 }, { 1 }, { 0 }, { 0 }, { 0 }, { 1 }, { 1 } }, + { { 0 }, { 0 }, { 1 }, { 1 }, { 0 }, { 0 }, { 0 }, { 1 }, { 1 } } + }; + b = f[1][8].f0; + } + struct S0 g[3][9] = + { { { 0 }, { 0 }, { 1 }, { 1 }, { 0 }, { 0 }, { 0 }, { 1 }, { 1 } }, + { { 0 }, { 0 }, { 1 }, { 1 }, { 0 }, { 0 }, { 0 }, { 1 }, { 1 } }, + { { 0 }, { 0 }, { 1 }, { 1 }, { 0 }, { 0 }, { 0 }, { 1 }, { 1 } } + }; + + if (g[1][8].f0 != 1) + __builtin_abort (); + + return 0; +} diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c index 936d3a6..7c0e90d 100644 --- a/gcc/tree-sra.c +++ b/gcc/tree-sra.c @@ -2074,7 +2074,8 @@ sort_and_splice_var_accesses (tree var) access->grp_scalar_write = grp_scalar_write; access->grp_assignment_read = grp_assignment_read; access->grp_assignment_write = grp_assignment_write; - access->grp_hint = multiple_scalar_reads || total_scalarization; + access->grp_hint = total_scalarization + || (multiple_scalar_reads && !constant_decl_p (var)); access->grp_total_scalarization = total_scalarization; access->grp_partial_lhs = grp_partial_lhs; access->grp_unscalarizable_region = unscalarizable_region; @@ -3559,32 +3560,31 @@ initialize_constant_pool_replacements (void) unsigned i; EXECUTE_IF_SET_IN_BITMAP (candidate_bitmap, 0, i, bi) - if (bitmap_bit_p (should_scalarize_away_bitmap, i) - && !bitmap_bit_p (cannot_scalarize_away_bitmap, i)) - { - tree var = candidate (i); - if (!constant_decl_p (var)) - continue; - vec *access_vec = get_base_access_vector (var); - if (!access_vec) - continue; - for (unsigned i = 0; i < access_vec->length (); i++) - { - struct access *access = (*access_vec)[i]; - if (!access->replacement_decl) - continue; - gassign *stmt = gimple_build_assign ( - get_access_replacement (access), unshare_expr (access->expr)); - if (dump_file && (dump_flags & TDF_DETAILS)) - { - fprintf (dump_file, "Generating constant initializer: "); - print_gimple_stmt (dump_file, stmt, 0, 1); - fprintf (dump_file, "\n"); - } - gsi_insert_after (&gsi, stmt, GSI_NEW_STMT); - update_stmt (stmt); - } - } + { + tree var = candidate (i); + if (!constant_decl_p (var)) + continue; + vec *access_vec = get_base_access_vector (var); + if (!access_vec) + continue; + for (unsigned i = 0; i < access_vec->length (); i++) + { + struct access *access = (*access_vec)[i]; + if (!access->replacement_decl) + continue; + gassign *stmt + = gimple_build_assign (get_access_replacement (access), + unshare_expr (access->expr)); + if (dump_file && (dump_flags & TDF_DETAILS)) + { + fprintf (dump_file, "Generating constant initializer: "); + print_gimple_stmt (dump_file, stmt, 0, 1); + fprintf (dump_file, "\n"); + } + gsi_insert_after (&gsi, stmt, GSI_NEW_STMT); + update_stmt (stmt); + } + } seq = gsi_seq (gsi); if (seq)