Re: SRA: don't drop clobbers
On Thu, Nov 20, 2014 at 7:11 PM, Martin Jambor mjam...@suse.cz wrote: Hi, On Mon, Nov 03, 2014 at 10:46:49PM +0100, Marc Glisse wrote: On Mon, 3 Nov 2014, Marc Glisse wrote: On Mon, 3 Nov 2014, Martin Jambor wrote: I just applied your patch on top of trunk revision 217032 on my Ah, that explains it, thanks. This patch is a follow-up to r217034. Still, I didn't expect the ICE you are seeing by applying this patch to older trunk, I'll try to reproduce that. It is TODO_update_address_taken that used to remove clobbers, and as you said ESRA goes straight to TODO_update_ssa, which explains why the clobbers caused trouble. In any case, after r217034, update_ssa should handle clobbers much better. Could you take an other look based on a more recent trunk, please? Sorry for the delay. Anyway, on the current trunk (i.e. Tuesday checkout) the patch works as expected, there are assignments from default definitions now and even though we do not warn as we should, the patch improves the generated code. The function foo from the testcase is optimized to return SR.1_2(D); as soon as release_ssa now, whereas unpatched trunk leaves an undefined load even in the optimized dump. Thus, I like the patch and given that you posted it well before stage1 end, I'd like to see it committed. Richi, can you have a look and perhaps approve it? Yes, the patch is ok. Thanks, Richard. Thanks, Martin
Re: SRA: don't drop clobbers
Hi, On Mon, Nov 03, 2014 at 10:46:49PM +0100, Marc Glisse wrote: On Mon, 3 Nov 2014, Marc Glisse wrote: On Mon, 3 Nov 2014, Martin Jambor wrote: I just applied your patch on top of trunk revision 217032 on my Ah, that explains it, thanks. This patch is a follow-up to r217034. Still, I didn't expect the ICE you are seeing by applying this patch to older trunk, I'll try to reproduce that. It is TODO_update_address_taken that used to remove clobbers, and as you said ESRA goes straight to TODO_update_ssa, which explains why the clobbers caused trouble. In any case, after r217034, update_ssa should handle clobbers much better. Could you take an other look based on a more recent trunk, please? Sorry for the delay. Anyway, on the current trunk (i.e. Tuesday checkout) the patch works as expected, there are assignments from default definitions now and even though we do not warn as we should, the patch improves the generated code. The function foo from the testcase is optimized to return SR.1_2(D); as soon as release_ssa now, whereas unpatched trunk leaves an undefined load even in the optimized dump. Thus, I like the patch and given that you posted it well before stage1 end, I'd like to see it committed. Richi, can you have a look and perhaps approve it? Thanks, Martin
SRA: don't drop clobbers
Hello,
now that the update_address_taken patch is in, let me re-post the SRA
follow-up. With this patch, testcase pr60517.C (attached) has a use of an
undefined variable at the time of the uninit pass. Sadly, while this
warned with earlier versions of the other patch (when I was inserting
default definitions of new variables), it does not anymore because we have
TREE_NO_WARNING all over the place. Still, I believe this is a step in the
right direction, and it passed bootstrap+testsuite (minus the new
testcase). Would it be ok to commit the tree-sra.c change? Then I could
close PR60770 (the warning itself is PR60517).
2014-11-03 Marc Glisse marc.gli...@inria.fr
PR tree-optimization/60770
* tree-sra.c (clobber_subtree): New function.
(sra_modify_constructor_assign): Call it.
--
Marc GlisseIndex: gcc/testsuite/g++.dg/tree-ssa/pr60517.C
===
--- gcc/testsuite/g++.dg/tree-ssa/pr60517.C (revision 0)
+++ gcc/testsuite/g++.dg/tree-ssa/pr60517.C (working copy)
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+/* { dg-options -O -Wall } */
+
+struct B {
+double x[2];
+};
+struct A {
+B b;
+B getB() { return b; }
+};
+
+double foo(A a) {
+double * x = (a.getB().x[0]);
+return x[0]; /* { dg-warning } */
+}
Index: gcc/tree-sra.c
===
--- gcc/tree-sra.c (revision 217035)
+++ gcc/tree-sra.c (working copy)
@@ -2716,20 +2716,51 @@ init_subtree_with_zero (struct access *a
if (insert_after)
gsi_insert_after (gsi, ds, GSI_NEW_STMT);
else
gsi_insert_before (gsi, ds, GSI_SAME_STMT);
}
for (child = access-first_child; child; child = child-next_sibling)
init_subtree_with_zero (child, gsi, insert_after, loc);
}
+/* Clobber all scalar replacements in an access subtree. ACCESS is the the
+ root of the subtree to be processed. GSI is the statement iterator used
+ for inserting statements which are added after the current statement if
+ INSERT_AFTER is true or before it otherwise. */
+
+static void
+clobber_subtree (struct access *access, gimple_stmt_iterator *gsi,
+ bool insert_after, location_t loc)
+
+{
+ struct access *child;
+
+ if (access-grp_to_be_replaced)
+{
+ tree rep = get_access_replacement (access);
+ tree clobber = build_constructor (access-type, NULL);
+ TREE_THIS_VOLATILE (clobber) = 1;
+ gimple stmt = gimple_build_assign (rep, clobber);
+
+ if (insert_after)
+ gsi_insert_after (gsi, stmt, GSI_NEW_STMT);
+ else
+ gsi_insert_before (gsi, stmt, GSI_SAME_STMT);
+ update_stmt (stmt);
+ gimple_set_location (stmt, loc);
+}
+
+ for (child = access-first_child; child; child = child-next_sibling)
+clobber_subtree (child, gsi, insert_after, loc);
+}
+
/* Search for an access representative for the given expression EXPR and
return it or NULL if it cannot be found. */
static struct access *
get_access_for_expr (tree expr)
{
HOST_WIDE_INT offset, size, max_size;
tree base;
/* FIXME: This should not be necessary but Ada produces V_C_Es with a type of
@@ -3028,43 +3059,41 @@ enum assignment_mod_result { SRA_AM_NONE
SRA_AM_REMOVED }; /* stmt eliminated */
/* Modify assignments with a CONSTRUCTOR on their RHS. STMT contains a pointer
to the assignment and GSI is the statement iterator pointing at it. Returns
the same values as sra_modify_assign. */
static enum assignment_mod_result
sra_modify_constructor_assign (gimple stmt, gimple_stmt_iterator *gsi)
{
tree lhs = gimple_assign_lhs (stmt);
- struct access *acc;
- location_t loc;
-
- acc = get_access_for_expr (lhs);
+ struct access *acc = get_access_for_expr (lhs);
if (!acc)
return SRA_AM_NONE;
+ location_t loc = gimple_location (stmt);
if (gimple_clobber_p (stmt))
{
- /* Remove clobbers of fully scalarized variables, otherwise
-do nothing. */
+ /* Clobber the replacement variable. */
+ clobber_subtree (acc, gsi, !acc-grp_covered, loc);
+ /* Remove clobbers of fully scalarized variables, they are dead. */
if (acc-grp_covered)
{
unlink_stmt_vdef (stmt);
gsi_remove (gsi, true);
release_defs (stmt);
return SRA_AM_REMOVED;
}
else
- return SRA_AM_NONE;
+ return SRA_AM_MODIFIED;
}
- loc = gimple_location (stmt);
if (vec_safe_length (CONSTRUCTOR_ELTS (gimple_assign_rhs1 (stmt))) 0)
{
/* I have never seen this code path trigger but if it can happen the
following should handle it gracefully. */
if (access_has_children_p (acc))
generate_subtree_copies (acc-first_child, lhs, acc-offset, 0, 0, gsi,
true, true, loc);
return SRA_AM_MODIFIED;
}
Re: SRA: don't drop clobbers
Hi,
On Mon, Nov 03, 2014 at 01:59:24PM +0100, Marc Glisse wrote:
Hello,
now that the update_address_taken patch is in, let me re-post the
SRA follow-up. With this patch, testcase pr60517.C (attached) has a
use of an undefined variable at the time of the uninit pass. Sadly,
while this warned with earlier versions of the other patch (when I
was inserting default definitions of new variables), it does not
anymore because we have TREE_NO_WARNING all over the place. Still, I
believe this is a step in the right direction, and it passed
bootstrap+testsuite (minus the new testcase).
I am not sure what you mean by minus the new testcase but, with the
gimple checking compiler at least, the testcase produces an ICE:
pr60517.C: In function ‘double foo(A)’:
pr60517.C:15:1: error: non-decl/MEM_REF LHS in clobber statement
}
^
SR.1_8
SR.1_8 ={v} {CLOBBER};
pr60517.C:15:1: internal compiler error: verify_gimple failed
0xc7810d verify_gimple_in_cfg(function*, bool)
/home/mjambor/gcc/mine/src/gcc/tree-cfg.c:5039
0xb8256f execute_function_todo
/home/mjambor/gcc/mine/src/gcc/passes.c:1758
0xb83183 execute_todo
/home/mjambor/gcc/mine/src/gcc/passes.c:1815
The problem is exactly what the verifier complains about, a clobber of
an SSA_NAME which I agree with the verifier is a really bad idea.
If you want SRA to produce information that at a given point a scalar
replacement (which will become an SSA_NAME) does not have any valid
data, the way to represent that is to load it with a value from a
default-definition SSA name. Something along the lines of
get_repl_default_def_ssa_name which unfortunately you cannot directly
use without confusing the SSA renamer.
The easiest way, at least to test a proof of concept, is probably to
create another declaration of the same type (reusing bits from
create_access_replacement, especially setting the same
DECL_DEBUG_EXPR), get its default-definition using
get_or_create_ssa_default_def and then assign that to the replacement
instead of trying to clobber it.
(A more correct approach would be to design a way of telling the
renamer that at this point the scalar becomes undefined and that it
should start using default-defs until the next definition appears but
that is likely to be difficult, although that is just my guess, it
might be easy.)
Would it be ok to commit the tree-sra.c change?
I don't think so, sorry about the bad news.
Martin
Re: SRA: don't drop clobbers
On Mon, 3 Nov 2014, Martin Jambor wrote:
On Mon, Nov 03, 2014 at 01:59:24PM +0100, Marc Glisse wrote:
Hello,
now that the update_address_taken patch is in, let me re-post the
SRA follow-up. With this patch, testcase pr60517.C (attached) has a
use of an undefined variable at the time of the uninit pass. Sadly,
while this warned with earlier versions of the other patch (when I
was inserting default definitions of new variables), it does not
anymore because we have TREE_NO_WARNING all over the place. Still, I
believe this is a step in the right direction, and it passed
bootstrap+testsuite (minus the new testcase).
I am not sure what you mean by minus the new testcase but, with the
gimple checking compiler at least, the testcase produces an ICE:
pr60517.C: In function ‘double foo(A)’:
pr60517.C:15:1: error: non-decl/MEM_REF LHS in clobber statement
}
^
SR.1_8
SR.1_8 ={v} {CLOBBER};
pr60517.C:15:1: internal compiler error: verify_gimple failed
0xc7810d verify_gimple_in_cfg(function*, bool)
/home/mjambor/gcc/mine/src/gcc/tree-cfg.c:5039
0xb8256f execute_function_todo
/home/mjambor/gcc/mine/src/gcc/passes.c:1758
0xb83183 execute_todo
/home/mjambor/gcc/mine/src/gcc/passes.c:1815
The problem is exactly what the verifier complains about, a clobber of
an SSA_NAME which I agree with the verifier is a really bad idea.
If you want SRA to produce information that at a given point a scalar
replacement (which will become an SSA_NAME) does not have any valid
data, the way to represent that is to load it with a value from a
default-definition SSA name. Something along the lines of
get_repl_default_def_ssa_name which unfortunately you cannot directly
use without confusing the SSA renamer.
The easiest way, at least to test a proof of concept, is probably to
create another declaration of the same type (reusing bits from
create_access_replacement, especially setting the same
DECL_DEBUG_EXPR), get its default-definition using
get_or_create_ssa_default_def and then assign that to the replacement
instead of trying to clobber it.
(A more correct approach would be to design a way of telling the
renamer that at this point the scalar becomes undefined and that it
should start using default-defs until the next definition appears but
that is likely to be difficult, although that is just my guess, it
might be easy.)
Would it be ok to commit the tree-sra.c change?
I don't think so, sorry about the bad news.
Thanks for your help. I am quite confused. I am not seeing the failure you
mention, but since I am building from trunk with no specific option I
should be getting gimple checking.
The way I expect things to work:
1) we have a clobber for a variable V
2) SRA creates a variable SR and replaces uses of V
3) (new) next to V's clobber, we insert a clobber for SR
4) update_address_taken notices that SR does not have its address taken,
and thus replaces it by SSA_NAMEs, and after the clobber by an undefined
variable (yesterday it would just have removed the clobber).
What you describe in a parenthesis about the renamer is pretty much what I
think rewrite_update_stmt does, with the clobber as the hint.
Do you have further comments, based on these explanations, that could help
me pinpoint what is going wrong?
--
Marc Glisse
Re: SRA: don't drop clobbers
Hi,
On Mon, Nov 03, 2014 at 05:17:22PM +0100, Marc Glisse wrote:
On Mon, 3 Nov 2014, Martin Jambor wrote:
On Mon, Nov 03, 2014 at 01:59:24PM +0100, Marc Glisse wrote:
now that the update_address_taken patch is in, let me re-post the
SRA follow-up. With this patch, testcase pr60517.C (attached) has a
use of an undefined variable at the time of the uninit pass. Sadly,
while this warned with earlier versions of the other patch (when I
was inserting default definitions of new variables), it does not
anymore because we have TREE_NO_WARNING all over the place. Still, I
believe this is a step in the right direction, and it passed
bootstrap+testsuite (minus the new testcase).
I am not sure what you mean by minus the new testcase but, with the
gimple checking compiler at least, the testcase produces an ICE:
pr60517.C: In function ‘double foo(A)’:
pr60517.C:15:1: error: non-decl/MEM_REF LHS in clobber statement
}
^
SR.1_8
SR.1_8 ={v} {CLOBBER};
pr60517.C:15:1: internal compiler error: verify_gimple failed
0xc7810d verify_gimple_in_cfg(function*, bool)
/home/mjambor/gcc/mine/src/gcc/tree-cfg.c:5039
0xb8256f execute_function_todo
/home/mjambor/gcc/mine/src/gcc/passes.c:1758
0xb83183 execute_todo
/home/mjambor/gcc/mine/src/gcc/passes.c:1815
The problem is exactly what the verifier complains about, a clobber of
an SSA_NAME which I agree with the verifier is a really bad idea.
If you want SRA to produce information that at a given point a scalar
replacement (which will become an SSA_NAME) does not have any valid
data, the way to represent that is to load it with a value from a
default-definition SSA name. Something along the lines of
get_repl_default_def_ssa_name which unfortunately you cannot directly
use without confusing the SSA renamer.
The easiest way, at least to test a proof of concept, is probably to
create another declaration of the same type (reusing bits from
create_access_replacement, especially setting the same
DECL_DEBUG_EXPR), get its default-definition using
get_or_create_ssa_default_def and then assign that to the replacement
instead of trying to clobber it.
(A more correct approach would be to design a way of telling the
renamer that at this point the scalar becomes undefined and that it
should start using default-defs until the next definition appears but
that is likely to be difficult, although that is just my guess, it
might be easy.)
Would it be ok to commit the tree-sra.c change?
I don't think so, sorry about the bad news.
Thanks for your help. I am quite confused. I am not seeing the
failure you mention, but since I am building from trunk with no
specific option I should be getting gimple checking.
The way I expect things to work:
1) we have a clobber for a variable V
2) SRA creates a variable SR and replaces uses of V
3) (new) next to V's clobber, we insert a clobber for SR
4) update_address_taken notices that SR does not have its address
taken, and thus replaces it by SSA_NAMEs, and after the clobber by
an undefined variable (yesterday it would just have removed the
clobber).
update_address_taken? The conversion to SSA form is done by means of
returning TODO_update_ssa from the (early) SRA pass (which is what I
mean by the renamer).
What you describe in a parenthesis about the renamer is pretty much
what I think rewrite_update_stmt does, with the clobber as the hint.
Do you have further comments, based on these explanations, that
could help me pinpoint what is going wrong?
I just applied your patch on top of trunk revision 217032 on my
x86_64-linux (openSUSE 13.1) desktop, configured with
/home/mjambor/gcc/mine/src/configure --prefix=/home/mjambor/gcc/mine/inst
--enable-languages=c,c++ --enable-checking=yes --disable-bootstrap
--with-plugin-ld=/home/mjambor/binutils/obj/gold/ld-new --disable-libsanitizer
--disable-multilib
built with simple make -j8,
and ran the testcase with make -k check RUNTESTFLAGS=dg.exp=pr60517.C
And the error popped out in gcc/testsuite/g++/g++.log. I really can't
think of reason why you don't see it.
But I do and little wonder, really, given the second condition in
verify_gimple_assign_single, which is there for more than a year.
Looking at the dumped statement it seems that the LHS is already an
SSA_NAME. Moreover, when I dumped the erroneous body in gdb, I have
not seen any default definitions anywhere. So if your intention was
to teach the renamer to turn this into default definitions, it does
not work and update_ssa did not get the hint.
Martin
Re: SRA: don't drop clobbers
On Mon, 3 Nov 2014, Martin Jambor wrote: I just applied your patch on top of trunk revision 217032 on my Ah, that explains it, thanks. This patch is a follow-up to r217034. Still, I didn't expect the ICE you are seeing by applying this patch to older trunk, I'll try to reproduce that. -- Marc Glisse
Re: SRA: don't drop clobbers
On Mon, 3 Nov 2014, Marc Glisse wrote: On Mon, 3 Nov 2014, Martin Jambor wrote: I just applied your patch on top of trunk revision 217032 on my Ah, that explains it, thanks. This patch is a follow-up to r217034. Still, I didn't expect the ICE you are seeing by applying this patch to older trunk, I'll try to reproduce that. It is TODO_update_address_taken that used to remove clobbers, and as you said ESRA goes straight to TODO_update_ssa, which explains why the clobbers caused trouble. In any case, after r217034, update_ssa should handle clobbers much better. Could you take an other look based on a more recent trunk, please? -- Marc Glisse
Re: SRA: don't drop clobbers
On Mon, Jun 30, 2014 at 1:38 AM, Marc Glisse marc.gli...@inria.fr wrote:
Hello,
with this patch on top of
https://gcc.gnu.org/ml/gcc-patches/2014-06/msg02315.html
we finally warn for the testcase of PR 60517.
The new function is copied from init_subtree_with_zero right above. I guess
it might be possible to merge them into a single function, if desired. I
don't understand the debug stuff, but hopefully by keeping the functions
similar enough it shouldn't be too broken.
When we see a clobber during scalarization, instead of dropping it, we add a
clobber to the new variable, which the previous patch turns into an SSA_NAME
with a default def. Then either we reach uninit and warn, or the variable
appears in a PHI and CCP optimizes.
Bootstrap+testsuite (all,obj-c++,ada,go) on x86_64-linux-gnu.
2014-07-01 Marc Glisse marc.gli...@inria.fr
PR c++/60517
PR tree-optimization/60770
gcc/
* tree-sra.c (clobber_subtree): New function.
(sra_modify_constructor_assign): Call it.
gcc/testsuite/
* g++.dg/tree-ssa/pr60517.C: New file.
--
Marc Glisse
Index: gcc/tree-sra.c
===
--- gcc/tree-sra.c (revision 212126)
+++ gcc/tree-sra.c (working copy)
@@ -2727,20 +2727,58 @@ init_subtree_with_zero (struct access *a
if (insert_after)
gsi_insert_after (gsi, ds, GSI_NEW_STMT);
else
gsi_insert_before (gsi, ds, GSI_SAME_STMT);
}
for (child = access-first_child; child; child = child-next_sibling)
init_subtree_with_zero (child, gsi, insert_after, loc);
}
Missing comment.
+static void
+clobber_subtree (struct access *access, gimple_stmt_iterator *gsi,
+ bool insert_after, location_t loc)
+
+{
+ struct access *child;
+
+ if (access-grp_to_be_replaced)
+{
+ tree rep = get_access_replacement (access);
+ tree clobber = build_constructor (access-type, NULL);
+ TREE_THIS_VOLATILE (clobber) = 1;
+ gimple stmt = gimple_build_assign (rep, clobber);
+
+ if (insert_after)
+ gsi_insert_after (gsi, stmt, GSI_NEW_STMT);
+ else
+ gsi_insert_before (gsi, stmt, GSI_SAME_STMT);
+ update_stmt (stmt);
+ gimple_set_location (stmt, loc);
+}
+ else if (access-grp_to_be_debug_replaced)
+{
Why would we care to create clobbers for debug stmts?! Are those
even valid?
Otherwise this looks good I think.
Richard.
+ tree rep = get_access_replacement (access);
+ tree clobber = build_constructor (access-type, NULL);
+ TREE_THIS_VOLATILE (clobber) = 1;
+ gimple ds = gimple_build_debug_bind (rep, clobber, gsi_stmt (*gsi));
+
+ if (insert_after)
+ gsi_insert_after (gsi, ds, GSI_NEW_STMT);
+ else
+ gsi_insert_before (gsi, ds, GSI_SAME_STMT);
+}
+
+ for (child = access-first_child; child; child = child-next_sibling)
+clobber_subtree (child, gsi, insert_after, loc);
+}
+
/* Search for an access representative for the given expression EXPR and
return it or NULL if it cannot be found. */
static struct access *
get_access_for_expr (tree expr)
{
HOST_WIDE_INT offset, size, max_size;
tree base;
/* FIXME: This should not be necessary but Ada produces V_C_Es with a
type of
@@ -3039,43 +3077,41 @@ enum assignment_mod_result { SRA_AM_NONE
SRA_AM_REMOVED }; /* stmt eliminated */
/* Modify assignments with a CONSTRUCTOR on their RHS. STMT contains a
pointer
to the assignment and GSI is the statement iterator pointing at it.
Returns
the same values as sra_modify_assign. */
static enum assignment_mod_result
sra_modify_constructor_assign (gimple *stmt, gimple_stmt_iterator *gsi)
{
tree lhs = gimple_assign_lhs (*stmt);
- struct access *acc;
- location_t loc;
-
- acc = get_access_for_expr (lhs);
+ struct access *acc = get_access_for_expr (lhs);
if (!acc)
return SRA_AM_NONE;
+ location_t loc = gimple_location (*stmt);
if (gimple_clobber_p (*stmt))
{
- /* Remove clobbers of fully scalarized variables, otherwise
-do nothing. */
+ /* Clobber the replacement variable. */
+ clobber_subtree (acc, gsi, !acc-grp_covered, loc);
+ /* Remove clobbers of fully scalarized variables, they are dead. */
if (acc-grp_covered)
{
unlink_stmt_vdef (*stmt);
gsi_remove (gsi, true);
release_defs (*stmt);
return SRA_AM_REMOVED;
}
else
- return SRA_AM_NONE;
+ return SRA_AM_MODIFIED;
}
- loc = gimple_location (*stmt);
if (vec_safe_length (CONSTRUCTOR_ELTS (gimple_assign_rhs1 (*stmt))) 0)
{
/* I have never seen this code path trigger but if it can happen the
following should handle it gracefully. */
if (access_has_children_p (acc))
Re: SRA: don't drop clobbers
On Thu, Jul 10, 2014 at 04:54:53PM +0200, Richard Biener wrote:
+ else if (access-grp_to_be_debug_replaced)
+{
Why would we care to create clobbers for debug stmts?! Are those
even valid?
It is not valid. Though, the fields supposedly live nowhere after the
clobber, so perhaps one could use
GIMPLE_DEBUG_BIND_NOVALUE instead of the clobber in the
gimple_build_debug_bind call, and drop the previous two lines.
Not sure if it is desirable though, it might cause the debug info to say
something is unavailable even if it still lives in some register or memory
for a few extra instructions.
Alex, what do you think?
+ tree rep = get_access_replacement (access);
+ tree clobber = build_constructor (access-type, NULL);
+ TREE_THIS_VOLATILE (clobber) = 1;
+ gimple ds = gimple_build_debug_bind (rep, clobber, gsi_stmt (*gsi));
+
+ if (insert_after)
+ gsi_insert_after (gsi, ds, GSI_NEW_STMT);
+ else
+ gsi_insert_before (gsi, ds, GSI_SAME_STMT);
+}
+
+ for (child = access-first_child; child; child = child-next_sibling)
+clobber_subtree (child, gsi, insert_after, loc);
+}
+
Jakub
Re: SRA: don't drop clobbers
On Mon, Jun 30, 2014 at 1:38 AM, Marc Glisse marc.gli...@inria.fr wrote:
Hello,
with this patch on top of
https://gcc.gnu.org/ml/gcc-patches/2014-06/msg02315.html
we finally warn for the testcase of PR 60517.
The new function is copied from init_subtree_with_zero right above. I guess
it might be possible to merge them into a single function, if desired. I
don't understand the debug stuff, but hopefully by keeping the functions
similar enough it shouldn't be too broken.
When we see a clobber during scalarization, instead of dropping it, we add a
clobber to the new variable, which the previous patch turns into an SSA_NAME
with a default def. Then either we reach uninit and warn, or the variable
appears in a PHI and CCP optimizes.
What's the point of a clobber of sth that was scalarized away? So ...
can you please explain in more detail?
Bootstrap+testsuite (all,obj-c++,ada,go) on x86_64-linux-gnu.
The new function needs a comment.
Richard.
2014-07-01 Marc Glisse marc.gli...@inria.fr
PR c++/60517
PR tree-optimization/60770
gcc/
* tree-sra.c (clobber_subtree): New function.
(sra_modify_constructor_assign): Call it.
gcc/testsuite/
* g++.dg/tree-ssa/pr60517.C: New file.
--
Marc Glisse
Index: gcc/tree-sra.c
===
--- gcc/tree-sra.c (revision 212126)
+++ gcc/tree-sra.c (working copy)
@@ -2727,20 +2727,58 @@ init_subtree_with_zero (struct access *a
if (insert_after)
gsi_insert_after (gsi, ds, GSI_NEW_STMT);
else
gsi_insert_before (gsi, ds, GSI_SAME_STMT);
}
for (child = access-first_child; child; child = child-next_sibling)
init_subtree_with_zero (child, gsi, insert_after, loc);
}
+static void
+clobber_subtree (struct access *access, gimple_stmt_iterator *gsi,
+ bool insert_after, location_t loc)
+
+{
+ struct access *child;
+
+ if (access-grp_to_be_replaced)
+{
+ tree rep = get_access_replacement (access);
+ tree clobber = build_constructor (access-type, NULL);
+ TREE_THIS_VOLATILE (clobber) = 1;
+ gimple stmt = gimple_build_assign (rep, clobber);
+
+ if (insert_after)
+ gsi_insert_after (gsi, stmt, GSI_NEW_STMT);
+ else
+ gsi_insert_before (gsi, stmt, GSI_SAME_STMT);
+ update_stmt (stmt);
+ gimple_set_location (stmt, loc);
+}
+ else if (access-grp_to_be_debug_replaced)
+{
+ tree rep = get_access_replacement (access);
+ tree clobber = build_constructor (access-type, NULL);
+ TREE_THIS_VOLATILE (clobber) = 1;
+ gimple ds = gimple_build_debug_bind (rep, clobber, gsi_stmt (*gsi));
+
+ if (insert_after)
+ gsi_insert_after (gsi, ds, GSI_NEW_STMT);
+ else
+ gsi_insert_before (gsi, ds, GSI_SAME_STMT);
+}
+
+ for (child = access-first_child; child; child = child-next_sibling)
+clobber_subtree (child, gsi, insert_after, loc);
+}
+
/* Search for an access representative for the given expression EXPR and
return it or NULL if it cannot be found. */
static struct access *
get_access_for_expr (tree expr)
{
HOST_WIDE_INT offset, size, max_size;
tree base;
/* FIXME: This should not be necessary but Ada produces V_C_Es with a
type of
@@ -3039,43 +3077,41 @@ enum assignment_mod_result { SRA_AM_NONE
SRA_AM_REMOVED }; /* stmt eliminated */
/* Modify assignments with a CONSTRUCTOR on their RHS. STMT contains a
pointer
to the assignment and GSI is the statement iterator pointing at it.
Returns
the same values as sra_modify_assign. */
static enum assignment_mod_result
sra_modify_constructor_assign (gimple *stmt, gimple_stmt_iterator *gsi)
{
tree lhs = gimple_assign_lhs (*stmt);
- struct access *acc;
- location_t loc;
-
- acc = get_access_for_expr (lhs);
+ struct access *acc = get_access_for_expr (lhs);
if (!acc)
return SRA_AM_NONE;
+ location_t loc = gimple_location (*stmt);
if (gimple_clobber_p (*stmt))
{
- /* Remove clobbers of fully scalarized variables, otherwise
-do nothing. */
+ /* Clobber the replacement variable. */
+ clobber_subtree (acc, gsi, !acc-grp_covered, loc);
+ /* Remove clobbers of fully scalarized variables, they are dead. */
if (acc-grp_covered)
{
unlink_stmt_vdef (*stmt);
gsi_remove (gsi, true);
release_defs (*stmt);
return SRA_AM_REMOVED;
}
else
- return SRA_AM_NONE;
+ return SRA_AM_MODIFIED;
}
- loc = gimple_location (*stmt);
if (vec_safe_length (CONSTRUCTOR_ELTS (gimple_assign_rhs1 (*stmt))) 0)
{
/* I have never seen this code path trigger but if it can happen the
following should handle it gracefully. */
if (access_has_children_p (acc))
Re: SRA: don't drop clobbers
On Mon, 7 Jul 2014, Richard Biener wrote: On Mon, Jun 30, 2014 at 1:38 AM, Marc Glisse marc.gli...@inria.fr wrote: Hello, with this patch on top of https://gcc.gnu.org/ml/gcc-patches/2014-06/msg02315.html we finally warn for the testcase of PR 60517. The new function is copied from init_subtree_with_zero right above. I guess it might be possible to merge them into a single function, if desired. I don't understand the debug stuff, but hopefully by keeping the functions similar enough it shouldn't be too broken. When we see a clobber during scalarization, instead of dropping it, we add a clobber to the new variable, which the previous patch turns into an SSA_NAME with a default def. Then either we reach uninit and warn, or the variable appears in a PHI and CCP optimizes. What's the point of a clobber of sth that was scalarized away? So ... can you please explain in more detail? The main idea of these patches is that when we read from a place that was clobbered, instead of dropping the clobber and reading what was there before, we can use a variable with a default definition to mark that the content is undefined. This enables both warnings and optimizations. The previous patch makes update_ssa handle replacing clobbers with default definitions when a variable doesn't have its address taken anymore. When SRA scalarizes, it creates a new variable and relies on update_ssa to finish the job. So I am inserting a clobber on the new variable so that update_ssa knows to use a default definition. The new function needs a comment. Indeed, I'll add one once the conversation on the first patch converges. Thanks, -- Marc Glisse
Re: SRA: don't drop clobbers
On 07/07/14 02:56, Richard Biener wrote: On Mon, Jun 30, 2014 at 1:38 AM, Marc Glisse marc.gli...@inria.fr wrote: Hello, with this patch on top of https://gcc.gnu.org/ml/gcc-patches/2014-06/msg02315.html we finally warn for the testcase of PR 60517. The new function is copied from init_subtree_with_zero right above. I guess it might be possible to merge them into a single function, if desired. I don't understand the debug stuff, but hopefully by keeping the functions similar enough it shouldn't be too broken. When we see a clobber during scalarization, instead of dropping it, we add a clobber to the new variable, which the previous patch turns into an SSA_NAME with a default def. Then either we reach uninit and warn, or the variable appears in a PHI and CCP optimizes. What's the point of a clobber of sth that was scalarized away? So ... can you please explain in more detail? Marc is trying to exploit the undefinedness of the object that exists after the CLOBBER. When we scalaraize, we are losing the undefinedness quality of the object. It's very similar to what's already happening in tree-into-ssa, he's just handling it in another place. Jeff
Re: SRA: don't drop clobbers
On July 7, 2014 11:32:10 AM CEST, Marc Glisse marc.gli...@inria.fr wrote: On Mon, 7 Jul 2014, Richard Biener wrote: On Mon, Jun 30, 2014 at 1:38 AM, Marc Glisse marc.gli...@inria.fr wrote: Hello, with this patch on top of https://gcc.gnu.org/ml/gcc-patches/2014-06/msg02315.html we finally warn for the testcase of PR 60517. The new function is copied from init_subtree_with_zero right above. I guess it might be possible to merge them into a single function, if desired. I don't understand the debug stuff, but hopefully by keeping the functions similar enough it shouldn't be too broken. When we see a clobber during scalarization, instead of dropping it, we add a clobber to the new variable, which the previous patch turns into an SSA_NAME with a default def. Then either we reach uninit and warn, or the variable appears in a PHI and CCP optimizes. What's the point of a clobber of sth that was scalarized away? So ... can you please explain in more detail? The main idea of these patches is that when we read from a place that was clobbered, instead of dropping the clobber and reading what was there before, we can use a variable with a default definition to mark that the content is undefined. This enables both warnings and optimizations. The previous patch makes update_ssa handle replacing clobbers with default definitions when a variable doesn't have its address taken anymore. When SRA scalarizes, it creates a new variable and relies on update_ssa to finish the job. So I am inserting a clobber on the new variable so that update_ssa knows to use a default definition. OK. But can't SRA simply replace the reads with undef SSA names when the use is dominated by a clobber? (I've long wanted to teach it some flow sensitivity by doing the replacements within a domwalk) Richard. The new function needs a comment. Indeed, I'll add one once the conversation on the first patch converges. Thanks,
Re: SRA: don't drop clobbers
On Mon, 7 Jul 2014, Richard Biener wrote: The main idea of these patches is that when we read from a place that was clobbered, instead of dropping the clobber and reading what was there before, we can use a variable with a default definition to mark that the content is undefined. This enables both warnings and optimizations. The previous patch makes update_ssa handle replacing clobbers with default definitions when a variable doesn't have its address taken anymore. When SRA scalarizes, it creates a new variable and relies on update_ssa to finish the job. So I am inserting a clobber on the new variable so that update_ssa knows to use a default definition. OK. But can't SRA simply replace the reads with undef SSA names when the use is dominated by a clobber? I guess it could, but it seems safer and much simpler to avoid duplicating the logic. SRA creates a new variable and replaces uses of the old one by uses of the new one. update_address_taken then handles replacing the new variable by a bunch of SSA_NAMEs, tracking which use corresponds to which def, so it seems natural to let it handle the clobber as well. Also, conceptually, saying that when the full variable is clobbered the subvariable is clobbered as well is a rather obvious operation. Otherwise, I could create an extra variable tmp, with the same type as the new variable, and replace the clobber by an assignment of the default definition of tmp to the new variable. update_address_taken will then see a regular assignment and handle it as usual (create a new SSA_NAME for it). So instead of creating a clobber that is immediatly replaced with a NOP, this creates an extra SSA_NAME that only disappears in the next propagation pass. Handling the reads myself directly in SRA and replacing them with undef SSA_NAMEs doesn't seem to fit well with how SRA works. It would certainly be possible, but would require quite a bit of new code. Also, the domination relation between the clobber and the reads is not always obvious and may require some PHIs in between. -- Marc Glisse
SRA: don't drop clobbers
Hello,
with this patch on top of
https://gcc.gnu.org/ml/gcc-patches/2014-06/msg02315.html
we finally warn for the testcase of PR 60517.
The new function is copied from init_subtree_with_zero right above. I
guess it might be possible to merge them into a single function, if
desired. I don't understand the debug stuff, but hopefully by keeping the
functions similar enough it shouldn't be too broken.
When we see a clobber during scalarization, instead of dropping it, we add
a clobber to the new variable, which the previous patch turns into an
SSA_NAME with a default def. Then either we reach uninit and warn, or the
variable appears in a PHI and CCP optimizes.
Bootstrap+testsuite (all,obj-c++,ada,go) on x86_64-linux-gnu.
2014-07-01 Marc Glisse marc.gli...@inria.fr
PR c++/60517
PR tree-optimization/60770
gcc/
* tree-sra.c (clobber_subtree): New function.
(sra_modify_constructor_assign): Call it.
gcc/testsuite/
* g++.dg/tree-ssa/pr60517.C: New file.
--
Marc GlisseIndex: gcc/tree-sra.c
===
--- gcc/tree-sra.c (revision 212126)
+++ gcc/tree-sra.c (working copy)
@@ -2727,20 +2727,58 @@ init_subtree_with_zero (struct access *a
if (insert_after)
gsi_insert_after (gsi, ds, GSI_NEW_STMT);
else
gsi_insert_before (gsi, ds, GSI_SAME_STMT);
}
for (child = access-first_child; child; child = child-next_sibling)
init_subtree_with_zero (child, gsi, insert_after, loc);
}
+static void
+clobber_subtree (struct access *access, gimple_stmt_iterator *gsi,
+ bool insert_after, location_t loc)
+
+{
+ struct access *child;
+
+ if (access-grp_to_be_replaced)
+{
+ tree rep = get_access_replacement (access);
+ tree clobber = build_constructor (access-type, NULL);
+ TREE_THIS_VOLATILE (clobber) = 1;
+ gimple stmt = gimple_build_assign (rep, clobber);
+
+ if (insert_after)
+ gsi_insert_after (gsi, stmt, GSI_NEW_STMT);
+ else
+ gsi_insert_before (gsi, stmt, GSI_SAME_STMT);
+ update_stmt (stmt);
+ gimple_set_location (stmt, loc);
+}
+ else if (access-grp_to_be_debug_replaced)
+{
+ tree rep = get_access_replacement (access);
+ tree clobber = build_constructor (access-type, NULL);
+ TREE_THIS_VOLATILE (clobber) = 1;
+ gimple ds = gimple_build_debug_bind (rep, clobber, gsi_stmt (*gsi));
+
+ if (insert_after)
+ gsi_insert_after (gsi, ds, GSI_NEW_STMT);
+ else
+ gsi_insert_before (gsi, ds, GSI_SAME_STMT);
+}
+
+ for (child = access-first_child; child; child = child-next_sibling)
+clobber_subtree (child, gsi, insert_after, loc);
+}
+
/* Search for an access representative for the given expression EXPR and
return it or NULL if it cannot be found. */
static struct access *
get_access_for_expr (tree expr)
{
HOST_WIDE_INT offset, size, max_size;
tree base;
/* FIXME: This should not be necessary but Ada produces V_C_Es with a type of
@@ -3039,43 +3077,41 @@ enum assignment_mod_result { SRA_AM_NONE
SRA_AM_REMOVED }; /* stmt eliminated */
/* Modify assignments with a CONSTRUCTOR on their RHS. STMT contains a pointer
to the assignment and GSI is the statement iterator pointing at it. Returns
the same values as sra_modify_assign. */
static enum assignment_mod_result
sra_modify_constructor_assign (gimple *stmt, gimple_stmt_iterator *gsi)
{
tree lhs = gimple_assign_lhs (*stmt);
- struct access *acc;
- location_t loc;
-
- acc = get_access_for_expr (lhs);
+ struct access *acc = get_access_for_expr (lhs);
if (!acc)
return SRA_AM_NONE;
+ location_t loc = gimple_location (*stmt);
if (gimple_clobber_p (*stmt))
{
- /* Remove clobbers of fully scalarized variables, otherwise
-do nothing. */
+ /* Clobber the replacement variable. */
+ clobber_subtree (acc, gsi, !acc-grp_covered, loc);
+ /* Remove clobbers of fully scalarized variables, they are dead. */
if (acc-grp_covered)
{
unlink_stmt_vdef (*stmt);
gsi_remove (gsi, true);
release_defs (*stmt);
return SRA_AM_REMOVED;
}
else
- return SRA_AM_NONE;
+ return SRA_AM_MODIFIED;
}
- loc = gimple_location (*stmt);
if (vec_safe_length (CONSTRUCTOR_ELTS (gimple_assign_rhs1 (*stmt))) 0)
{
/* I have never seen this code path trigger but if it can happen the
following should handle it gracefully. */
if (access_has_children_p (acc))
generate_subtree_copies (acc-first_child, lhs, acc-offset, 0, 0, gsi,
true, true, loc);
return SRA_AM_MODIFIED;
}
Index: gcc/testsuite/g++.dg/tree-ssa/pr60517.C
===
--- gcc/testsuite/g++.dg/tree-ssa/pr60517.C (revision 0)
+++
