* [PATCH] Fix profiledbootstrap - store-merging aliasing issue (PR bootstrap/82916)
@ 2017-11-09 21:52 Jakub Jelinek
2017-11-10 8:02 ` Richard Biener
0 siblings, 1 reply; 4+ messages in thread
From: Jakub Jelinek @ 2017-11-09 21:52 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches
Hi!
We want to terminate a chain if a chain with different base (or insn
outside of any chain) has a store that the current stmt might use, or
overwrite. The functions it used didn't cover the store after store
case which in the middle-end aliasing model needs to avoid tbaa, because
the latter store might be after a placement new or might otherwise change
the dynamic object type of the object.
The following patch does that. Bootstrapped/regtested on x86_64-linux and
i686-linux, additionally profiledbootstrapped with the configure options
Markus mentioned in the PR. Ok for trunk?
2017-11-09 Jakub Jelinek <jakub@redhat.com>
PR bootstrap/82916
* gimple-ssa-store-merging.c
(pass_store_merging::terminate_all_aliasing_chains): For
gimple_store_p stmts also call refs_output_dependent_p.
* gcc.dg/store_merging_2.c: Only expect 2 successful mergings instead
of 3.
* gcc.dg/pr82916.c: New test.
--- gcc/gimple-ssa-store-merging.c.jj 2017-11-09 15:51:08.000000000 +0100
+++ gcc/gimple-ssa-store-merging.c 2017-11-09 18:01:20.789236951 +0100
@@ -945,6 +945,7 @@ pass_store_merging::terminate_all_aliasi
if (!gimple_vuse (stmt))
return false;
+ tree store_lhs = gimple_store_p (stmt) ? gimple_get_lhs (stmt) : NULL_TREE;
for (imm_store_chain_info *next = m_stores_head, *cur = next; cur; cur = next)
{
next = cur->next;
@@ -958,8 +959,10 @@ pass_store_merging::terminate_all_aliasi
unsigned int i;
FOR_EACH_VEC_ELT (cur->m_store_info, i, info)
{
- if (ref_maybe_used_by_stmt_p (stmt, gimple_assign_lhs (info->stmt))
- || stmt_may_clobber_ref_p (stmt, gimple_assign_lhs (info->stmt)))
+ tree lhs = gimple_assign_lhs (info->stmt);
+ if (ref_maybe_used_by_stmt_p (stmt, lhs)
+ || stmt_may_clobber_ref_p (stmt, lhs)
+ || (store_lhs && refs_output_dependent_p (store_lhs, lhs)))
{
if (dump_file && (dump_flags & TDF_DETAILS))
{
--- gcc/testsuite/gcc.dg/store_merging_2.c.jj 2017-11-08 16:46:19.000000000 +0100
+++ gcc/testsuite/gcc.dg/store_merging_2.c 2017-11-09 18:16:33.482344795 +0100
@@ -77,4 +77,4 @@ main (void)
return 0;
}
-/* { dg-final { scan-tree-dump-times "Merging successful" 3 "store-merging" } } */
+/* { dg-final { scan-tree-dump-times "Merging successful" 2 "store-merging" } } */
--- gcc/testsuite/gcc.dg/pr82916.c.jj 2017-11-09 18:12:40.707128841 +0100
+++ gcc/testsuite/gcc.dg/pr82916.c 2017-11-09 18:12:19.000000000 +0100
@@ -0,0 +1,47 @@
+/* PR bootstrap/82916 */
+/* { dg-do run } */
+/* { dg-options "-O2 -fno-tree-dse" } */
+
+struct A { struct A *next; };
+struct C
+{
+ int *of;
+ struct C *parent, *prev, *next;
+ int depth;
+ int min;
+ struct C *min_occ;
+};
+
+__attribute__((noipa)) struct C *
+foo (int *node)
+{
+ struct A *p = __builtin_malloc (sizeof (struct C));
+ if (!p)
+ return 0;
+ p->next = 0;
+ /* Originally placement new. */
+ struct C *nw = (struct C *)(void *)p;
+ nw->of = node;
+ nw->parent = 0;
+ nw->prev = 0;
+ nw->next = 0;
+ nw->depth = 0;
+ nw->min_occ = nw;
+ nw->min = 0;
+ return nw;
+}
+
+int
+main ()
+{
+ int o;
+ struct C *p = foo (&o);
+ if (p)
+ {
+ if (p->of != &o || p->parent || p->prev || p->next || p->depth
+ || p->min || p->min_occ != p)
+ __builtin_abort ();
+ }
+ __builtin_free (p);
+ return 0;
+}
Jakub
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Fix profiledbootstrap - store-merging aliasing issue (PR bootstrap/82916)
2017-11-09 21:52 [PATCH] Fix profiledbootstrap - store-merging aliasing issue (PR bootstrap/82916) Jakub Jelinek
@ 2017-11-10 8:02 ` Richard Biener
2017-11-10 8:29 ` Jakub Jelinek
0 siblings, 1 reply; 4+ messages in thread
From: Richard Biener @ 2017-11-10 8:02 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
On Thu, 9 Nov 2017, Jakub Jelinek wrote:
> Hi!
>
> We want to terminate a chain if a chain with different base (or insn
> outside of any chain) has a store that the current stmt might use, or
> overwrite. The functions it used didn't cover the store after store
> case which in the middle-end aliasing model needs to avoid tbaa, because
> the latter store might be after a placement new or might otherwise change
> the dynamic object type of the object.
>
> The following patch does that. Bootstrapped/regtested on x86_64-linux and
> i686-linux, additionally profiledbootstrapped with the configure options
> Markus mentioned in the PR. Ok for trunk?
>
> 2017-11-09 Jakub Jelinek <jakub@redhat.com>
>
> PR bootstrap/82916
> * gimple-ssa-store-merging.c
> (pass_store_merging::terminate_all_aliasing_chains): For
> gimple_store_p stmts also call refs_output_dependent_p.
>
> * gcc.dg/store_merging_2.c: Only expect 2 successful mergings instead
> of 3.
> * gcc.dg/pr82916.c: New test.
>
> --- gcc/gimple-ssa-store-merging.c.jj 2017-11-09 15:51:08.000000000 +0100
> +++ gcc/gimple-ssa-store-merging.c 2017-11-09 18:01:20.789236951 +0100
> @@ -945,6 +945,7 @@ pass_store_merging::terminate_all_aliasi
> if (!gimple_vuse (stmt))
> return false;
>
> + tree store_lhs = gimple_store_p (stmt) ? gimple_get_lhs (stmt) : NULL_TREE;
> for (imm_store_chain_info *next = m_stores_head, *cur = next; cur; cur = next)
> {
> next = cur->next;
> @@ -958,8 +959,10 @@ pass_store_merging::terminate_all_aliasi
> unsigned int i;
> FOR_EACH_VEC_ELT (cur->m_store_info, i, info)
> {
> - if (ref_maybe_used_by_stmt_p (stmt, gimple_assign_lhs (info->stmt))
> - || stmt_may_clobber_ref_p (stmt, gimple_assign_lhs (info->stmt)))
> + tree lhs = gimple_assign_lhs (info->stmt);
> + if (ref_maybe_used_by_stmt_p (stmt, lhs)
> + || stmt_may_clobber_ref_p (stmt, lhs)
> + || (store_lhs && refs_output_dependent_p (store_lhs, lhs)))
Looks good but may do redundant work for store_lhs? So rather
|| (! store_lhs && stmt_may_clobber_ref_p (stmt, lhs)
|| (store_lhs && refs_output_dependent_p (store_lhs, lhs)
? Fails to handle storing calls (in case those can appear in the chains).
Looks like we miss some convenient stmt_output/anti_dependent_p (you can
follow stmt_may_clobbers_ref_p[_1] for cut&pasting and/or add a
bool tbaa flag we can pass down to stmt_may_clobber_ref_p_1).
That said - the patch is ok, any improvements can be done as followup.
Thanks,
Richard.
> {
> if (dump_file && (dump_flags & TDF_DETAILS))
> {
> --- gcc/testsuite/gcc.dg/store_merging_2.c.jj 2017-11-08 16:46:19.000000000 +0100
> +++ gcc/testsuite/gcc.dg/store_merging_2.c 2017-11-09 18:16:33.482344795 +0100
> @@ -77,4 +77,4 @@ main (void)
> return 0;
> }
>
> -/* { dg-final { scan-tree-dump-times "Merging successful" 3 "store-merging" } } */
> +/* { dg-final { scan-tree-dump-times "Merging successful" 2 "store-merging" } } */
> --- gcc/testsuite/gcc.dg/pr82916.c.jj 2017-11-09 18:12:40.707128841 +0100
> +++ gcc/testsuite/gcc.dg/pr82916.c 2017-11-09 18:12:19.000000000 +0100
> @@ -0,0 +1,47 @@
> +/* PR bootstrap/82916 */
> +/* { dg-do run } */
> +/* { dg-options "-O2 -fno-tree-dse" } */
> +
> +struct A { struct A *next; };
> +struct C
> +{
> + int *of;
> + struct C *parent, *prev, *next;
> + int depth;
> + int min;
> + struct C *min_occ;
> +};
> +
> +__attribute__((noipa)) struct C *
> +foo (int *node)
> +{
> + struct A *p = __builtin_malloc (sizeof (struct C));
> + if (!p)
> + return 0;
> + p->next = 0;
> + /* Originally placement new. */
> + struct C *nw = (struct C *)(void *)p;
> + nw->of = node;
> + nw->parent = 0;
> + nw->prev = 0;
> + nw->next = 0;
> + nw->depth = 0;
> + nw->min_occ = nw;
> + nw->min = 0;
> + return nw;
> +}
> +
> +int
> +main ()
> +{
> + int o;
> + struct C *p = foo (&o);
> + if (p)
> + {
> + if (p->of != &o || p->parent || p->prev || p->next || p->depth
> + || p->min || p->min_occ != p)
> + __builtin_abort ();
> + }
> + __builtin_free (p);
> + return 0;
> +}
>
> Jakub
>
>
--
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Fix profiledbootstrap - store-merging aliasing issue (PR bootstrap/82916)
2017-11-10 8:02 ` Richard Biener
@ 2017-11-10 8:29 ` Jakub Jelinek
2017-11-10 9:37 ` Richard Biener
0 siblings, 1 reply; 4+ messages in thread
From: Jakub Jelinek @ 2017-11-10 8:29 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches
On Fri, Nov 10, 2017 at 08:52:16AM +0100, Richard Biener wrote:
> > @@ -958,8 +959,10 @@ pass_store_merging::terminate_all_aliasi
> > unsigned int i;
> > FOR_EACH_VEC_ELT (cur->m_store_info, i, info)
> > {
> > - if (ref_maybe_used_by_stmt_p (stmt, gimple_assign_lhs (info->stmt))
> > - || stmt_may_clobber_ref_p (stmt, gimple_assign_lhs (info->stmt)))
> > + tree lhs = gimple_assign_lhs (info->stmt);
> > + if (ref_maybe_used_by_stmt_p (stmt, lhs)
> > + || stmt_may_clobber_ref_p (stmt, lhs)
> > + || (store_lhs && refs_output_dependent_p (store_lhs, lhs)))
>
> Looks good but may do redundant work for store_lhs? So rather
>
> || (! store_lhs && stmt_may_clobber_ref_p (stmt, lhs)
> || (store_lhs && refs_output_dependent_p (store_lhs, lhs)
>
> ? Fails to handle storing calls (in case those can appear in the chains).
info->stmt is known to be a store, but stmt is not, it can be any other
stmt, including calls, so the above would miss the calls handling.
> Looks like we miss some convenient stmt_output/anti_dependent_p (you can
> follow stmt_may_clobbers_ref_p[_1] for cut&pasting and/or add a
> bool tbaa flag we can pass down to stmt_may_clobber_ref_p_1).
So perhaps bool tbaa = true argument to both stmt_may_clobber_ref_p_1
and stmt_may_clobber_ref_p, or just stmt_may_clobber_ref_p_1 and
add some differently named alternative to stmt_may_clobber_ref_p
(in that case, any suggestions on a good name?)?
> That said - the patch is ok, any improvements can be done as followup.
Jakub
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Fix profiledbootstrap - store-merging aliasing issue (PR bootstrap/82916)
2017-11-10 8:29 ` Jakub Jelinek
@ 2017-11-10 9:37 ` Richard Biener
0 siblings, 0 replies; 4+ messages in thread
From: Richard Biener @ 2017-11-10 9:37 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
On Fri, 10 Nov 2017, Jakub Jelinek wrote:
> On Fri, Nov 10, 2017 at 08:52:16AM +0100, Richard Biener wrote:
> > > @@ -958,8 +959,10 @@ pass_store_merging::terminate_all_aliasi
> > > unsigned int i;
> > > FOR_EACH_VEC_ELT (cur->m_store_info, i, info)
> > > {
> > > - if (ref_maybe_used_by_stmt_p (stmt, gimple_assign_lhs (info->stmt))
> > > - || stmt_may_clobber_ref_p (stmt, gimple_assign_lhs (info->stmt)))
> > > + tree lhs = gimple_assign_lhs (info->stmt);
> > > + if (ref_maybe_used_by_stmt_p (stmt, lhs)
> > > + || stmt_may_clobber_ref_p (stmt, lhs)
> > > + || (store_lhs && refs_output_dependent_p (store_lhs, lhs)))
> >
> > Looks good but may do redundant work for store_lhs? So rather
> >
> > || (! store_lhs && stmt_may_clobber_ref_p (stmt, lhs)
> > || (store_lhs && refs_output_dependent_p (store_lhs, lhs)
> >
> > ? Fails to handle storing calls (in case those can appear in the chains).
>
> info->stmt is known to be a store, but stmt is not, it can be any other
> stmt, including calls, so the above would miss the calls handling.
>
> > Looks like we miss some convenient stmt_output/anti_dependent_p (you can
> > follow stmt_may_clobbers_ref_p[_1] for cut&pasting and/or add a
> > bool tbaa flag we can pass down to stmt_may_clobber_ref_p_1).
>
> So perhaps bool tbaa = true argument to both stmt_may_clobber_ref_p_1
> and stmt_may_clobber_ref_p, or just stmt_may_clobber_ref_p_1 and
> add some differently named alternative to stmt_may_clobber_ref_p
> (in that case, any suggestions on a good name?)?
Internally (aka static fn) I'd just add a bool param w/o default.
The external API should be stmt_output/anti_dependent_p and I think
these days with C++ we could make stmt_may_clobber_ref_p_1 taking ao_ref
and stmt_may_clobber_ref_p taking a tree overloads of
stmt_may_clobber_ref_p. We'd then have _1 being static and having the
extra arg.
Richard.
> > That said - the patch is ok, any improvements can be done as followup.
>
> Jakub
>
>
--
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-11-10 8:29 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-09 21:52 [PATCH] Fix profiledbootstrap - store-merging aliasing issue (PR bootstrap/82916) Jakub Jelinek
2017-11-10 8:02 ` Richard Biener
2017-11-10 8:29 ` Jakub Jelinek
2017-11-10 9:37 ` Richard Biener
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).