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 <mjam...@suse.cz>
* 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 <mjam...@suse.cz>
* 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
