From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7367 invoked by alias); 6 Jun 2019 16:00:59 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 7358 invoked by uid 89); 6 Jun 2019 16:00:59 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-17.0 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_PASS autolearn=ham version=3.3.1 spammy=si, Special, teaching, H*UA:https X-HELO: mx1.suse.de Received: from mx2.suse.de (HELO mx1.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 06 Jun 2019 16:00:56 +0000 Received: from relay2.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 2C80DAD2A; Thu, 6 Jun 2019 16:00:54 +0000 (UTC) From: Martin Jambor To: GCC Patches Cc: Jan Hubicka Subject: Re: Teach same_types_for_tbaa to structurally compare arrays, pointers and vectors In-Reply-To: References: <20190524111433.mv5z33ysiatlxmxz@kam.mff.cuni.cz> <20190524131856.zduvz27dbjfy6yqw@kam.mff.cuni.cz> <20190527082804.uxp3ugxulvdray5z@kam.mff.cuni.cz> <20190529132057.ivcrg3upxubuaazh@kam.mff.cuni.cz> <20190529140804.bfxc7c3t3xgd5avy@kam.mff.cuni.cz> User-Agent: Notmuch/0.28.4 (https://notmuchmail.org) Emacs/26.2 (x86_64-suse-linux-gnu) Date: Thu, 06 Jun 2019 16:00:00 -0000 Message-ID: MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2019-06/txt/msg00368.txt.bz2 Hi, (now even including gcc-patches mailing list which we managed to drop again and Honza whom I forgot to CC the last time) On Thu, Jun 06 2019, Richard Biener wrote: > yOn Tue, 4 Jun 2019, Martin Jambor wrote: >> >> @@ -1822,9 +1863,19 @@ build_ref_for_model (location_t loc, tree base, HOST_WIDE_INT offset, >> >> NULL_TREE); >> >> } >> >> else >> >> - return >> >> - build_ref_for_offset (loc, base, offset, model->reverse, model->type, >> >> - gsi, insert_after); >> >> + { >> >> + tree res; >> >> + if (model->grp_same_access_path >> >> + && offset <= model->offset >> >> + && get_object_alignment (base) >= TYPE_ALIGN (TREE_TYPE (base)) >> > >> > not sure what this tests - but I think it should be part of the >> > grp_same_access_path check? >> > >> >> It checks that base is not under-aligned, I was assuming that I can >> safely construct COMPONENT_REFs and ARRAY_REFs on a properly aligned >> base. I hope that is still safe even of reference copying with >> unsharing but of course there is more room for surprises. >> >> It cannot be part of grp_same_access_path check because BASE is now >> something quite different. For example when optimizing >> >> s = *p; >> v = s.i; >> >> build_ref_for_model can be called to construct reference to load p->i >> and BASE is *p, as opposed to grp_same_access_path where the path is >> based on s. > > Oh, I see. Still alignment is ultimatively on the base, so > there shouldn't be any constraints here. That is, if you > substitute a base with different alignment the accesses will > change accordingly and that's independend on whether you > use a simple MEM_REF or re-build the access path. > >> >> The patch passes bootstrap end testing on x86_64-linux, please let me >> know if there is anything else you'd like me to adjust. > > Looks good to me. As said, eventually the alignment check is > unnecessary. OK, thank you. I am going to commit it and then immediately follow up with a patch removing the test. The combination has just passed bootstrap and testing on an x86_64-linux. At least I hope the above is a permission to do so. Thanks, Martin Subject: [PATCH 2/2] Drop alignment check in build_reconstructed_reference 2019-06-06 Martin Jambor * tree-sra.c (build_reconstructed_reference): Drop the alignment check. --- gcc/tree-sra.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c index a246a93a48d..074d4964379 100644 --- a/gcc/tree-sra.c +++ b/gcc/tree-sra.c @@ -1817,9 +1817,6 @@ build_reconstructed_reference (location_t, tree base, struct access *model) expr = TREE_OPERAND (expr, 0); } - if (get_object_alignment (base) < get_object_alignment (expr)) - return NULL; - TREE_OPERAND (prev_expr, 0) = base; tree ref = unshare_expr (model->expr); TREE_OPERAND (prev_expr, 0) = expr; -- 2.21.0 For the reference of people on the mailing list, the first patch was: Subject: [PATCH 1/2] Make SRA re-construct orginal memory accesses when easy 2019-06-03 Martin Jambor * tree-sra.c (struct access): New field grp_same_access_path. (dump_access): Dump it. (build_reconstructed_reference): New function. (build_ref_for_model): Use it if possible. (path_comparable_for_same_access): New function. (same_access_path_p): Likewise. (sort_and_splice_var_accesses): Set the new flag. (analyze_access_subtree): Likewise. (propagate_subaccesses_across_link): Propagate zero value of the new flag down the access tree. testsuite/ * gcc.dg/tree-ssa/alias-access-path-1.c: Remove -fno-tree-sra option. * gcc.dg/tree-ssa/ssa-dse-26.c: Disable FRE. * testsuite/gnat.dg/opt39.adb: Adjust scan dump. Addressed Richi's comments --- .../gcc.dg/tree-ssa/alias-access-path-1.c | 2 +- gcc/testsuite/gcc.dg/tree-ssa/ssa-dse-26.c | 2 +- gcc/testsuite/gnat.dg/opt39.adb | 3 +- gcc/tree-sra.c | 135 ++++++++++++++++-- 4 files changed, 131 insertions(+), 11 deletions(-) diff --git a/gcc/testsuite/gcc.dg/tree-ssa/alias-access-path-1.c b/gcc/testsuite/gcc.dg/tree-ssa/alias-access-path-1.c index 264f72aff0a..ba90b56fe5c 100644 --- a/gcc/testsuite/gcc.dg/tree-ssa/alias-access-path-1.c +++ b/gcc/testsuite/gcc.dg/tree-ssa/alias-access-path-1.c @@ -1,5 +1,5 @@ /* { dg-do compile } */ -/* { dg-options "-O2 -fdump-tree-fre3 -fno-tree-sra" } */ +/* { dg-options "-O2 -fdump-tree-fre3" } */ struct foo { int val; diff --git a/gcc/testsuite/gcc.dg/tree-ssa/ssa-dse-26.c b/gcc/testsuite/gcc.dg/tree-ssa/ssa-dse-26.c index 32d63899b63..836a8092ab9 100644 --- a/gcc/testsuite/gcc.dg/tree-ssa/ssa-dse-26.c +++ b/gcc/testsuite/gcc.dg/tree-ssa/ssa-dse-26.c @@ -1,5 +1,5 @@ /* { dg-do compile } */ -/* { dg-options "-O2 -fdump-tree-dse1-details -fno-short-enums" } */ +/* { dg-options "-O2 -fdump-tree-dse1-details -fno-short-enums -fno-tree-fre" } */ /* { dg-skip-if "temporary variable for constraint_expr is never used" { msp430-*-* } } */ enum constraint_expr_type diff --git a/gcc/testsuite/gnat.dg/opt39.adb b/gcc/testsuite/gnat.dg/opt39.adb index 3b12cf201ec..0a5ef67a2ed 100644 --- a/gcc/testsuite/gnat.dg/opt39.adb +++ b/gcc/testsuite/gnat.dg/opt39.adb @@ -27,4 +27,5 @@ begin end if; end; --- { dg-final { scan-tree-dump-times "MEM" 1 "optimized" } } +-- { dg-final { scan-tree-dump-not "MEM" "optimized" } } +-- { dg-final { scan-tree-dump-not "tmp" "optimized" } } diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c index fd51a3d0323..a246a93a48d 100644 --- a/gcc/tree-sra.c +++ b/gcc/tree-sra.c @@ -106,6 +106,7 @@ along with GCC; see the file COPYING3. If not see #include "ipa-utils.h" #include "builtins.h" + /* Enumeration of all aggregate reductions we can do. */ enum sra_mode { SRA_MODE_EARLY_IPA, /* early call regularization */ SRA_MODE_EARLY_INTRA, /* early intraprocedural SRA */ @@ -242,6 +243,10 @@ struct access access tree. */ unsigned grp_unscalarized_data : 1; + /* Set if all accesses in the group consist of the same chain of + COMPONENT_REFs and ARRAY_REFs. */ + unsigned grp_same_access_path : 1; + /* Does this access and/or group contain a write access through a BIT_FIELD_REF? */ unsigned grp_partial_lhs : 1; @@ -443,16 +448,18 @@ dump_access (FILE *f, struct access *access, bool grp) "grp_scalar_write = %d, grp_total_scalarization = %d, " "grp_hint = %d, grp_covered = %d, " "grp_unscalarizable_region = %d, grp_unscalarized_data = %d, " - "grp_partial_lhs = %d, grp_to_be_replaced = %d, " - "grp_to_be_debug_replaced = %d, grp_maybe_modified = %d, " + "grp_same_access_path = %d, grp_partial_lhs = %d, " + "grp_to_be_replaced = %d, grp_to_be_debug_replaced = %d, " + "grp_maybe_modified = %d, " "grp_not_necessarilly_dereferenced = %d\n", access->grp_read, access->grp_write, access->grp_assignment_read, access->grp_assignment_write, access->grp_scalar_read, access->grp_scalar_write, access->grp_total_scalarization, access->grp_hint, access->grp_covered, access->grp_unscalarizable_region, access->grp_unscalarized_data, - access->grp_partial_lhs, access->grp_to_be_replaced, - access->grp_to_be_debug_replaced, access->grp_maybe_modified, + access->grp_same_access_path, access->grp_partial_lhs, + access->grp_to_be_replaced, access->grp_to_be_debug_replaced, + access->grp_maybe_modified, access->grp_not_necessarilly_dereferenced); else fprintf (f, ", write = %d, grp_total_scalarization = %d, " @@ -1795,6 +1802,30 @@ build_ref_for_offset (location_t loc, tree base, poly_int64 offset, return mem_ref; } +/* Construct and return a memory reference that is equal to a portion of + MODEL->expr but is based on BASE. If this cannot be done, return NULL. */ + +static tree +build_reconstructed_reference (location_t, tree base, struct access *model) +{ + tree expr = model->expr, prev_expr = NULL; + while (!types_compatible_p (TREE_TYPE (expr), TREE_TYPE (base))) + { + if (!handled_component_p (expr)) + return NULL; + prev_expr = expr; + expr = TREE_OPERAND (expr, 0); + } + + if (get_object_alignment (base) < get_object_alignment (expr)) + return NULL; + + TREE_OPERAND (prev_expr, 0) = base; + tree ref = unshare_expr (model->expr); + TREE_OPERAND (prev_expr, 0) = expr; + return ref; +} + /* Construct a memory reference to a part of an aggregate BASE at the given OFFSET and of the same type as MODEL. In case this is a reference to a bit-field, the function will replicate the last component_ref of model's @@ -1822,9 +1853,19 @@ build_ref_for_model (location_t loc, tree base, HOST_WIDE_INT offset, NULL_TREE); } else - return - build_ref_for_offset (loc, base, offset, model->reverse, model->type, - gsi, insert_after); + { + tree res; + if (model->grp_same_access_path + && !TREE_THIS_VOLATILE (base) + && offset <= model->offset + /* build_reconstructed_reference can still fail if we have already + massaged BASE because of another type incompatibility. */ + && (res = build_reconstructed_reference (loc, base, model))) + return res; + else + return build_ref_for_offset (loc, base, offset, model->reverse, + model->type, gsi, insert_after); + } } /* Attempt to build a memory reference that we could but into a gimple @@ -2076,6 +2117,69 @@ find_var_candidates (void) return ret; } +/* Return true if EXP is a reference chain of COMPONENT_REFs and AREAY_REFs + ending either with a DECL or a MEM_REF with zero offset. */ + +static bool +path_comparable_for_same_access (tree expr) +{ + while (handled_component_p (expr)) + { + if (TREE_CODE (expr) == ARRAY_REF) + { + /* SSA name indices can occur here too when the array is of sie one. + But we cannot just re-use array_refs with SSA names elsewhere in + the function, so disallow non-constant indices. TODO: Remove this + limitation after teaching build_reconstructed_reference to replace + the index with the index type lower bound. */ + if (TREE_CODE (TREE_OPERAND (expr, 1)) != INTEGER_CST) + return false; + } + expr = TREE_OPERAND (expr, 0); + } + + if (TREE_CODE (expr) == MEM_REF) + { + if (!zerop (TREE_OPERAND (expr, 1))) + return false; + } + else + gcc_assert (DECL_P (expr)); + + return true; +} + +/* Assuming that EXP1 consists of only COMPONENT_REFs and ARRAY_REFs, return + true if the chain of these handled components are exactly the same as EXP2 + and the expression under them is the same DECL or an equivalent MEM_REF. + The reference picked by compare_access_positions must go to EXP1. */ + +static bool +same_access_path_p (tree exp1, tree exp2) +{ + if (TREE_CODE (exp1) != TREE_CODE (exp2)) + { + /* Special case single-field structures loaded sometimes as the field + and sometimes as the structure. If the field is of a scalar type, + compare_access_positions will put it into exp1. + + TODO: The gimple register type condition can be removed if teach + compare_access_positions to put inner types first. */ + if (is_gimple_reg_type (TREE_TYPE (exp1)) + && TREE_CODE (exp1) == COMPONENT_REF + && (TYPE_MAIN_VARIANT (TREE_TYPE (TREE_OPERAND (exp1, 0))) + == TYPE_MAIN_VARIANT (TREE_TYPE (exp2)))) + exp1 = TREE_OPERAND (exp1, 0); + else + return false; + } + + if (!operand_equal_p (exp1, exp2, OEP_ADDRESS_OF)) + return false; + + return true; +} + /* Sort all accesses for the given variable, check for partial overlaps and return NULL if there are any. If there are none, pick a representative for each combination of offset and size and create a linked list out of them. @@ -2116,6 +2220,7 @@ sort_and_splice_var_accesses (tree var) bool grp_partial_lhs = access->grp_partial_lhs; bool first_scalar = is_gimple_reg_type (access->type); bool unscalarizable_region = access->grp_unscalarizable_region; + bool grp_same_access_path = true; bool bf_non_full_precision = (INTEGRAL_TYPE_P (access->type) && TYPE_PRECISION (access->type) != access->size @@ -2134,6 +2239,8 @@ sort_and_splice_var_accesses (tree var) gcc_assert (access->offset >= low && access->offset + access->size <= high); + grp_same_access_path = path_comparable_for_same_access (access->expr); + j = i + 1; while (j < access_count) { @@ -2184,6 +2291,11 @@ sort_and_splice_var_accesses (tree var) } unscalarizable_region = true; } + + if (grp_same_access_path + && !same_access_path_p (access->expr, ac2->expr)) + grp_same_access_path = false; + ac2->group_representative = access; j++; } @@ -2202,6 +2314,7 @@ sort_and_splice_var_accesses (tree var) access->grp_total_scalarization = total_scalarization; access->grp_partial_lhs = grp_partial_lhs; access->grp_unscalarizable_region = unscalarizable_region; + access->grp_same_access_path = grp_same_access_path; *prev_acc_ptr = access; prev_acc_ptr = &access->next_grp; @@ -2471,6 +2584,8 @@ analyze_access_subtree (struct access *root, struct access *parent, root->grp_assignment_write = 1; if (parent->grp_total_scalarization) root->grp_total_scalarization = 1; + if (!parent->grp_same_access_path) + root->grp_same_access_path = 0; } if (root->grp_unscalarizable_region) @@ -2721,13 +2836,17 @@ propagate_subaccesses_across_link (struct access *lacc, struct access *racc) lacc->type = racc->type; if (build_user_friendly_ref_for_offset (&t, TREE_TYPE (t), lacc->offset, racc->type)) - lacc->expr = t; + { + lacc->expr = t; + lacc->grp_same_access_path = true; + } else { lacc->expr = build_ref_for_model (EXPR_LOCATION (lacc->base), lacc->base, lacc->offset, racc, NULL, false); lacc->grp_no_warning = true; + lacc->grp_same_access_path = false; } } return ret; -- 2.21.0