This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
[PATCH] Fix store-merging alias check, apply some TLC
- From: Richard Biener <rguenther at suse dot de>
- To: gcc-patches at gcc dot gnu dot org
- Cc: kyrylo dot tkachov at arm dot com
- Date: Wed, 2 Nov 2016 11:51:59 +0100 (CET)
- Subject: [PATCH] Fix store-merging alias check, apply some TLC
- Authentication-results: sourceware.org; auth=none
This fixes the alias check in terminate_all_aliasing_chains -- the
base we use for the hash table indexing does not constitute an object
that aliases all stores in the chain (consider for example the MEM_REF
handling which strips the offset completely).
I've refactored data structures a bit in the process of making
(as a followup) 'base_addr' a true address (and thus avoid building
that new MEM_REF for example). A followup will then try to support
(some) bases with offset != NULL_TREE.
Bootstrap / regtest running on x86_64-unknown-linux-gnu.
Richard.
2016-11-02 Richard Biener <rguenther@suse.de>
* gimple-ssa-store-merging.c (struct store_immediate_info): Remove
redundant val and dest members.
(store_immediate_info::store_immediate_info): Adjust.
(merged_store_group::merged_store_group): Adjust.
(merged_store_group::apply_stores): Likewise.
(struct imm_store_chain_info): Add base_addr field.
(imm_store_chain_info::imm_store_chain_info): New constructor.
(imm_store_chain_info::terminate_and_process_chain): Do not pass base.
(imm_store_chain_info::output_merged_store): Likewise.
(imm_store_chain_info::output_merged_stores): Likewise.
(pass_tree_store_merging::terminate_all_aliasing_chains): Take
imm_store_chain_info instead of base. Fix alias check.
(pass_tree_store_merging::terminate_and_release_chain): Likewise.
(imm_store_chain_info::coalesce_immediate_stores): Adjust.
Index: gcc/gimple-ssa-store-merging.c
===================================================================
--- gcc/gimple-ssa-store-merging.c (revision 241779)
+++ gcc/gimple-ssa-store-merging.c (working copy)
@@ -140,19 +140,17 @@ struct store_immediate_info
{
unsigned HOST_WIDE_INT bitsize;
unsigned HOST_WIDE_INT bitpos;
- tree val;
- tree dest;
gimple *stmt;
unsigned int order;
- store_immediate_info (unsigned HOST_WIDE_INT, unsigned HOST_WIDE_INT, tree,
- tree, gimple *, unsigned int);
+ store_immediate_info (unsigned HOST_WIDE_INT, unsigned HOST_WIDE_INT,
+ gimple *, unsigned int);
};
store_immediate_info::store_immediate_info (unsigned HOST_WIDE_INT bs,
- unsigned HOST_WIDE_INT bp, tree v,
- tree d, gimple *st,
+ unsigned HOST_WIDE_INT bp,
+ gimple *st,
unsigned int ord)
- : bitsize (bs), bitpos (bp), val (v), dest (d), stmt (st), order (ord)
+ : bitsize (bs), bitpos (bp), stmt (st), order (ord)
{
}
@@ -557,7 +555,7 @@ merged_store_group::merged_store_group (
/* VAL has memory allocated for it in apply_stores once the group
width has been finalized. */
val = NULL;
- align = get_object_alignment (info->dest);
+ align = get_object_alignment (gimple_assign_lhs (info->stmt));
stores.create (1);
stores.safe_push (info);
last_stmt = info->stmt;
@@ -654,14 +652,16 @@ merged_store_group::apply_stores ()
FOR_EACH_VEC_ELT (stores, i, info)
{
unsigned int pos_in_buffer = info->bitpos - start;
- bool ret = encode_tree_to_bitpos (info->val, val, info->bitsize,
- pos_in_buffer, buf_size);
+ bool ret = encode_tree_to_bitpos (gimple_assign_rhs1 (info->stmt),
+ val, info->bitsize,
+ pos_in_buffer, buf_size);
if (dump_file && (dump_flags & TDF_DETAILS))
{
if (ret)
{
fprintf (dump_file, "After writing ");
- print_generic_expr (dump_file, info->val, 0);
+ print_generic_expr (dump_file,
+ gimple_assign_rhs1 (info->stmt), 0);
fprintf (dump_file, " of size " HOST_WIDE_INT_PRINT_DEC
" at position %d the merged region contains:\n",
info->bitsize, pos_in_buffer);
@@ -680,13 +680,15 @@ merged_store_group::apply_stores ()
struct imm_store_chain_info
{
+ tree base_addr;
auto_vec<struct store_immediate_info *> m_store_info;
auto_vec<merged_store_group *> m_merged_store_groups;
- bool terminate_and_process_chain (tree);
+ imm_store_chain_info (tree b_a) : base_addr (b_a) {}
+ bool terminate_and_process_chain ();
bool coalesce_immediate_stores ();
- bool output_merged_store (tree, merged_store_group *);
- bool output_merged_stores (tree);
+ bool output_merged_store (merged_store_group *);
+ bool output_merged_stores ();
};
const pass_data pass_data_tree_store_merging = {
@@ -722,8 +724,9 @@ private:
hash_map<tree_operand_hash, struct imm_store_chain_info *> m_stores;
bool terminate_and_process_all_chains ();
- bool terminate_all_aliasing_chains (tree, tree, bool, gimple *);
- bool terminate_and_release_chain (tree);
+ bool terminate_all_aliasing_chains (tree, imm_store_chain_info **,
+ bool, gimple *);
+ bool terminate_and_release_chain (imm_store_chain_info *);
}; // class pass_store_merging
/* Terminate and process all recorded chains. Return true if any changes
@@ -736,7 +739,7 @@ pass_store_merging::terminate_and_proces
= m_stores.begin ();
bool ret = false;
for (; iter != m_stores.end (); ++iter)
- ret |= terminate_and_release_chain ((*iter).first);
+ ret |= terminate_and_release_chain ((*iter).second);
return ret;
}
@@ -750,7 +753,9 @@ pass_store_merging::terminate_and_proces
If that is the case we have to terminate any chain anchored at BASE. */
bool
-pass_store_merging::terminate_all_aliasing_chains (tree dest, tree base,
+pass_store_merging::terminate_all_aliasing_chains (tree dest,
+ imm_store_chain_info
+ **chain_info,
bool var_offset_p,
gimple *stmt)
{
@@ -760,44 +765,38 @@ pass_store_merging::terminate_all_aliasi
if (!gimple_vuse (stmt))
return false;
- struct imm_store_chain_info **chain_info = NULL;
-
/* Check if the assignment destination (BASE) is part of a store chain.
This is to catch non-constant stores to destinations that may be part
of a chain. */
- if (base)
+ if (chain_info)
{
- chain_info = m_stores.get (base);
- if (chain_info)
+ /* We have a chain at BASE and we're writing to [BASE + <variable>].
+ This can interfere with any of the stores so terminate
+ the chain. */
+ if (var_offset_p)
{
- /* We have a chain at BASE and we're writing to [BASE + <variable>].
- This can interfere with any of the stores so terminate
- the chain. */
- if (var_offset_p)
- {
- terminate_and_release_chain (base);
- ret = true;
- }
- /* Otherwise go through every store in the chain to see if it
- aliases with any of them. */
- else
+ terminate_and_release_chain (*chain_info);
+ ret = true;
+ }
+ /* Otherwise go through every store in the chain to see if it
+ aliases with any of them. */
+ else
+ {
+ struct store_immediate_info *info;
+ unsigned int i;
+ FOR_EACH_VEC_ELT ((*chain_info)->m_store_info, i, info)
{
- struct store_immediate_info *info;
- unsigned int i;
- FOR_EACH_VEC_ELT ((*chain_info)->m_store_info, i, info)
+ if (stmt_may_clobber_ref_p (info->stmt, dest))
{
- if (refs_may_alias_p (info->dest, dest))
+ if (dump_file && (dump_flags & TDF_DETAILS))
{
- if (dump_file && (dump_flags & TDF_DETAILS))
- {
- fprintf (dump_file,
- "stmt causes chain termination:\n");
- print_gimple_stmt (dump_file, stmt, 0, 0);
- }
- terminate_and_release_chain (base);
- ret = true;
- break;
+ fprintf (dump_file,
+ "stmt causes chain termination:\n");
+ print_gimple_stmt (dump_file, stmt, 0, 0);
}
+ terminate_and_release_chain (*chain_info);
+ ret = true;
+ break;
}
}
}
@@ -814,11 +813,16 @@ pass_store_merging::terminate_all_aliasi
if (chain_info && (*chain_info) == (*iter).second)
continue;
- tree key = (*iter).first;
- if (ref_maybe_used_by_stmt_p (stmt, key)
- || stmt_may_clobber_ref_p (stmt, key))
+ /* We can't use the base object here as that does not reliably exist.
+ Build a ao_ref from the base object address (if we know the
+ minimum and maximum offset and the maximum size we could improve
+ things here). */
+ ao_ref chain_ref;
+ ao_ref_init_from_ptr_and_size (&chain_ref, (*iter).first, NULL_TREE);
+ if (ref_maybe_used_by_stmt_p (stmt, &chain_ref)
+ || stmt_may_clobber_ref_p_1 (stmt, &chain_ref))
{
- terminate_and_release_chain (key);
+ terminate_and_release_chain ((*iter).second);
ret = true;
}
}
@@ -831,19 +835,11 @@ pass_store_merging::terminate_all_aliasi
entry is removed after the processing in any case. */
bool
-pass_store_merging::terminate_and_release_chain (tree base)
+pass_store_merging::terminate_and_release_chain (imm_store_chain_info *chain_info)
{
- struct imm_store_chain_info **chain_info = m_stores.get (base);
-
- if (!chain_info)
- return false;
-
- gcc_assert (*chain_info);
-
- bool ret = (*chain_info)->terminate_and_process_chain (base);
- delete *chain_info;
- m_stores.remove (base);
-
+ bool ret = chain_info->terminate_and_process_chain ();
+ m_stores.remove (chain_info->base_addr);
+ delete chain_info;
return ret;
}
@@ -880,7 +876,7 @@ imm_store_chain_info::coalesce_immediate
fprintf (dump_file, "Store %u:\nbitsize:" HOST_WIDE_INT_PRINT_DEC
" bitpos:" HOST_WIDE_INT_PRINT_DEC " val:\n",
i, info->bitsize, info->bitpos);
- print_generic_expr (dump_file, info->val, 0);
+ print_generic_expr (dump_file, gimple_assign_rhs1 (info->stmt), 0);
fprintf (dump_file, "\n------------\n");
}
@@ -1103,7 +1099,7 @@ split_group (merged_store_group *group,
return true. */
bool
-imm_store_chain_info::output_merged_store (tree base, merged_store_group *group)
+imm_store_chain_info::output_merged_store (merged_store_group *group)
{
unsigned HOST_WIDE_INT start_byte_pos = group->start / BITS_PER_UNIT;
@@ -1141,7 +1137,7 @@ imm_store_chain_info::output_merged_stor
tree int_type = build_nonstandard_integer_type (try_size, UNSIGNED);
int_type = build_aligned_type (int_type, align);
- tree addr = build_fold_addr_expr (base);
+ tree addr = build_fold_addr_expr (base_addr);
tree dest = fold_build2 (MEM_REF, int_type, addr,
build_int_cst (offset_type, try_pos));
@@ -1213,14 +1209,14 @@ imm_store_chain_info::output_merged_stor
successful. Return true iff any changes were made. */
bool
-imm_store_chain_info::output_merged_stores (tree base)
+imm_store_chain_info::output_merged_stores ()
{
unsigned int i;
merged_store_group *merged_store;
bool ret = false;
FOR_EACH_VEC_ELT (m_merged_store_groups, i, merged_store)
{
- if (output_merged_store (base, merged_store))
+ if (output_merged_store (merged_store))
{
unsigned int j;
store_immediate_info *store;
@@ -1250,7 +1246,7 @@ imm_store_chain_info::output_merged_stor
Return true if any changes were made. */
bool
-imm_store_chain_info::terminate_and_process_chain (tree base)
+imm_store_chain_info::terminate_and_process_chain ()
{
/* Process store chain. */
bool ret = false;
@@ -1258,7 +1254,7 @@ imm_store_chain_info::terminate_and_proc
{
ret = coalesce_immediate_stores ();
if (ret)
- ret = output_merged_stores (base);
+ ret = output_merged_stores ();
}
/* Delete all the entries we allocated ourselves. */
@@ -1413,7 +1409,7 @@ pass_store_merging::execute (function *f
if (chain_info)
{
info = new store_immediate_info (
- bitsize, bitpos, rhs, lhs, stmt,
+ bitsize, bitpos, stmt,
(*chain_info)->m_store_info.length ());
if (dump_file && (dump_flags & TDF_DETAILS))
{
@@ -1432,17 +1428,17 @@ pass_store_merging::execute (function *f
fprintf (dump_file,
"Reached maximum number of statements"
" to merge:\n");
- terminate_and_release_chain (base_addr);
+ terminate_and_release_chain (*chain_info);
}
continue;
}
/* Store aliases any existing chain? */
- terminate_all_aliasing_chains (lhs, base_addr, false, stmt);
+ terminate_all_aliasing_chains (lhs, chain_info, false, stmt);
/* Start a new chain. */
struct imm_store_chain_info *new_chain
- = new imm_store_chain_info;
- info = new store_immediate_info (bitsize, bitpos, rhs, lhs,
+ = new imm_store_chain_info (base_addr);
+ info = new store_immediate_info (bitsize, bitpos,
stmt, 0);
new_chain->m_store_info.safe_push (info);
m_stores.put (base_addr, new_chain);
@@ -1457,13 +1453,13 @@ pass_store_merging::execute (function *f
}
}
else
- terminate_all_aliasing_chains (lhs, base_addr,
+ terminate_all_aliasing_chains (lhs, chain_info,
offset != NULL_TREE, stmt);
continue;
}
- terminate_all_aliasing_chains (NULL_TREE, NULL_TREE, false, stmt);
+ terminate_all_aliasing_chains (NULL_TREE, NULL, false, stmt);
}
terminate_and_process_all_chains ();
}