* update address taken: don't drop clobbers
@ 2014-06-28 22:33 Marc Glisse
2014-06-29 23:38 ` SRA: " Marc Glisse
` (2 more replies)
0 siblings, 3 replies; 48+ messages in thread
From: Marc Glisse @ 2014-06-28 22:33 UTC (permalink / raw)
To: gcc-patches
[-- Attachment #1: Type: TEXT/PLAIN, Size: 1560 bytes --]
Hello,
we currently drop clobbers on variables whose address is not taken
anymore. However, rewrite_stmt has code to replace them with an SSA_NAME
with a default definition (an uninitialized variable), and I believe
rewrite_update_stmt should do the same. This allows us to warn sometimes
(see testcase), but during the debugging I also noticed several places
where it allowed CCP to simplify further PHIs, so this is also an
optimization.
In an earlier version of the patch, I was using
get_or_create_ssa_default_def (cfun, sym);
(I was reusing the same variable). This passed bootstrap+testsuite on all
languages except for ada. Indeed, the compiler wanted to coalesce several
SSA_NAMEs, including those new ones, in out-of-ssa, but couldn't. There
are abnormal PHIs involved. Maybe it shouldn't have insisted on coalescing
an undefined ssa_name, maybe something should have prevented us from
reaching such a situation, but creating a new variable was the simplest
workaround.
Some things could be done to improve the error message in uninit:
- getting the location of the variable,
- differenciating uninitialized from clobbered,
but that can come later.
Bootstrap+testsuite (all,obj-c++,ada,go) on x86_64-unknown-linux-gnu.
2014-06-30 Marc Glisse <marc.glisse@inria.fr>
PR tree-optimization/60770
gcc/
* tree-ssa.c (execute_update_addresses_taken): Don't drop clobbers.
* tree-into-ssa.c (maybe_register_def): Replace clobbers with a
default definition.
gcc/testsuite/
* gcc.dg/tree-ssa/pr60770-1.c: New file.
--
Marc Glisse
[-- Attachment #2: Type: TEXT/PLAIN, Size: 3366 bytes --]
Index: gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c
===================================================================
--- gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c (revision 0)
+++ gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c (working copy)
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-options "-O -Wall" } */
+
+int f(int n){
+ int*p;
+ {
+ int yyy=n;
+ p=&yyy;
+ }
+ return *p; /* { dg-warning "yyy" } */
+}
Index: gcc/tree-into-ssa.c
===================================================================
--- gcc/tree-into-ssa.c (revision 212109)
+++ gcc/tree-into-ssa.c (working copy)
@@ -1831,26 +1831,38 @@ maybe_register_def (def_operand_p def_p,
{
tree def = DEF_FROM_PTR (def_p);
tree sym = DECL_P (def) ? def : SSA_NAME_VAR (def);
/* If DEF is a naked symbol that needs renaming, create a new
name for it. */
if (marked_for_renaming (sym))
{
if (DECL_P (def))
{
- tree tracked_var;
-
- def = make_ssa_name (def, stmt);
+ if (gimple_clobber_p (stmt) && is_gimple_reg (sym))
+ {
+ /* Replace clobber stmts with a default def. Create a new
+ variable so we don't later think we must coalesce, which would
+ fail with some ada abnormal PHIs. Still, we try to keep a
+ similar name so error messages make sense. */
+ unlink_stmt_vdef (stmt);
+ gsi_replace (&gsi, gimple_build_nop (), true);
+ tree id = DECL_NAME (sym);
+ const char* name = id ? IDENTIFIER_POINTER (id) : 0;
+ tree newvar = create_tmp_var (TREE_TYPE (sym), name);
+ def = get_or_create_ssa_default_def (cfun, newvar);
+ }
+ else
+ def = make_ssa_name (def, stmt);
SET_DEF (def_p, def);
- tracked_var = target_for_debug_bind (sym);
+ tree tracked_var = target_for_debug_bind (sym);
if (tracked_var)
{
gimple note = gimple_build_debug_bind (tracked_var, def, stmt);
/* If stmt ends the bb, insert the debug stmt on the single
non-EH edge from the stmt. */
if (gsi_one_before_end_p (gsi) && stmt_ends_bb_p (stmt))
{
basic_block bb = gsi_bb (gsi);
edge_iterator ei;
edge e, ef = NULL;
Index: gcc/tree-ssa.c
===================================================================
--- gcc/tree-ssa.c (revision 212109)
+++ gcc/tree-ssa.c (working copy)
@@ -1607,32 +1607,20 @@ execute_update_addresses_taken (void)
rhs = gimple_assign_rhs1 (stmt);
if (gimple_assign_lhs (stmt) != lhs
&& !useless_type_conversion_p (TREE_TYPE (lhs),
TREE_TYPE (rhs)))
rhs = fold_build1 (VIEW_CONVERT_EXPR,
TREE_TYPE (lhs), rhs);
if (gimple_assign_lhs (stmt) != lhs)
gimple_assign_set_lhs (stmt, lhs);
- /* For var ={v} {CLOBBER}; where var lost
- TREE_ADDRESSABLE just remove the stmt. */
- if (DECL_P (lhs)
- && TREE_CLOBBER_P (rhs)
- && bitmap_bit_p (suitable_for_renaming, DECL_UID (lhs)))
- {
- unlink_stmt_vdef (stmt);
- gsi_remove (&gsi, true);
- release_defs (stmt);
- continue;
- }
-
if (gimple_assign_rhs1 (stmt) != rhs)
{
gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
gimple_assign_set_rhs_from_tree (&gsi, rhs);
}
}
else if (gimple_code (stmt) == GIMPLE_CALL)
{
unsigned i;
^ permalink raw reply [flat|nested] 48+ messages in thread
* SRA: don't drop clobbers
2014-06-28 22:33 update address taken: don't drop clobbers Marc Glisse
@ 2014-06-29 23:38 ` Marc Glisse
2014-07-07 8:56 ` Richard Biener
2014-07-10 14:55 ` Richard Biener
2014-06-30 19:31 ` update address taken: " Jeff Law
2014-07-10 15:10 ` Richard Biener
2 siblings, 2 replies; 48+ messages in thread
From: Marc Glisse @ 2014-06-29 23:38 UTC (permalink / raw)
To: gcc-patches
[-- Attachment #1: Type: TEXT/PLAIN, Size: 1029 bytes --]
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.glisse@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
[-- Attachment #2: Type: TEXT/PLAIN, Size: 4257 bytes --]
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))
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)
+++ 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 "" } */
+}
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: update address taken: don't drop clobbers
2014-06-28 22:33 update address taken: don't drop clobbers Marc Glisse
2014-06-29 23:38 ` SRA: " Marc Glisse
@ 2014-06-30 19:31 ` Jeff Law
2014-07-06 14:24 ` Marc Glisse
2014-07-10 15:10 ` Richard Biener
2 siblings, 1 reply; 48+ messages in thread
From: Jeff Law @ 2014-06-30 19:31 UTC (permalink / raw)
To: Marc Glisse, gcc-patches
On 06/28/14 16:33, Marc Glisse wrote:
> In an earlier version of the patch, I was using
> get_or_create_ssa_default_def (cfun, sym);
> (I was reusing the same variable). This passed bootstrap+testsuite on
> all languages except for ada. Indeed, the compiler wanted to coalesce
> several SSA_NAMEs, including those new ones, in out-of-ssa, but
> couldn't.
And that's what you need to delve deeper into. Why did it refuse to
coalesce?
As long as the lifetimes do not overlap, then coalescing should have worked.
jeff
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: update address taken: don't drop clobbers
2014-06-30 19:31 ` update address taken: " Jeff Law
@ 2014-07-06 14:24 ` Marc Glisse
2014-07-06 14:54 ` pinskia
` (2 more replies)
0 siblings, 3 replies; 48+ messages in thread
From: Marc Glisse @ 2014-07-06 14:24 UTC (permalink / raw)
To: Jeff Law; +Cc: gcc-patches
[-- Attachment #1: Type: TEXT/PLAIN, Size: 2493 bytes --]
On Mon, 30 Jun 2014, Jeff Law wrote:
> On 06/28/14 16:33, Marc Glisse wrote:
>> In an earlier version of the patch, I was using
>> get_or_create_ssa_default_def (cfun, sym);
>> (I was reusing the same variable). This passed bootstrap+testsuite on
>> all languages except for ada. Indeed, the compiler wanted to coalesce
>> several SSA_NAMEs, including those new ones, in out-of-ssa, but
>> couldn't.
> And that's what you need to delve deeper into. Why did it refuse to
> coalesce?
>
> As long as the lifetimes do not overlap, then coalescing should have worked.
What is the lifetime of an SSA_NAME with a default definition? The way we
handle it now, we start from the uses and go back to all blocks that can
reach one of the uses, since there is no defining statement where we could
stop (intuitively we should stop where the clobber was, if not earlier).
This huge lifetime makes it very likely for such an SSA_NAME to conflict
with others. And if an abnormal phi is involved, and thus we must
coalesce, there is a problem.
The patch attached (it should probably use ssa_undefined_value_p with a
new extra argument to say whether we care about partially-undefined) makes
the lifetime of undefined local variables empty and lets the original
patch work with:
def = get_or_create_ssa_default_def (cfun, sym);
instead of creating a new variable.
However, I am not convinced reusing the same variable is necessarily the
best thing. For warnings, we can create a new variable with the same name
(with .1 added by gcc at the end) and copy the location info (I am not
doing that yet), so little is lost. A new variable expresses more clearly
that the value it holds is random crap. If I reuse the same variable, the
SRA patch doesn't warn because SRA explicitly sets TREE_NO_WARNING (this
can probably be changed, but that's starting to be a lot of changes).
Creating a new variable is also more general. When reading *ssa_name after
*ssa_name={v}{CLOBBER}; or after free(ssa_name); we have no variable to
reuse so we will need to create a new undefined variable, and if a
variable is global or a PARM_DECL, its default definition is not an
undefined value (though it will probably happen in a different pass, so it
doesn't have to behave the same).
(Side note: we don't seem to have any code to notice that:
a=phi<undef,b>
b=phi<undef,a>
means both phis can be replaced with undefined variables.)
Do you have any advice on the right direction?
--
Marc Glisse
[-- Attachment #2: Type: TEXT/PLAIN, Size: 1978 bytes --]
Index: gcc/tree-ssa-live.c
===================================================================
--- gcc/tree-ssa-live.c (revision 212306)
+++ gcc/tree-ssa-live.c (working copy)
@@ -1086,20 +1086,28 @@ set_var_live_on_entry (tree ssa_name, tr
if (stmt)
{
def_bb = gimple_bb (stmt);
/* Mark defs in liveout bitmap temporarily. */
if (def_bb)
bitmap_set_bit (&live->liveout[def_bb->index], p);
}
else
def_bb = ENTRY_BLOCK_PTR_FOR_FN (cfun);
+ /* An undefined local variable does not need to be very alive. */
+ if (SSA_NAME_IS_DEFAULT_DEF (ssa_name))
+ {
+ tree var = SSA_NAME_VAR (ssa_name);
+ if (var && TREE_CODE (var) == VAR_DECL && !is_global_var (var))
+ return;
+ }
+
/* Visit each use of SSA_NAME and if it isn't in the same block as the def,
add it to the list of live on entry blocks. */
FOR_EACH_IMM_USE_FAST (use, imm_iter, ssa_name)
{
gimple use_stmt = USE_STMT (use);
basic_block add_block = NULL;
if (gimple_code (use_stmt) == GIMPLE_PHI)
{
/* Uses in PHI's are considered to be live at exit of the SRC block
@@ -1422,20 +1430,27 @@ verify_live_on_entry (tree_live_info_p l
fprintf (stderr, "\n");
}
else
fprintf (stderr, " and there is no default def.\n");
}
}
}
else
if (d == var)
{
+ /* An undefined local variable does not need to be very
+ alive. */
+ tree real_var = SSA_NAME_VAR (var);
+ if (real_var && TREE_CODE (real_var) == VAR_DECL
+ && !is_global_var (real_var))
+ continue;
+
/* The only way this var shouldn't be marked live on entry is
if it occurs in a PHI argument of the block. */
size_t z;
bool ok = false;
gimple_stmt_iterator gsi;
for (gsi = gsi_start_phis (e->dest);
!gsi_end_p (gsi) && !ok;
gsi_next (&gsi))
{
gimple phi = gsi_stmt (gsi);
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: update address taken: don't drop clobbers
2014-07-06 14:24 ` Marc Glisse
@ 2014-07-06 14:54 ` pinskia
2014-07-06 15:01 ` Marc Glisse
2014-07-07 10:21 ` Richard Biener
2014-07-07 17:20 ` Jeff Law
2 siblings, 1 reply; 48+ messages in thread
From: pinskia @ 2014-07-06 14:54 UTC (permalink / raw)
To: Marc Glisse; +Cc: Jeff Law, gcc-patches
> On Jul 6, 2014, at 7:23 AM, Marc Glisse <marc.glisse@inria.fr> wrote:
>
>> On Mon, 30 Jun 2014, Jeff Law wrote:
>>
>>> On 06/28/14 16:33, Marc Glisse wrote:
>>> In an earlier version of the patch, I was using
>>> get_or_create_ssa_default_def (cfun, sym);
>>> (I was reusing the same variable). This passed bootstrap+testsuite on
>>> all languages except for ada. Indeed, the compiler wanted to coalesce
>>> several SSA_NAMEs, including those new ones, in out-of-ssa, but
>>> couldn't.
>> And that's what you need to delve deeper into. Why did it refuse to coalesce?
>>
>> As long as the lifetimes do not overlap, then coalescing should have worked.
>
> What is the lifetime of an SSA_NAME with a default definition? The way we handle it now, we start from the uses and go back to all blocks that can reach one of the uses, since there is no defining statement where we could stop (intuitively we should stop where the clobber was, if not earlier). This huge lifetime makes it very likely for such an SSA_NAME to conflict with others. And if an abnormal phi is involved, and thus we must coalesce, there is a problem.
>
> The patch attached (it should probably use ssa_undefined_value_p with a new extra argument to say whether we care about partially-undefined) makes the lifetime of undefined local variables empty and lets the original patch work with:
> def = get_or_create_ssa_default_def (cfun, sym);
> instead of creating a new variable.
>
> However, I am not convinced reusing the same variable is necessarily the best thing. For warnings, we can create a new variable with the same name (with .1 added by gcc at the end) and copy the location info (I am not doing that yet), so little is lost. A new variable expresses more clearly that the value it holds is random crap. If I reuse the same variable, the SRA patch doesn't warn because SRA explicitly sets TREE_NO_WARNING (this can probably be changed, but that's starting to be a lot of changes). Creating a new variable is also more general. When reading *ssa_name after *ssa_name={v}{CLOBBER}; or after free(ssa_name); we have no variable to reuse so we will need to create a new undefined variable, and if a variable is global or a PARM_DECL, its default definition is not an undefined value (though it will probably happen in a different pass, so it doesn't have to behave the same).
>
> (Side note: we don't seem to have any code to notice that:
> a=phi<undef,b>
> b=phi<undef,a>
> means both phis can be replaced with undefined variables.)
>
> Do you have any advice on the right direction?
The below patch won't work for parameters.
Thanks,
Andrew
>
> --
> Marc Glisse
> Index: gcc/tree-ssa-live.c
> ===================================================================
> --- gcc/tree-ssa-live.c (revision 212306)
> +++ gcc/tree-ssa-live.c (working copy)
> @@ -1086,20 +1086,28 @@ set_var_live_on_entry (tree ssa_name, tr
> if (stmt)
> {
> def_bb = gimple_bb (stmt);
> /* Mark defs in liveout bitmap temporarily. */
> if (def_bb)
> bitmap_set_bit (&live->liveout[def_bb->index], p);
> }
> else
> def_bb = ENTRY_BLOCK_PTR_FOR_FN (cfun);
>
> + /* An undefined local variable does not need to be very alive. */
> + if (SSA_NAME_IS_DEFAULT_DEF (ssa_name))
> + {
> + tree var = SSA_NAME_VAR (ssa_name);
> + if (var && TREE_CODE (var) == VAR_DECL && !is_global_var (var))
> + return;
> + }
> +
> /* Visit each use of SSA_NAME and if it isn't in the same block as the def,
> add it to the list of live on entry blocks. */
> FOR_EACH_IMM_USE_FAST (use, imm_iter, ssa_name)
> {
> gimple use_stmt = USE_STMT (use);
> basic_block add_block = NULL;
>
> if (gimple_code (use_stmt) == GIMPLE_PHI)
> {
> /* Uses in PHI's are considered to be live at exit of the SRC block
> @@ -1422,20 +1430,27 @@ verify_live_on_entry (tree_live_info_p l
> fprintf (stderr, "\n");
> }
> else
> fprintf (stderr, " and there is no default def.\n");
> }
> }
> }
> else
> if (d == var)
> {
> + /* An undefined local variable does not need to be very
> + alive. */
> + tree real_var = SSA_NAME_VAR (var);
> + if (real_var && TREE_CODE (real_var) == VAR_DECL
> + && !is_global_var (real_var))
> + continue;
> +
> /* The only way this var shouldn't be marked live on entry is
> if it occurs in a PHI argument of the block. */
> size_t z;
> bool ok = false;
> gimple_stmt_iterator gsi;
> for (gsi = gsi_start_phis (e->dest);
> !gsi_end_p (gsi) && !ok;
> gsi_next (&gsi))
> {
> gimple phi = gsi_stmt (gsi);
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: update address taken: don't drop clobbers
2014-07-06 14:54 ` pinskia
@ 2014-07-06 15:01 ` Marc Glisse
0 siblings, 0 replies; 48+ messages in thread
From: Marc Glisse @ 2014-07-06 15:01 UTC (permalink / raw)
To: pinskia; +Cc: Jeff Law, gcc-patches
On Sun, 6 Jul 2014, pinskia@gmail.com wrote:
> The below patch won't work for parameters.
Er, that's why I am testing: TREE_CODE (var) == VAR_DECL
(and the patch passed the testsuite with all languages)
Could you be more specific about what won't work?
--
Marc Glisse
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: SRA: don't drop clobbers
2014-06-29 23:38 ` SRA: " Marc Glisse
@ 2014-07-07 8:56 ` Richard Biener
2014-07-07 9:32 ` Marc Glisse
2014-07-07 16:59 ` Jeff Law
2014-07-10 14:55 ` Richard Biener
1 sibling, 2 replies; 48+ messages in thread
From: Richard Biener @ 2014-07-07 8:56 UTC (permalink / raw)
To: Marc Glisse; +Cc: GCC Patches
On Mon, Jun 30, 2014 at 1:38 AM, Marc Glisse <marc.glisse@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.glisse@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))
> 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)
> +++ 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 "" } */
> +}
>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: SRA: don't drop clobbers
2014-07-07 8:56 ` Richard Biener
@ 2014-07-07 9:32 ` Marc Glisse
2014-07-07 18:32 ` Richard Biener
2014-07-07 16:59 ` Jeff Law
1 sibling, 1 reply; 48+ messages in thread
From: Marc Glisse @ 2014-07-07 9:32 UTC (permalink / raw)
To: Richard Biener; +Cc: GCC Patches
On Mon, 7 Jul 2014, Richard Biener wrote:
> On Mon, Jun 30, 2014 at 1:38 AM, Marc Glisse <marc.glisse@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
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: update address taken: don't drop clobbers
2014-07-06 14:24 ` Marc Glisse
2014-07-06 14:54 ` pinskia
@ 2014-07-07 10:21 ` Richard Biener
2014-07-07 17:20 ` Jeff Law
2 siblings, 0 replies; 48+ messages in thread
From: Richard Biener @ 2014-07-07 10:21 UTC (permalink / raw)
To: Marc Glisse; +Cc: Jeff Law, GCC Patches
On Sun, Jul 6, 2014 at 4:23 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
> On Mon, 30 Jun 2014, Jeff Law wrote:
>
>> On 06/28/14 16:33, Marc Glisse wrote:
>>>
>>> In an earlier version of the patch, I was using
>>> get_or_create_ssa_default_def (cfun, sym);
>>> (I was reusing the same variable). This passed bootstrap+testsuite on
>>> all languages except for ada. Indeed, the compiler wanted to coalesce
>>> several SSA_NAMEs, including those new ones, in out-of-ssa, but
>>> couldn't.
>>
>> And that's what you need to delve deeper into. Why did it refuse to
>> coalesce?
>>
>> As long as the lifetimes do not overlap, then coalescing should have
>> worked.
>
>
> What is the lifetime of an SSA_NAME with a default definition? The way we
> handle it now, we start from the uses and go back to all blocks that can
> reach one of the uses, since there is no defining statement where we could
> stop (intuitively we should stop where the clobber was, if not earlier).
> This huge lifetime makes it very likely for such an SSA_NAME to conflict
> with others. And if an abnormal phi is involved, and thus we must coalesce,
> there is a problem.
>
> The patch attached (it should probably use ssa_undefined_value_p with a new
> extra argument to say whether we care about partially-undefined) makes the
> lifetime of undefined local variables empty and lets the original patch work
> with:
> def = get_or_create_ssa_default_def (cfun, sym);
> instead of creating a new variable.
>
> However, I am not convinced reusing the same variable is necessarily the
> best thing. For warnings, we can create a new variable with the same name
> (with .1 added by gcc at the end) and copy the location info (I am not doing
> that yet), so little is lost. A new variable expresses more clearly that the
> value it holds is random crap. If I reuse the same variable, the SRA patch
> doesn't warn because SRA explicitly sets TREE_NO_WARNING (this can probably
> be changed, but that's starting to be a lot of changes). Creating a new
> variable is also more general. When reading *ssa_name after
> *ssa_name={v}{CLOBBER}; or after free(ssa_name); we have no variable to
> reuse so we will need to create a new undefined variable, and if a variable
> is global or a PARM_DECL, its default definition is not an undefined value
> (though it will probably happen in a different pass, so it doesn't have to
> behave the same).
>
> (Side note: we don't seem to have any code to notice that:
> a=phi<undef,b>
> b=phi<undef,a>
> means both phis can be replaced with undefined variables.)
Propagators will never replace sth with unefined but they will exploit
the undefinedness. Constant propagation is even a bit more aggressive.
> Do you have any advice on the right direction?
The patch below would work (btw, please use ssa_undefined_value_p),
but I wonder if it's the conflict code that should get adjustments
rather than the lifeness computation (lifeness of undefined values
shouldn't be needed at all).
Also depends on how we expand an undefined value of course.
Richard.
> --
> Marc Glisse
> Index: gcc/tree-ssa-live.c
> ===================================================================
> --- gcc/tree-ssa-live.c (revision 212306)
> +++ gcc/tree-ssa-live.c (working copy)
> @@ -1086,20 +1086,28 @@ set_var_live_on_entry (tree ssa_name, tr
> if (stmt)
> {
> def_bb = gimple_bb (stmt);
> /* Mark defs in liveout bitmap temporarily. */
> if (def_bb)
> bitmap_set_bit (&live->liveout[def_bb->index], p);
> }
> else
> def_bb = ENTRY_BLOCK_PTR_FOR_FN (cfun);
>
> + /* An undefined local variable does not need to be very alive. */
> + if (SSA_NAME_IS_DEFAULT_DEF (ssa_name))
> + {
> + tree var = SSA_NAME_VAR (ssa_name);
> + if (var && TREE_CODE (var) == VAR_DECL && !is_global_var (var))
> + return;
> + }
> +
> /* Visit each use of SSA_NAME and if it isn't in the same block as the
> def,
> add it to the list of live on entry blocks. */
> FOR_EACH_IMM_USE_FAST (use, imm_iter, ssa_name)
> {
> gimple use_stmt = USE_STMT (use);
> basic_block add_block = NULL;
>
> if (gimple_code (use_stmt) == GIMPLE_PHI)
> {
> /* Uses in PHI's are considered to be live at exit of the SRC
> block
> @@ -1422,20 +1430,27 @@ verify_live_on_entry (tree_live_info_p l
> fprintf (stderr, "\n");
> }
> else
> fprintf (stderr, " and there is no default def.\n");
> }
> }
> }
> else
> if (d == var)
> {
> + /* An undefined local variable does not need to be very
> + alive. */
> + tree real_var = SSA_NAME_VAR (var);
> + if (real_var && TREE_CODE (real_var) == VAR_DECL
> + && !is_global_var (real_var))
> + continue;
> +
> /* The only way this var shouldn't be marked live on entry
> is
> if it occurs in a PHI argument of the block. */
> size_t z;
> bool ok = false;
> gimple_stmt_iterator gsi;
> for (gsi = gsi_start_phis (e->dest);
> !gsi_end_p (gsi) && !ok;
> gsi_next (&gsi))
> {
> gimple phi = gsi_stmt (gsi);
>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: SRA: don't drop clobbers
2014-07-07 8:56 ` Richard Biener
2014-07-07 9:32 ` Marc Glisse
@ 2014-07-07 16:59 ` Jeff Law
1 sibling, 0 replies; 48+ messages in thread
From: Jeff Law @ 2014-07-07 16:59 UTC (permalink / raw)
To: Richard Biener, Marc Glisse; +Cc: GCC Patches
On 07/07/14 02:56, Richard Biener wrote:
> On Mon, Jun 30, 2014 at 1:38 AM, Marc Glisse <marc.glisse@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
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: update address taken: don't drop clobbers
2014-07-06 14:24 ` Marc Glisse
2014-07-06 14:54 ` pinskia
2014-07-07 10:21 ` Richard Biener
@ 2014-07-07 17:20 ` Jeff Law
2014-07-08 13:31 ` Marc Glisse
2 siblings, 1 reply; 48+ messages in thread
From: Jeff Law @ 2014-07-07 17:20 UTC (permalink / raw)
To: Marc Glisse; +Cc: gcc-patches
On 07/06/14 08:23, Marc Glisse wrote:
> What is the lifetime of an SSA_NAME with a default definition? The way
> we handle it now, we start from the uses and go back to all blocks that
> can reach one of the uses, since there is no defining statement where we
> could stop
Right, that's exactly what I would expect given the typical definition
of liveness.
(intuitively we should stop where the clobber was, if not
> earlier). This huge lifetime makes it very likely for such an SSA_NAME
> to conflict with others. And if an abnormal phi is involved, and thus we
> must coalesce, there is a problem.
So this suggests another approach. Could we just assume that it's
always safe to coalesce with a default definition?
>
> The patch attached (it should probably use ssa_undefined_value_p with a
> new extra argument to say whether we care about partially-undefined)
> makes the lifetime of undefined local variables empty and lets the
> original patch work with:
> def = get_or_create_ssa_default_def (cfun, sym);
> instead of creating a new variable.
Not a terrible suggestion either and accomplishes the same thing as the
mental model I had of special casing this stuff in the coalescing code.
I guess this is the first thing we probably should sort out. Do we want
to handle this is the conflict, coalescing or lifetime analysis.
We've certainly had similar hacks in the code which built the conflict
matrix for the register allocator in the past (old local-alloc/global
variant, pre IRA). By not recoding those conflicts, coalescing just
magically works.
By handling it when we build the lifetimes, they'll magically coalesce
because there's no conflicts as well. It may help other code which
utilizes lifetimes as well. But a part of me doesn't like it because it
is different than the traditionally formulated life analysis.
The question I havent thought much about is would the coalescing code do
the right thing if we have one of these undefined SSA_NAMEs that
coalesce into two different partitions.
ie, let's say we have two PHI nodes in different blocks. Each PHI has
one or more source/dest operations that are marked as
SSA_NAME_OCCURS_IN_ABNORMAL_PHI. Furthermore, they all reference a
single RHS undefined SSA_NAME.
When we coalesce the undefined SSA_NAME with all the others in the first
PHI, doesn't that mean that we then have to coalesce all the SSA_NAMES
in both PHIs together (because that undefined SSA_NAME appears on the
RHS on the 2nd PHI too).
Does this then argue that each use of an undefined SSA_NAME should be a
unique SSA_NAME?
Ugh, I wonder if I just opened a can of worms here...
>
> However, I am not convinced reusing the same variable is necessarily the
> best thing. For warnings, we can create a new variable with the same
> name (with .1 added by gcc at the end) and copy the location info (I am
> not doing that yet), so little is lost. A new variable expresses more
> clearly that the value it holds is random crap. If I reuse the same
> variable, the SRA patch doesn't warn because SRA explicitly sets
> TREE_NO_WARNING (this can probably be changed, but that's starting to be
> a lot of changes). Creating a new variable is also more general. When
> reading *ssa_name after *ssa_name={v}{CLOBBER}; or after free(ssa_name);
> we have no variable to reuse so we will need to create a new undefined
> variable, and if a variable is global or a PARM_DECL, its default
> definition is not an undefined value (though it will probably happen in
> a different pass, so it doesn't have to behave the same).
My concern about using another variable was primarily that it was being
used to hide a deeper issue.
>
> (Side note: we don't seem to have any code to notice that:
> a=phi<undef,b>
> b=phi<undef,a>
> means both phis can be replaced with undefined variables.)
Various passes will consider the undef value to be whatever value is
convenient for optimization. So for the first, we'd consider the undef
value to be the same as "b" because that allows the PHI to be propagated
away.
Obviously when there are many different values in the RHS, it's a lot
less likely that we'll be able to propagate away the PHI.
Jeff
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: SRA: don't drop clobbers
2014-07-07 9:32 ` Marc Glisse
@ 2014-07-07 18:32 ` Richard Biener
2014-07-07 20:15 ` Marc Glisse
0 siblings, 1 reply; 48+ messages in thread
From: Richard Biener @ 2014-07-07 18:32 UTC (permalink / raw)
To: Marc Glisse; +Cc: GCC Patches
On July 7, 2014 11:32:10 AM CEST, Marc Glisse <marc.glisse@inria.fr> wrote:
>On Mon, 7 Jul 2014, Richard Biener wrote:
>
>> On Mon, Jun 30, 2014 at 1:38 AM, Marc Glisse <marc.glisse@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,
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: SRA: don't drop clobbers
2014-07-07 18:32 ` Richard Biener
@ 2014-07-07 20:15 ` Marc Glisse
0 siblings, 0 replies; 48+ messages in thread
From: Marc Glisse @ 2014-07-07 20:15 UTC (permalink / raw)
To: Richard Biener; +Cc: GCC Patches
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
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: update address taken: don't drop clobbers
2014-07-07 17:20 ` Jeff Law
@ 2014-07-08 13:31 ` Marc Glisse
0 siblings, 0 replies; 48+ messages in thread
From: Marc Glisse @ 2014-07-08 13:31 UTC (permalink / raw)
To: Richard Biener, Jeff Law; +Cc: GCC Patches
On Mon, 7 Jul 2014, Jeff Law wrote:
> On 07/06/14 08:23, Marc Glisse wrote:
>> What is the lifetime of an SSA_NAME with a default definition? The way
>> we handle it now, we start from the uses and go back to all blocks that
>> can reach one of the uses, since there is no defining statement where we
>> could stop
> Right, that's exactly what I would expect given the typical definition of
> liveness.
>
>
>> (intuitively we should stop where the clobber was, if not
>> earlier). This huge lifetime makes it very likely for such an SSA_NAME
>> to conflict with others. And if an abnormal phi is involved, and thus we
>> must coalesce, there is a problem.
> So this suggests another approach. Could we just assume that it's always
> safe to coalesce with a default definition?
>
>
>> The patch attached (it should probably use ssa_undefined_value_p with a
>> new extra argument to say whether we care about partially-undefined)
>> makes the lifetime of undefined local variables empty and lets the
>> original patch work with:
>> def = get_or_create_ssa_default_def (cfun, sym);
>> instead of creating a new variable.
> Not a terrible suggestion either and accomplishes the same thing as the
> mental model I had of special casing this stuff in the coalescing code.
>
> I guess this is the first thing we probably should sort out. Do we want to
> handle this is the conflict, coalescing or lifetime analysis.
>
> We've certainly had similar hacks in the code which built the conflict matrix
> for the register allocator in the past (old local-alloc/global variant, pre
> IRA). By not recoding those conflicts, coalescing just magically works.
>
> By handling it when we build the lifetimes, they'll magically coalesce
> because there's no conflicts as well. It may help other code which utilizes
> lifetimes as well. But a part of me doesn't like it because it is different
> than the traditionally formulated life analysis.
Those variables don't have a birth time so their lifetime is quite
ill-defined in any case. I first tried to force coalesce for those
variables, but it is not very convenient. We do want to consider them
for coalescing, we can't just ignore them. But when we coalesce them
with a first variable, we want to mark it somehow so we don't later
coalesce them with a second variable that conflicts with the first. The
way we do that normally is by computing the union of the lifetimes, but
if we ignore the lifetime for this variable... It isn't just a matter of
adding || ssa_undefined_value_p (var) in the conflict test. Giving them
an empty lifetime to begin with seems to do the right thing. If we don't
modify the lifetime computation, we would have in coalesce to pass only
the "defined" variables to calculate_live_ranges and re-add the
undefined variables afterwards with an empty range.
> The question I havent thought much about is would the coalescing code
> do the right thing if we have one of these undefined SSA_NAMEs that
> coalesce into two different partitions.
>
> ie, let's say we have two PHI nodes in different blocks. Each PHI has
> one or more source/dest operations that are marked as
> SSA_NAME_OCCURS_IN_ABNORMAL_PHI. Furthermore, they all reference a
> single RHS undefined SSA_NAME.
>
> When we coalesce the undefined SSA_NAME with all the others in the
> first PHI, doesn't that mean that we then have to coalesce all the
> SSA_NAMES in both PHIs together (because that undefined SSA_NAME
> appears on the RHS on the 2nd PHI too).
>
> Does this then argue that each use of an undefined SSA_NAME should be
> a unique SSA_NAME?
I did wonder about the same thing. But without the patch, there were only
2 files in the whole gcc+testsuite that had an issue (in the ada
front-end), and with the patch they were fine, so if the situation you
describe is possible, at least it is very uncommon.
Creating a new variable instead of reusing the old one may be less
likely to cause this kind of trouble. At least at creation time we know
there is no conflict since it is coming from a variable by an operation
that is the reverse of coalescing.
> Ugh, I wonder if I just opened a can of worms here...
>
>>
>> However, I am not convinced reusing the same variable is necessarily the
>> best thing. For warnings, we can create a new variable with the same
>> name (with .1 added by gcc at the end) and copy the location info (I am
>> not doing that yet), so little is lost. A new variable expresses more
>> clearly that the value it holds is random crap. If I reuse the same
>> variable, the SRA patch doesn't warn because SRA explicitly sets
>> TREE_NO_WARNING (this can probably be changed, but that's starting to be
>> a lot of changes). Creating a new variable is also more general. When
>> reading *ssa_name after *ssa_name={v}{CLOBBER}; or after free(ssa_name);
>> we have no variable to reuse so we will need to create a new undefined
>> variable, and if a variable is global or a PARM_DECL, its default
>> definition is not an undefined value (though it will probably happen in
>> a different pass, so it doesn't have to behave the same).
> My concern about using another variable was primarily that it was being used
> to hide a deeper issue.
>
>
>>
>> (Side note: we don't seem to have any code to notice that:
>> a=phi<undef,b>
>> b=phi<undef,a>
>> means both phis can be replaced with undefined variables.)
> Various passes will consider the undef value to be whatever value is
> convenient for optimization. So for the first, we'd consider the undef value
> to be the same as "b" because that allows the PHI to be propagated away.
Considering that a is the same as b and b is the same as a is not as good
as knowing that a and b are both undefined. I don't know if CCP is clever
enough to still optimize if b is used in a further PHI, but it would have
an easier time (and coalesce and the uninit warning as well) if a=undef;
b=undef; were forwarded into their uses instead. Not a priority though.
> Obviously when there are many different values in the RHS, it's a lot less
> likely that we'll be able to propagate away the PHI.
On Mon, 7 Jul 2014, Richard Biener wrote:
> On Sun, Jul 6, 2014 at 4:23 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
>> On Mon, 30 Jun 2014, Jeff Law wrote:
>>
>>> On 06/28/14 16:33, Marc Glisse wrote:
>>>>
>>>> In an earlier version of the patch, I was using
>>>> get_or_create_ssa_default_def (cfun, sym);
>>>> (I was reusing the same variable). This passed bootstrap+testsuite on
>>>> all languages except for ada. Indeed, the compiler wanted to coalesce
>>>> several SSA_NAMEs, including those new ones, in out-of-ssa, but
>>>> couldn't.
>>>
>>> And that's what you need to delve deeper into. Why did it refuse to
>>> coalesce?
>>>
>>> As long as the lifetimes do not overlap, then coalescing should have
>>> worked.
>>
>>
>> What is the lifetime of an SSA_NAME with a default definition? The way we
>> handle it now, we start from the uses and go back to all blocks that can
>> reach one of the uses, since there is no defining statement where we could
>> stop (intuitively we should stop where the clobber was, if not earlier).
>> This huge lifetime makes it very likely for such an SSA_NAME to conflict
>> with others. And if an abnormal phi is involved, and thus we must coalesce,
>> there is a problem.
>>
>> The patch attached (it should probably use ssa_undefined_value_p with a new
>> extra argument to say whether we care about partially-undefined) makes the
>> lifetime of undefined local variables empty and lets the original patch work
>> with:
>> def = get_or_create_ssa_default_def (cfun, sym);
>> instead of creating a new variable.
>>
>> However, I am not convinced reusing the same variable is necessarily the
>> best thing. For warnings, we can create a new variable with the same name
>> (with .1 added by gcc at the end) and copy the location info (I am not doing
>> that yet), so little is lost. A new variable expresses more clearly that the
>> value it holds is random crap. If I reuse the same variable, the SRA patch
>> doesn't warn because SRA explicitly sets TREE_NO_WARNING (this can probably
>> be changed, but that's starting to be a lot of changes). Creating a new
>> variable is also more general. When reading *ssa_name after
>> *ssa_name={v}{CLOBBER}; or after free(ssa_name); we have no variable to
>> reuse so we will need to create a new undefined variable, and if a variable
>> is global or a PARM_DECL, its default definition is not an undefined value
>> (though it will probably happen in a different pass, so it doesn't have to
>> behave the same).
>>
>> (Side note: we don't seem to have any code to notice that:
>> a=phi<undef,b>
>> b=phi<undef,a>
>> means both phis can be replaced with undefined variables.)
>
> Propagators will never replace sth with unefined but they will exploit
> the undefinedness. Constant propagation is even a bit more aggressive.
>
>> Do you have any advice on the right direction?
>
> The patch below would work (btw, please use ssa_undefined_value_p),
> but I wonder if it's the conflict code that should get adjustments
> rather than the lifeness computation (lifeness of undefined values
> shouldn't be needed at all).
>
> Also depends on how we expand an undefined value of course.
I didn't really look at the expansion part.
--
Marc Glisse
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: SRA: don't drop clobbers
2014-06-29 23:38 ` SRA: " Marc Glisse
2014-07-07 8:56 ` Richard Biener
@ 2014-07-10 14:55 ` Richard Biener
2014-07-10 15:01 ` Jakub Jelinek
1 sibling, 1 reply; 48+ messages in thread
From: Richard Biener @ 2014-07-10 14:55 UTC (permalink / raw)
To: Marc Glisse; +Cc: GCC Patches
On Mon, Jun 30, 2014 at 1:38 AM, Marc Glisse <marc.glisse@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.glisse@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))
> 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)
> +++ 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 "" } */
> +}
>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: SRA: don't drop clobbers
2014-07-10 14:55 ` Richard Biener
@ 2014-07-10 15:01 ` Jakub Jelinek
0 siblings, 0 replies; 48+ messages in thread
From: Jakub Jelinek @ 2014-07-10 15:01 UTC (permalink / raw)
To: Richard Biener, Alexandre Oliva; +Cc: Marc Glisse, GCC Patches
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
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: update address taken: don't drop clobbers
2014-06-28 22:33 update address taken: don't drop clobbers Marc Glisse
2014-06-29 23:38 ` SRA: " Marc Glisse
2014-06-30 19:31 ` update address taken: " Jeff Law
@ 2014-07-10 15:10 ` Richard Biener
2014-07-10 15:49 ` Michael Matz
` (4 more replies)
2 siblings, 5 replies; 48+ messages in thread
From: Richard Biener @ 2014-07-10 15:10 UTC (permalink / raw)
To: Marc Glisse; +Cc: GCC Patches
On Sun, Jun 29, 2014 at 12:33 AM, Marc Glisse <marc.glisse@inria.fr> wrote:
> Hello,
>
> we currently drop clobbers on variables whose address is not taken anymore.
> However, rewrite_stmt has code to replace them with an SSA_NAME with a
> default definition (an uninitialized variable), and I believe
> rewrite_update_stmt should do the same. This allows us to warn sometimes
> (see testcase), but during the debugging I also noticed several places where
> it allowed CCP to simplify further PHIs, so this is also an optimization.
>
> In an earlier version of the patch, I was using
> get_or_create_ssa_default_def (cfun, sym);
> (I was reusing the same variable). This passed bootstrap+testsuite on all
> languages except for ada. Indeed, the compiler wanted to coalesce several
> SSA_NAMEs, including those new ones, in out-of-ssa, but couldn't. There are
> abnormal PHIs involved. Maybe it shouldn't have insisted on coalescing an
> undefined ssa_name, maybe something should have prevented us from reaching
> such a situation, but creating a new variable was the simplest workaround.
Hmm. We indeed notice "late" that the new names are used in abnormal
PHIs. Note that usually rewriting a symbol into SSA form does not
create overlapping life-ranges - but of course you are possibly introducing
those by the new use of the default definitions.
Apart from the out-of-SSA patch you proposed elsewhere a possibility
is to simply never mark undefined SSA names as
SSA_NAME_OCCURS_IN_ABNORMAL_PHI ... (or not mark those
as must-coalesce).
> Some things could be done to improve the error message in uninit:
> - getting the location of the variable,
> - differenciating uninitialized from clobbered,
> but that can come later.
>
> Bootstrap+testsuite (all,obj-c++,ada,go) on x86_64-unknown-linux-gnu.
>
> 2014-06-30 Marc Glisse <marc.glisse@inria.fr>
>
> PR tree-optimization/60770
> gcc/
> * tree-ssa.c (execute_update_addresses_taken): Don't drop clobbers.
> * tree-into-ssa.c (maybe_register_def): Replace clobbers with a
> default definition.
> gcc/testsuite/
> * gcc.dg/tree-ssa/pr60770-1.c: New file.
>
> --
> Marc Glisse
> Index: gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c (revision 0)
> +++ gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c (working copy)
> @@ -0,0 +1,11 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O -Wall" } */
> +
> +int f(int n){
> + int*p;
> + {
> + int yyy=n;
> + p=&yyy;
> + }
> + return *p; /* { dg-warning "yyy" } */
> +}
> Index: gcc/tree-into-ssa.c
> ===================================================================
> --- gcc/tree-into-ssa.c (revision 212109)
> +++ gcc/tree-into-ssa.c (working copy)
> @@ -1831,26 +1831,38 @@ maybe_register_def (def_operand_p def_p,
> {
> tree def = DEF_FROM_PTR (def_p);
> tree sym = DECL_P (def) ? def : SSA_NAME_VAR (def);
>
> /* If DEF is a naked symbol that needs renaming, create a new
> name for it. */
> if (marked_for_renaming (sym))
> {
> if (DECL_P (def))
> {
> - tree tracked_var;
> -
> - def = make_ssa_name (def, stmt);
> + if (gimple_clobber_p (stmt) && is_gimple_reg (sym))
sym should always be a gimple reg here (it's marked for renaming).
> + {
> + /* Replace clobber stmts with a default def. Create a new
> + variable so we don't later think we must coalesce, which
> would
> + fail with some ada abnormal PHIs. Still, we try to keep a
> + similar name so error messages make sense. */
> + unlink_stmt_vdef (stmt);
I think that's redundant with gsi_replace (note that using gsi_replace
looks dangerous here as it calls update_stmt during SSA rewrite...
that might open a can of worms).
> + gsi_replace (&gsi, gimple_build_nop (), true);
> + tree id = DECL_NAME (sym);
> + const char* name = id ? IDENTIFIER_POINTER (id) : 0;
> + tree newvar = create_tmp_var (TREE_TYPE (sym), name);
> + def = get_or_create_ssa_default_def (cfun, newvar);
So - can't you simply do
gimple_assign_set_rhs_from_tree (&gsi,
get_or_create_dda_default_def (cfun, sym));
? Thus replace x = CLOBBER; with x_3 = x_2(D);
> + }
> + else
and of course still rewrite the DEF then. IMHO the copy-propagation
you do is premature optimization.
> + def = make_ssa_name (def, stmt);
> SET_DEF (def_p, def);
>
> - tracked_var = target_for_debug_bind (sym);
> + tree tracked_var = target_for_debug_bind (sym);
> if (tracked_var)
> {
> gimple note = gimple_build_debug_bind (tracked_var, def,
> stmt);
> /* If stmt ends the bb, insert the debug stmt on the single
> non-EH edge from the stmt. */
> if (gsi_one_before_end_p (gsi) && stmt_ends_bb_p (stmt))
> {
> basic_block bb = gsi_bb (gsi);
> edge_iterator ei;
> edge e, ef = NULL;
> Index: gcc/tree-ssa.c
> ===================================================================
> --- gcc/tree-ssa.c (revision 212109)
> +++ gcc/tree-ssa.c (working copy)
> @@ -1607,32 +1607,20 @@ execute_update_addresses_taken (void)
> rhs = gimple_assign_rhs1 (stmt);
> if (gimple_assign_lhs (stmt) != lhs
> && !useless_type_conversion_p (TREE_TYPE (lhs),
> TREE_TYPE (rhs)))
> rhs = fold_build1 (VIEW_CONVERT_EXPR,
> TREE_TYPE (lhs), rhs);
>
> if (gimple_assign_lhs (stmt) != lhs)
> gimple_assign_set_lhs (stmt, lhs);
>
> - /* For var ={v} {CLOBBER}; where var lost
> - TREE_ADDRESSABLE just remove the stmt. */
> - if (DECL_P (lhs)
> - && TREE_CLOBBER_P (rhs)
> - && bitmap_bit_p (suitable_for_renaming, DECL_UID (lhs)))
> - {
> - unlink_stmt_vdef (stmt);
> - gsi_remove (&gsi, true);
> - release_defs (stmt);
> - continue;
> - }
> -
> if (gimple_assign_rhs1 (stmt) != rhs)
> {
> gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
> gimple_assign_set_rhs_from_tree (&gsi, rhs);
> }
> }
>
> else if (gimple_code (stmt) == GIMPLE_CALL)
> {
> unsigned i;
>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: update address taken: don't drop clobbers
2014-07-10 15:10 ` Richard Biener
@ 2014-07-10 15:49 ` Michael Matz
2014-07-10 18:23 ` Jeff Law
2014-07-12 6:15 ` Marc Glisse
` (3 subsequent siblings)
4 siblings, 1 reply; 48+ messages in thread
From: Michael Matz @ 2014-07-10 15:49 UTC (permalink / raw)
To: Richard Biener; +Cc: Marc Glisse, GCC Patches
Hi,
On Thu, 10 Jul 2014, Richard Biener wrote:
> Apart from the out-of-SSA patch you proposed elsewhere a possibility
> is to simply never mark undefined SSA names as
> SSA_NAME_OCCURS_IN_ABNORMAL_PHI ... (or not mark those
> as must-coalesce).
The insight to note is, that undefined SSA names should really be
coalesced with something (otherwise you lost an optimization opportunity),
but it doesn't matter with _what_ each use of the undefined name is
coalesced, you can even identify different uses of them with different SSA
names (e.g. the LHS of each using stmt). Requires some change in the
order things are done in out-of-ssa.
Ciao,
Michael.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: update address taken: don't drop clobbers
2014-07-10 15:49 ` Michael Matz
@ 2014-07-10 18:23 ` Jeff Law
2014-07-11 8:10 ` Richard Biener
2014-07-11 12:06 ` Michael Matz
0 siblings, 2 replies; 48+ messages in thread
From: Jeff Law @ 2014-07-10 18:23 UTC (permalink / raw)
To: Michael Matz, Richard Biener; +Cc: Marc Glisse, GCC Patches
On 07/10/14 09:48, Michael Matz wrote:
> Hi,
>
> On Thu, 10 Jul 2014, Richard Biener wrote:
>
>> Apart from the out-of-SSA patch you proposed elsewhere a possibility
>> is to simply never mark undefined SSA names as
>> SSA_NAME_OCCURS_IN_ABNORMAL_PHI ... (or not mark those
>> as must-coalesce).
>
> The insight to note is, that undefined SSA names should really be
> coalesced with something (otherwise you lost an optimization opportunity),
> but it doesn't matter with _what_ each use of the undefined name is
> coalesced, you can even identify different uses of them with different SSA
> names (e.g. the LHS of each using stmt). Requires some change in the
> order things are done in out-of-ssa.
The last part is what I hinted might be problematical. If some
undefined SSA_NAME appears on the RHS of two PHIs and we want to
coalesce that undefined SSA_NAME with the LHS of each of those PHIs,
then the LHS of those two PHIs must coalesce as well. At least that's
my recollection of how all that stuff worked.
It was that realization that made me wonder if we should have a unique
SSA_NAME at each undefined use point.
jeff
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: update address taken: don't drop clobbers
2014-07-10 18:23 ` Jeff Law
@ 2014-07-11 8:10 ` Richard Biener
2014-07-11 8:14 ` Richard Biener
2014-07-11 12:06 ` Michael Matz
1 sibling, 1 reply; 48+ messages in thread
From: Richard Biener @ 2014-07-11 8:10 UTC (permalink / raw)
To: Jeff Law; +Cc: Michael Matz, Marc Glisse, GCC Patches
On Thu, Jul 10, 2014 at 8:22 PM, Jeff Law <law@redhat.com> wrote:
> On 07/10/14 09:48, Michael Matz wrote:
>>
>> Hi,
>>
>> On Thu, 10 Jul 2014, Richard Biener wrote:
>>
>>> Apart from the out-of-SSA patch you proposed elsewhere a possibility
>>> is to simply never mark undefined SSA names as
>>> SSA_NAME_OCCURS_IN_ABNORMAL_PHI ... (or not mark those
>>> as must-coalesce).
>>
>>
>> The insight to note is, that undefined SSA names should really be
>> coalesced with something (otherwise you lost an optimization opportunity),
>> but it doesn't matter with _what_ each use of the undefined name is
>> coalesced, you can even identify different uses of them with different SSA
>> names (e.g. the LHS of each using stmt). Requires some change in the
>> order things are done in out-of-ssa.
>
> The last part is what I hinted might be problematical. If some undefined
> SSA_NAME appears on the RHS of two PHIs and we want to coalesce that
> undefined SSA_NAME with the LHS of each of those PHIs, then the LHS of those
> two PHIs must coalesce as well. At least that's my recollection of how all
> that stuff worked.
Yes, coalescing doesn't do "live-range splitting" to avoid coalescing the
two PHI results. But they have to be coalesced anyway.
I still think simply never recording conflicts for undefined SSA names
is a proper "hack" to avoid this issue.
> It was that realization that made me wonder if we should have a unique
> SSA_NAME at each undefined use point.
That would be unnecessarily expensive.
Richard.
> jeff
>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: update address taken: don't drop clobbers
2014-07-11 8:10 ` Richard Biener
@ 2014-07-11 8:14 ` Richard Biener
0 siblings, 0 replies; 48+ messages in thread
From: Richard Biener @ 2014-07-11 8:14 UTC (permalink / raw)
To: Jeff Law; +Cc: Michael Matz, Marc Glisse, GCC Patches
On Fri, Jul 11, 2014 at 10:10 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Thu, Jul 10, 2014 at 8:22 PM, Jeff Law <law@redhat.com> wrote:
>> On 07/10/14 09:48, Michael Matz wrote:
>>>
>>> Hi,
>>>
>>> On Thu, 10 Jul 2014, Richard Biener wrote:
>>>
>>>> Apart from the out-of-SSA patch you proposed elsewhere a possibility
>>>> is to simply never mark undefined SSA names as
>>>> SSA_NAME_OCCURS_IN_ABNORMAL_PHI ... (or not mark those
>>>> as must-coalesce).
>>>
>>>
>>> The insight to note is, that undefined SSA names should really be
>>> coalesced with something (otherwise you lost an optimization opportunity),
>>> but it doesn't matter with _what_ each use of the undefined name is
>>> coalesced, you can even identify different uses of them with different SSA
>>> names (e.g. the LHS of each using stmt). Requires some change in the
>>> order things are done in out-of-ssa.
>>
>> The last part is what I hinted might be problematical. If some undefined
>> SSA_NAME appears on the RHS of two PHIs and we want to coalesce that
>> undefined SSA_NAME with the LHS of each of those PHIs, then the LHS of those
>> two PHIs must coalesce as well. At least that's my recollection of how all
>> that stuff worked.
>
> Yes, coalescing doesn't do "live-range splitting" to avoid coalescing the
> two PHI results. But they have to be coalesced anyway.
>
> I still think simply never recording conflicts for undefined SSA names
> is a proper "hack" to avoid this issue.
>
>> It was that realization that made me wonder if we should have a unique
>> SSA_NAME at each undefined use point.
>
> That would be unnecessarily expensive.
Btw, the bug must be already kind-of preexisting due to
bool
may_propagate_copy (tree dest, tree orig)
{
...
/* If ORIG flows in from an abnormal edge, it cannot be propagated. */
if (TREE_CODE (orig) == SSA_NAME
&& SSA_NAME_OCCURS_IN_ABNORMAL_PHI (orig)
/* If it is the default definition and an automatic variable then
we can though and it is important that we do to avoid
uninitialized regular copies. */
&& !(SSA_NAME_IS_DEFAULT_DEF (orig)
&& (SSA_NAME_VAR (orig) == NULL_TREE
|| TREE_CODE (SSA_NAME_VAR (orig)) == VAR_DECL)))
return false;
but it never replaces an abnormal SSA names with its default definition
due to the next check:
/* If DEST is an SSA_NAME that flows from an abnormal edge, then it
cannot be replaced. */
if (TREE_CODE (dest) == SSA_NAME
&& SSA_NAME_OCCURS_IN_ABNORMAL_PHI (dest))
return false;
still it might create overlapping life-ranges for abnormals.
Richard.
> Richard.
>
>> jeff
>>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: update address taken: don't drop clobbers
2014-07-10 18:23 ` Jeff Law
2014-07-11 8:10 ` Richard Biener
@ 2014-07-11 12:06 ` Michael Matz
2014-07-11 17:16 ` Jeff Law
1 sibling, 1 reply; 48+ messages in thread
From: Michael Matz @ 2014-07-11 12:06 UTC (permalink / raw)
To: Jeff Law; +Cc: Richard Biener, Marc Glisse, GCC Patches
Hi,
On Thu, 10 Jul 2014, Jeff Law wrote:
> > The insight to note is, that undefined SSA names should really be
> > coalesced with something (otherwise you lost an optimization opportunity),
> > but it doesn't matter with _what_ each use of the undefined name is
> > coalesced, you can even identify different uses of them with different SSA
> > names (e.g. the LHS of each using stmt). Requires some change in the
> > order things are done in out-of-ssa.
>
> The last part is what I hinted might be problematical. If some
> undefined SSA_NAME appears on the RHS of two PHIs and we want to
> coalesce that undefined SSA_NAME with the LHS of each of those PHIs,
> then the LHS of those two PHIs must coalesce as well. At least that's
> my recollection of how all that stuff worked.
Only with the usual definition of coalescing (being transitive). For
undefined SSA names the definition can be mended.
> It was that realization that made me wonder if we should have a unique
> SSA_NAME at each undefined use point.
It's easier to implicitely regard every individual use of an undefined SSA
name as a unique name in coalescing I think (instead of having it be a
unique name explicitely). That is, given:
bb1:
x_1 = PHI <a_2, b_3(UND)>
...
bb2:
x_4 = PHI <y_5, b_3(UND)>
...
There is no reason to not regard the two uses of b_3 as separate and
identify the first with x_1 and the second with x_2, _without_ coalescing
x_1 and x_2. But yes, this doesn't fit readily into the normal coalescing
merge-find framework, but rather would have to be something after
coalescing when writing out-of-ssa (whenever an undefined use is rewritten
just take a random other fitting variable).
Ciao,
Michael.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: update address taken: don't drop clobbers
2014-07-11 12:06 ` Michael Matz
@ 2014-07-11 17:16 ` Jeff Law
0 siblings, 0 replies; 48+ messages in thread
From: Jeff Law @ 2014-07-11 17:16 UTC (permalink / raw)
To: Michael Matz; +Cc: Richard Biener, Marc Glisse, GCC Patches
On 07/11/14 06:05, Michael Matz wrote:
> Hi,
>
> On Thu, 10 Jul 2014, Jeff Law wrote:
>
>>> The insight to note is, that undefined SSA names should really be
>>> coalesced with something (otherwise you lost an optimization opportunity),
>>> but it doesn't matter with _what_ each use of the undefined name is
>>> coalesced, you can even identify different uses of them with different SSA
>>> names (e.g. the LHS of each using stmt). Requires some change in the
>>> order things are done in out-of-ssa.
>>
>> The last part is what I hinted might be problematical. If some
>> undefined SSA_NAME appears on the RHS of two PHIs and we want to
>> coalesce that undefined SSA_NAME with the LHS of each of those PHIs,
>> then the LHS of those two PHIs must coalesce as well. At least that's
>> my recollection of how all that stuff worked.
>
> Only with the usual definition of coalescing (being transitive). For
> undefined SSA names the definition can be mended.
My recollection is the transitive nature of coalescing is baked into our
implementation. I'd be happy to be proved wrong :-)
Jeff
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: update address taken: don't drop clobbers
2014-07-10 15:10 ` Richard Biener
2014-07-10 15:49 ` Michael Matz
@ 2014-07-12 6:15 ` Marc Glisse
2014-07-24 13:06 ` Richard Biener
2014-07-27 11:53 ` Marc Glisse
` (2 subsequent siblings)
4 siblings, 1 reply; 48+ messages in thread
From: Marc Glisse @ 2014-07-12 6:15 UTC (permalink / raw)
To: Richard Biener; +Cc: GCC Patches
On Thu, 10 Jul 2014, Richard Biener wrote:
>> --- gcc/tree-into-ssa.c (revision 212109)
>> +++ gcc/tree-into-ssa.c (working copy)
>> @@ -1831,26 +1831,38 @@ maybe_register_def (def_operand_p def_p,
>> {
>> tree def = DEF_FROM_PTR (def_p);
>> tree sym = DECL_P (def) ? def : SSA_NAME_VAR (def);
>>
>> /* If DEF is a naked symbol that needs renaming, create a new
>> name for it. */
>> if (marked_for_renaming (sym))
>> {
>> if (DECL_P (def))
>> {
>> - tree tracked_var;
>> -
>> - def = make_ssa_name (def, stmt);
>> + if (gimple_clobber_p (stmt) && is_gimple_reg (sym))
>
> sym should always be a gimple reg here (it's marked for renaming).
>
>> + {
>> + /* Replace clobber stmts with a default def. Create a new
>> + variable so we don't later think we must coalesce, which
>> would
>> + fail with some ada abnormal PHIs. Still, we try to keep a
>> + similar name so error messages make sense. */
>> + unlink_stmt_vdef (stmt);
>
> I think that's redundant with gsi_replace (note that using gsi_replace
> looks dangerous here as it calls update_stmt during SSA rewrite...
> that might open a can of worms).
IIRC it was failing without unlink_stmt_vdef (maybe that was in a
different version of the patch not using gsi_replace, but I don't think
so). I was hoping that a clobber had little enough effects that
update_stmt was unlikely to break anything. Anyway it doesn't matter if I
use your suggestion below.
>> + gsi_replace (&gsi, gimple_build_nop (), true);
>> + tree id = DECL_NAME (sym);
>> + const char* name = id ? IDENTIFIER_POINTER (id) : 0;
>> + tree newvar = create_tmp_var (TREE_TYPE (sym), name);
>> + def = get_or_create_ssa_default_def (cfun, newvar);
>
> So - can't you simply do
>
> gimple_assign_set_rhs_from_tree (&gsi,
> get_or_create_dda_default_def (cfun, sym));
>
> ? Thus replace x = CLOBBER; with x_3 = x_2(D);
>
>> + }
>> + else
>
> and of course still rewrite the DEF then. IMHO the copy-propagation
> you do is premature optimization.
I'll try that. I was trying to remain as close as possible to what you
wrote in rewrite_stmt:
if (gimple_clobber_p (stmt)
&& is_gimple_reg (var))
{
/* If we rewrite a DECL into SSA form then drop its
clobber stmts and replace uses with a new default def. */
gcc_checking_assert (TREE_CODE (var) == VAR_DECL
&& !gimple_vdef (stmt));
gsi_replace (si, gimple_build_nop (), true);
register_new_def (get_or_create_ssa_default_def (cfun, var), var);
break;
}
I'll be away next week, but I'll re-read all the replies carefully when I
come back.
--
Marc Glisse
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: update address taken: don't drop clobbers
2014-07-12 6:15 ` Marc Glisse
@ 2014-07-24 13:06 ` Richard Biener
0 siblings, 0 replies; 48+ messages in thread
From: Richard Biener @ 2014-07-24 13:06 UTC (permalink / raw)
To: Marc Glisse; +Cc: GCC Patches
On Sat, Jul 12, 2014 at 8:15 AM, Marc Glisse <marc.glisse@inria.fr> wrote:
> On Thu, 10 Jul 2014, Richard Biener wrote:
>
>>> --- gcc/tree-into-ssa.c (revision 212109)
>>> +++ gcc/tree-into-ssa.c (working copy)
>>> @@ -1831,26 +1831,38 @@ maybe_register_def (def_operand_p def_p,
>>> {
>>> tree def = DEF_FROM_PTR (def_p);
>>> tree sym = DECL_P (def) ? def : SSA_NAME_VAR (def);
>>>
>>> /* If DEF is a naked symbol that needs renaming, create a new
>>> name for it. */
>>> if (marked_for_renaming (sym))
>>> {
>>> if (DECL_P (def))
>>> {
>>> - tree tracked_var;
>>> -
>>> - def = make_ssa_name (def, stmt);
>>> + if (gimple_clobber_p (stmt) && is_gimple_reg (sym))
>>
>>
>> sym should always be a gimple reg here (it's marked for renaming).
>>
>>> + {
>>> + /* Replace clobber stmts with a default def. Create a new
>>> + variable so we don't later think we must coalesce, which
>>> would
>>> + fail with some ada abnormal PHIs. Still, we try to keep
>>> a
>>> + similar name so error messages make sense. */
>>> + unlink_stmt_vdef (stmt);
>>
>>
>> I think that's redundant with gsi_replace (note that using gsi_replace
>> looks dangerous here as it calls update_stmt during SSA rewrite...
>> that might open a can of worms).
>
>
> IIRC it was failing without unlink_stmt_vdef (maybe that was in a different
> version of the patch not using gsi_replace, but I don't think so). I was
> hoping that a clobber had little enough effects that update_stmt was
> unlikely to break anything. Anyway it doesn't matter if I use your
> suggestion below.
The important part is that it not ICE (of course) and that it doesn't
trigger useless SSA renaming of .MEM - gsi_replace does
update_stmt which does the unlink_stmt_vdef for you if the .MEM
is no longer necessary.
Richard.
>
>>> + gsi_replace (&gsi, gimple_build_nop (), true);
>>> + tree id = DECL_NAME (sym);
>>> + const char* name = id ? IDENTIFIER_POINTER (id) : 0;
>>> + tree newvar = create_tmp_var (TREE_TYPE (sym), name);
>>> + def = get_or_create_ssa_default_def (cfun, newvar);
>>
>>
>> So - can't you simply do
>>
>> gimple_assign_set_rhs_from_tree (&gsi,
>> get_or_create_dda_default_def (cfun, sym));
>>
>> ? Thus replace x = CLOBBER; with x_3 = x_2(D);
>>
>>> + }
>>> + else
>>
>>
>> and of course still rewrite the DEF then. IMHO the copy-propagation
>> you do is premature optimization.
>
>
> I'll try that. I was trying to remain as close as possible to what you wrote
> in rewrite_stmt:
>
> if (gimple_clobber_p (stmt)
> && is_gimple_reg (var))
> {
> /* If we rewrite a DECL into SSA form then drop its
> clobber stmts and replace uses with a new default def. */
> gcc_checking_assert (TREE_CODE (var) == VAR_DECL
> && !gimple_vdef (stmt));
> gsi_replace (si, gimple_build_nop (), true);
> register_new_def (get_or_create_ssa_default_def (cfun, var),
> var);
> break;
> }
>
>
> I'll be away next week, but I'll re-read all the replies carefully when I
> come back.
>
> --
> Marc Glisse
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: update address taken: don't drop clobbers
2014-07-10 15:10 ` Richard Biener
2014-07-10 15:49 ` Michael Matz
2014-07-12 6:15 ` Marc Glisse
@ 2014-07-27 11:53 ` Marc Glisse
2014-07-27 18:01 ` Marc Glisse
2014-10-18 22:23 ` Marc Glisse
4 siblings, 0 replies; 48+ messages in thread
From: Marc Glisse @ 2014-07-27 11:53 UTC (permalink / raw)
To: Richard Biener; +Cc: GCC Patches
On Thu, 10 Jul 2014, Richard Biener wrote:
>> --- gcc/tree-into-ssa.c (revision 212109)
>> +++ gcc/tree-into-ssa.c (working copy)
>> @@ -1831,26 +1831,38 @@ maybe_register_def (def_operand_p def_p,
>> {
>> tree def = DEF_FROM_PTR (def_p);
>> tree sym = DECL_P (def) ? def : SSA_NAME_VAR (def);
>>
>> /* If DEF is a naked symbol that needs renaming, create a new
>> name for it. */
>> if (marked_for_renaming (sym))
>> {
>> if (DECL_P (def))
>> {
>> - tree tracked_var;
>> -
>> - def = make_ssa_name (def, stmt);
>> + if (gimple_clobber_p (stmt) && is_gimple_reg (sym))
>
> sym should always be a gimple reg here (it's marked for renaming).
Not quite. If I remove the is_gimple_reg check, I get:
/data/repos/gcc/sra/libgcc/libgcc2.c: In function '__divti3':
/data/repos/gcc/sra/libgcc/libgcc2.c:1246:1: error: non-trivial conversion at assignment
}
^
const union DWunion
void
# .MEM_149 = VDEF <.MEM_148>
nn ={v} .MEM_7(D);
>> + {
>> + /* Replace clobber stmts with a default def. Create a new
>> + variable so we don't later think we must coalesce, which
>> would
>> + fail with some ada abnormal PHIs. Still, we try to keep a
>> + similar name so error messages make sense. */
>> + unlink_stmt_vdef (stmt);
>
> I think that's redundant with gsi_replace (note that using gsi_replace
> looks dangerous here as it calls update_stmt during SSA rewrite...
> that might open a can of worms).
>
>> + gsi_replace (&gsi, gimple_build_nop (), true);
>> + tree id = DECL_NAME (sym);
>> + const char* name = id ? IDENTIFIER_POINTER (id) : 0;
>> + tree newvar = create_tmp_var (TREE_TYPE (sym), name);
>> + def = get_or_create_ssa_default_def (cfun, newvar);
>
> So - can't you simply do
>
> gimple_assign_set_rhs_from_tree (&gsi,
> get_or_create_dda_default_def (cfun, sym));
>
> ? Thus replace x = CLOBBER; with x_3 = x_2(D);
If I write just that, I get a failure because of a missing USE. I need to
run update_stmt (but then you are saying it is dangerous...).
And it also fails to warn for the C++ testcase because SRA sets
nowarning_flag (it doesn't if I create a new variable), but I guess we can
see about changing that next.
--
Marc Glisse
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: update address taken: don't drop clobbers
2014-07-10 15:10 ` Richard Biener
` (2 preceding siblings ...)
2014-07-27 11:53 ` Marc Glisse
@ 2014-07-27 18:01 ` Marc Glisse
2014-09-07 15:28 ` Marc Glisse
2014-10-18 22:23 ` Marc Glisse
4 siblings, 1 reply; 48+ messages in thread
From: Marc Glisse @ 2014-07-27 18:01 UTC (permalink / raw)
To: Richard Biener; +Cc: GCC Patches
On Thu, 10 Jul 2014, Richard Biener wrote:
>> --- gcc/tree-into-ssa.c (revision 212109)
>> +++ gcc/tree-into-ssa.c (working copy)
>> @@ -1831,26 +1831,38 @@ maybe_register_def (def_operand_p def_p,
>> {
>> tree def = DEF_FROM_PTR (def_p);
>> tree sym = DECL_P (def) ? def : SSA_NAME_VAR (def);
>>
>> /* If DEF is a naked symbol that needs renaming, create a new
>> name for it. */
>> if (marked_for_renaming (sym))
>> {
>> if (DECL_P (def))
>> {
>> - tree tracked_var;
>> -
>> - def = make_ssa_name (def, stmt);
>> + if (gimple_clobber_p (stmt) && is_gimple_reg (sym))
>
> sym should always be a gimple reg here (it's marked for renaming).
>
>> + {
>> + /* Replace clobber stmts with a default def. Create a new
>> + variable so we don't later think we must coalesce, which
>> would
>> + fail with some ada abnormal PHIs. Still, we try to keep a
>> + similar name so error messages make sense. */
>> + unlink_stmt_vdef (stmt);
>
> I think that's redundant with gsi_replace (note that using gsi_replace
> looks dangerous here as it calls update_stmt during SSA rewrite...
> that might open a can of worms).
>
>> + gsi_replace (&gsi, gimple_build_nop (), true);
>> + tree id = DECL_NAME (sym);
>> + const char* name = id ? IDENTIFIER_POINTER (id) : 0;
>> + tree newvar = create_tmp_var (TREE_TYPE (sym), name);
>> + def = get_or_create_ssa_default_def (cfun, newvar);
>
> So - can't you simply do
>
> gimple_assign_set_rhs_from_tree (&gsi,
> get_or_create_dda_default_def (cfun, sym));
>
> ? Thus replace x = CLOBBER; with x_3 = x_2(D);
>
>> + }
>> + else
>
> and of course still rewrite the DEF then. IMHO the copy-propagation
> you do is premature optimization.
Using your version, I end up with spurious warnings, in particular for
va_list. pass_fold_builtins stops va_start/va_end taking the address of
the list, so we get:
list_6 = list_2(D);
in place of the clobber at the end of the function. And there is no
DCE-like pass afterwards, so we warn for the use of list_2(D).
(passes.def contains a comment about running dce before uninit)
I don't know if update_address_taken could avoid generating this
assignment where the lhs has 0 use, but this shows the optimization is not
completely premature.
(uninit could also check for this case, but that feels like a bad hack)
--
Marc Glisse
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: update address taken: don't drop clobbers
2014-07-27 18:01 ` Marc Glisse
@ 2014-09-07 15:28 ` Marc Glisse
2014-10-15 14:36 ` Marc Glisse
0 siblings, 1 reply; 48+ messages in thread
From: Marc Glisse @ 2014-09-07 15:28 UTC (permalink / raw)
To: Richard Biener; +Cc: GCC Patches
On Sun, 27 Jul 2014, Marc Glisse wrote:
> On Thu, 10 Jul 2014, Richard Biener wrote:
>
>>> --- gcc/tree-into-ssa.c (revision 212109)
>>> +++ gcc/tree-into-ssa.c (working copy)
>>> @@ -1831,26 +1831,38 @@ maybe_register_def (def_operand_p def_p,
>>> {
>>> tree def = DEF_FROM_PTR (def_p);
>>> tree sym = DECL_P (def) ? def : SSA_NAME_VAR (def);
>>>
>>> /* If DEF is a naked symbol that needs renaming, create a new
>>> name for it. */
>>> if (marked_for_renaming (sym))
>>> {
>>> if (DECL_P (def))
>>> {
>>> - tree tracked_var;
>>> -
>>> - def = make_ssa_name (def, stmt);
>>> + if (gimple_clobber_p (stmt) && is_gimple_reg (sym))
>>
>> sym should always be a gimple reg here (it's marked for renaming).
>>
>>> + {
>>> + /* Replace clobber stmts with a default def. Create a new
>>> + variable so we don't later think we must coalesce, which
>>> would
>>> + fail with some ada abnormal PHIs. Still, we try to keep
>>> a
>>> + similar name so error messages make sense. */
>>> + unlink_stmt_vdef (stmt);
>>
>> I think that's redundant with gsi_replace (note that using gsi_replace
>> looks dangerous here as it calls update_stmt during SSA rewrite...
>> that might open a can of worms).
>>
>>> + gsi_replace (&gsi, gimple_build_nop (), true);
>>> + tree id = DECL_NAME (sym);
>>> + const char* name = id ? IDENTIFIER_POINTER (id) : 0;
>>> + tree newvar = create_tmp_var (TREE_TYPE (sym), name);
>>> + def = get_or_create_ssa_default_def (cfun, newvar);
>>
>> So - can't you simply do
>>
>> gimple_assign_set_rhs_from_tree (&gsi,
>> get_or_create_dda_default_def (cfun, sym));
>>
>> ? Thus replace x = CLOBBER; with x_3 = x_2(D);
>>
>>> + }
>>> + else
>>
>> and of course still rewrite the DEF then. IMHO the copy-propagation
>> you do is premature optimization.
>
> Using your version, I end up with spurious warnings, in particular for
> va_list. pass_fold_builtins stops va_start/va_end taking the address of the
> list, so we get:
>
> list_6 = list_2(D);
>
> in place of the clobber at the end of the function. And there is no DCE-like
> pass afterwards, so we warn for the use of list_2(D).
> (passes.def contains a comment about running dce before uninit)
>
> I don't know if update_address_taken could avoid generating this assignment
> where the lhs has 0 use, but this shows the optimization is not completely
> premature.
>
> (uninit could also check for this case, but that feels like a bad hack)
I would like some guidance on this. I just tried this trivial patch:
NEXT_PASS (pass_split_crit_edges);
+ NEXT_PASS (pass_dce);
NEXT_PASS (pass_late_warn_uninitialized);
and it does not cause any regression, it even XPASS
gfortran.dg/reassoc_6.f for some reason. The FIXME note just above in
passes.def mentions 2 testcases that are already xfailed anyway.
Would that extra pass be acceptable?
Otherwise, what do you think should be responsible for cleaning up the
dead assignments?
--
Marc Glisse
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: update address taken: don't drop clobbers
2014-09-07 15:28 ` Marc Glisse
@ 2014-10-15 14:36 ` Marc Glisse
2014-10-15 16:12 ` Jeff Law
0 siblings, 1 reply; 48+ messages in thread
From: Marc Glisse @ 2014-10-15 14:36 UTC (permalink / raw)
To: GCC Patches
On Sun, 7 Sep 2014, Marc Glisse wrote:
> On Sun, 27 Jul 2014, Marc Glisse wrote:
>
>> On Thu, 10 Jul 2014, Richard Biener wrote:
>>
>>>> --- gcc/tree-into-ssa.c (revision 212109)
>>>> +++ gcc/tree-into-ssa.c (working copy)
>>>> @@ -1831,26 +1831,38 @@ maybe_register_def (def_operand_p def_p,
>>>> {
>>>> tree def = DEF_FROM_PTR (def_p);
>>>> tree sym = DECL_P (def) ? def : SSA_NAME_VAR (def);
>>>>
>>>> /* If DEF is a naked symbol that needs renaming, create a new
>>>> name for it. */
>>>> if (marked_for_renaming (sym))
>>>> {
>>>> if (DECL_P (def))
>>>> {
>>>> - tree tracked_var;
>>>> -
>>>> - def = make_ssa_name (def, stmt);
>>>> + if (gimple_clobber_p (stmt) && is_gimple_reg (sym))
>>>
>>> sym should always be a gimple reg here (it's marked for renaming).
>>>
>>>> + {
>>>> + /* Replace clobber stmts with a default def. Create a new
>>>> + variable so we don't later think we must coalesce, which
>>>> would
>>>> + fail with some ada abnormal PHIs. Still, we try to keep
>>>> a
>>>> + similar name so error messages make sense. */
>>>> + unlink_stmt_vdef (stmt);
>>>
>>> I think that's redundant with gsi_replace (note that using gsi_replace
>>> looks dangerous here as it calls update_stmt during SSA rewrite...
>>> that might open a can of worms).
>>>
>>>> + gsi_replace (&gsi, gimple_build_nop (), true);
>>>> + tree id = DECL_NAME (sym);
>>>> + const char* name = id ? IDENTIFIER_POINTER (id) : 0;
>>>> + tree newvar = create_tmp_var (TREE_TYPE (sym), name);
>>>> + def = get_or_create_ssa_default_def (cfun, newvar);
>>>
>>> So - can't you simply do
>>>
>>> gimple_assign_set_rhs_from_tree (&gsi,
>>> get_or_create_dda_default_def (cfun, sym));
>>>
>>> ? Thus replace x = CLOBBER; with x_3 = x_2(D);
>>>
>>>> + }
>>>> + else
>>>
>>> and of course still rewrite the DEF then. IMHO the copy-propagation
>>> you do is premature optimization.
>>
>> Using your version, I end up with spurious warnings, in particular for
>> va_list. pass_fold_builtins stops va_start/va_end taking the address of the
>> list, so we get:
>>
>> list_6 = list_2(D);
>>
>> in place of the clobber at the end of the function. And there is no
>> DCE-like pass afterwards, so we warn for the use of list_2(D).
>> (passes.def contains a comment about running dce before uninit)
>>
>> I don't know if update_address_taken could avoid generating this assignment
>> where the lhs has 0 use, but this shows the optimization is not completely
>> premature.
>>
>> (uninit could also check for this case, but that feels like a bad hack)
>
> I would like some guidance on this. I just tried this trivial patch:
>
> NEXT_PASS (pass_split_crit_edges);
> + NEXT_PASS (pass_dce);
> NEXT_PASS (pass_late_warn_uninitialized);
>
> and it does not cause any regression, it even XPASS gfortran.dg/reassoc_6.f
> for some reason. The FIXME note just above in passes.def mentions 2 testcases
> that are already xfailed anyway.
>
> Would that extra pass be acceptable?
>
> Otherwise, what do you think should be responsible for cleaning up the dead
> assignments?
Does anyone have an opinion on which side needs to be improved? As a
reminder:
- we have a va_list with its address taken by va_start/va_end.
- fab lowers va_start/va_end and the list doesn't have its address taken
anymore.
- update_address_taken replaces the clobber: list =v {}; with an
assignment of an undefined value: list_6 = list_2(D);
- uninit warns about this.
Some possible directions:
- "prematurely" optimize in update_address_taken so we don't generate the
useless assignment.
- add a dce pass before uninit.
--
Marc Glisse
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: update address taken: don't drop clobbers
2014-10-15 14:36 ` Marc Glisse
@ 2014-10-15 16:12 ` Jeff Law
2014-10-16 11:20 ` Richard Biener
0 siblings, 1 reply; 48+ messages in thread
From: Jeff Law @ 2014-10-15 16:12 UTC (permalink / raw)
To: Marc Glisse, GCC Patches
On 10/15/14 08:35, Marc Glisse wrote:
>>
>> Would that extra pass be acceptable?
>>
>> Otherwise, what do you think should be responsible for cleaning up the
>> dead assignments?
>
> Does anyone have an opinion on which side needs to be improved? As a
> reminder:
>
> - we have a va_list with its address taken by va_start/va_end.
> - fab lowers va_start/va_end and the list doesn't have its address taken
> anymore.
> - update_address_taken replaces the clobber: list =v {}; with an
> assignment of an undefined value: list_6 = list_2(D);
> - uninit warns about this.
>
> Some possible directions:
> - "prematurely" optimize in update_address_taken so we don't generate
> the useless assignment.
> - add a dce pass before uninit.
I tend to land on the side of minimizing false positives, so the comment
about PR18501 is a "don't care" to me. If the optimizers remove a dead
assignment and we no longer warn about a potential uninitialized use in
the dead assignment, then I consider that good. Not everyone agrees
with that way of thinking, obviously.
So my inclination would be to evaluate independent of the pr18501
issues. ie, what's the compile-time cost vs runtime benefit of running
DCE here. I'm guessing there's little runtime benefit for this
particular case.
So my next line of thinking would be can we arrange to conditionally run
DCE? ie, have update_address_taken signal that it did something that
has a reasonable chance of exposing dead code and only run DCE in that
case. Obviously this only helps if it rarely signals :-) I don't think
we have any infrastructure for this right now.
Finally I'd look at how difficult it would be to have
update_address_taken cleanup after itself. If the LHS is in SSA form,
then if we find it has no uses, can we just remove the assignment
completely?
jeff
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: update address taken: don't drop clobbers
2014-10-15 16:12 ` Jeff Law
@ 2014-10-16 11:20 ` Richard Biener
2014-10-16 14:11 ` Marc Glisse
2014-10-16 17:23 ` Jeff Law
0 siblings, 2 replies; 48+ messages in thread
From: Richard Biener @ 2014-10-16 11:20 UTC (permalink / raw)
To: Jeff Law; +Cc: Marc Glisse, GCC Patches
On Wed, Oct 15, 2014 at 6:08 PM, Jeff Law <law@redhat.com> wrote:
> On 10/15/14 08:35, Marc Glisse wrote:
>>>
>>>
>>> Would that extra pass be acceptable?
Ugh, rather not. We have too many passes ;)
>>> Otherwise, what do you think should be responsible for cleaning up the
>>> dead assignments?
>>
>>
>> Does anyone have an opinion on which side needs to be improved? As a
>> reminder:
>>
>> - we have a va_list with its address taken by va_start/va_end.
>> - fab lowers va_start/va_end and the list doesn't have its address taken
>> anymore.
>> - update_address_taken replaces the clobber: list =v {}; with an
>> assignment of an undefined value: list_6 = list_2(D);
>> - uninit warns about this.
>>
>> Some possible directions:
>> - "prematurely" optimize in update_address_taken so we don't generate
>> the useless assignment.
>> - add a dce pass before uninit.
>
> I tend to land on the side of minimizing false positives, so the comment
> about PR18501 is a "don't care" to me. If the optimizers remove a dead
> assignment and we no longer warn about a potential uninitialized use in the
> dead assignment, then I consider that good. Not everyone agrees with that
> way of thinking, obviously.
>
> So my inclination would be to evaluate independent of the pr18501 issues.
> ie, what's the compile-time cost vs runtime benefit of running DCE here.
> I'm guessing there's little runtime benefit for this particular case.
>
> So my next line of thinking would be can we arrange to conditionally run
> DCE? ie, have update_address_taken signal that it did something that has a
> reasonable chance of exposing dead code and only run DCE in that case.
> Obviously this only helps if it rarely signals :-) I don't think we have
> any infrastructure for this right now.
>
> Finally I'd look at how difficult it would be to have update_address_taken
> cleanup after itself. If the LHS is in SSA form, then if we find it has no
> uses, can we just remove the assignment completely?
It doesn't even know that it has no uses (the variable still needs to be
written into SSA form). OTOH it is a missed DSE opportunity before
update-address-taken?
As of premature optimization - into-SSA could notice it created SSA
names with no uses and trigger a fast DCE.
Btw, I wonder what this odd folding of variadic builtins is about, and why
it is not done in the stdarg pass (and only there), which would be earlier.
Richard.
> jeff
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: update address taken: don't drop clobbers
2014-10-16 11:20 ` Richard Biener
@ 2014-10-16 14:11 ` Marc Glisse
2014-10-16 14:34 ` Richard Biener
2014-10-16 17:29 ` Jeff Law
2014-10-16 17:23 ` Jeff Law
1 sibling, 2 replies; 48+ messages in thread
From: Marc Glisse @ 2014-10-16 14:11 UTC (permalink / raw)
To: Richard Biener; +Cc: Jeff Law, GCC Patches
On Thu, 16 Oct 2014, Richard Biener wrote:
> On Wed, Oct 15, 2014 at 6:08 PM, Jeff Law <law@redhat.com> wrote:
>> On 10/15/14 08:35, Marc Glisse wrote:
>>>>
>>>>
>>>> Would that extra pass be acceptable?
>
> Ugh, rather not. We have too many passes ;)
I expected you would say that...
>>>> Otherwise, what do you think should be responsible for cleaning up the
>>>> dead assignments?
>>>
>>>
>>> Does anyone have an opinion on which side needs to be improved? As a
>>> reminder:
>>>
>>> - we have a va_list with its address taken by va_start/va_end.
>>> - fab lowers va_start/va_end and the list doesn't have its address taken
>>> anymore.
>>> - update_address_taken replaces the clobber: list =v {}; with an
>>> assignment of an undefined value: list_6 = list_2(D);
>>> - uninit warns about this.
>>>
>>> Some possible directions:
>>> - "prematurely" optimize in update_address_taken so we don't generate
>>> the useless assignment.
>>> - add a dce pass before uninit.
>>
>> I tend to land on the side of minimizing false positives, so the comment
>> about PR18501 is a "don't care" to me. If the optimizers remove a dead
>> assignment and we no longer warn about a potential uninitialized use in the
>> dead assignment, then I consider that good. Not everyone agrees with that
>> way of thinking, obviously.
I agree with that.
>> So my inclination would be to evaluate independent of the pr18501 issues.
>> ie, what's the compile-time cost vs runtime benefit of running DCE here.
>> I'm guessing there's little runtime benefit for this particular case.
>>
>> So my next line of thinking would be can we arrange to conditionally run
>> DCE? ie, have update_address_taken signal that it did something that has a
>> reasonable chance of exposing dead code and only run DCE in that case.
>> Obviously this only helps if it rarely signals :-) I don't think we have
>> any infrastructure for this right now.
>>
>> Finally I'd look at how difficult it would be to have update_address_taken
>> cleanup after itself. If the LHS is in SSA form, then if we find it has no
>> uses, can we just remove the assignment completely?
>
> It doesn't even know that it has no uses (the variable still needs to be
> written into SSA form). OTOH it is a missed DSE opportunity before
> update-address-taken?
Uh, we are talking about what happens to a clobber when the variable stops
having its address taken. We don't want to DSE clobbers (do we?).
(and va_start is just an example, many transformations could cause this)
> As of premature optimization - into-SSA could notice it created SSA
> names with no uses and trigger a fast DCE.
I am looking into that, it must be doable. It seems not too hard, in
maybe_register_def, to push all results from make_ssa_name to some
data-structure (I don't think new_ssa_names gives me that list, but there
may be other ways to get it without introducing yet another list), and
either mark them as used in maybe_replace_use or get_reaching_def, or
better loop through them at the end, checking has_zero_uses (it is a bit
wasteful, only those coming from clobbers may have 0 uses (or we missed a
dce/dse earlier), but it should be fast enough, even walking on all
ssa_names should be fast enough). If we go to that much trouble, we may as
well clean them while we are there. It isn't obvious to me how to notice
unused new ssa_names more easily, to trigger a DCE.
> Btw, I wonder what this odd folding of variadic builtins is about, and why
> it is not done in the stdarg pass (and only there), which would be earlier.
pass_fold_builtins::execute has:
case BUILT_IN_VA_START:
case BUILT_IN_VA_END:
case BUILT_IN_VA_COPY:
/* These shouldn't be folded before pass_stdarg. */
result = optimize_stdarg_builtin (stmt);
while pass_stdarg only mentions them as:
/* Don't look at __builtin_va_{start,end}, they are ok. */
It does seem late, but that will be for someone else to look at.
By the way, I noticed something strange in tree-into-ssa.c:
static inline bool
is_old_name (tree name)
{
unsigned ver = SSA_NAME_VERSION (name);
if (!new_ssa_names)
return false;
return (ver < SBITMAP_SIZE (new_ssa_names)
&& bitmap_bit_p (old_ssa_names, ver));
}
There are a lot of "new" in this function about "old".
--
Marc Glisse
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: update address taken: don't drop clobbers
2014-10-16 14:11 ` Marc Glisse
@ 2014-10-16 14:34 ` Richard Biener
2014-10-16 17:29 ` Jeff Law
1 sibling, 0 replies; 48+ messages in thread
From: Richard Biener @ 2014-10-16 14:34 UTC (permalink / raw)
To: Marc Glisse; +Cc: Jeff Law, GCC Patches
On Thu, Oct 16, 2014 at 4:11 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
> On Thu, 16 Oct 2014, Richard Biener wrote:
>
>> On Wed, Oct 15, 2014 at 6:08 PM, Jeff Law <law@redhat.com> wrote:
>>>
>>> On 10/15/14 08:35, Marc Glisse wrote:
>>>>>
>>>>>
>>>>>
>>>>> Would that extra pass be acceptable?
>>
>>
>> Ugh, rather not. We have too many passes ;)
>
>
> I expected you would say that...
>
>>>>> Otherwise, what do you think should be responsible for cleaning up the
>>>>> dead assignments?
>>>>
>>>>
>>>>
>>>> Does anyone have an opinion on which side needs to be improved? As a
>>>> reminder:
>>>>
>>>> - we have a va_list with its address taken by va_start/va_end.
>>>> - fab lowers va_start/va_end and the list doesn't have its address taken
>>>> anymore.
>>>> - update_address_taken replaces the clobber: list =v {}; with an
>>>> assignment of an undefined value: list_6 = list_2(D);
>>>> - uninit warns about this.
>>>>
>>>> Some possible directions:
>>>> - "prematurely" optimize in update_address_taken so we don't generate
>>>> the useless assignment.
>>>> - add a dce pass before uninit.
>>>
>>>
>>> I tend to land on the side of minimizing false positives, so the comment
>>> about PR18501 is a "don't care" to me. If the optimizers remove a dead
>>> assignment and we no longer warn about a potential uninitialized use in
>>> the
>>> dead assignment, then I consider that good. Not everyone agrees with
>>> that
>>> way of thinking, obviously.
>
>
> I agree with that.
>
>>> So my inclination would be to evaluate independent of the pr18501 issues.
>>> ie, what's the compile-time cost vs runtime benefit of running DCE here.
>>> I'm guessing there's little runtime benefit for this particular case.
>>>
>>> So my next line of thinking would be can we arrange to conditionally run
>>> DCE? ie, have update_address_taken signal that it did something that has
>>> a
>>> reasonable chance of exposing dead code and only run DCE in that case.
>>> Obviously this only helps if it rarely signals :-) I don't think we have
>>> any infrastructure for this right now.
>>>
>>> Finally I'd look at how difficult it would be to have
>>> update_address_taken
>>> cleanup after itself. If the LHS is in SSA form, then if we find it has
>>> no
>>> uses, can we just remove the assignment completely?
>>
>>
>> It doesn't even know that it has no uses (the variable still needs to be
>> written into SSA form). OTOH it is a missed DSE opportunity before
>> update-address-taken?
>
>
> Uh, we are talking about what happens to a clobber when the variable stops
> having its address taken. We don't want to DSE clobbers (do we?).
No.
> (and va_start is just an example, many transformations could cause this)
Ok, I see.
>> As of premature optimization - into-SSA could notice it created SSA
>> names with no uses and trigger a fast DCE.
>
>
> I am looking into that, it must be doable. It seems not too hard, in
> maybe_register_def, to push all results from make_ssa_name to some
> data-structure (I don't think new_ssa_names gives me that list, but there
> may be other ways to get it without introducing yet another list), and
> either mark them as used in maybe_replace_use or get_reaching_def, or better
> loop through them at the end, checking has_zero_uses (it is a bit wasteful,
> only those coming from clobbers may have 0 uses (or we missed a dce/dse
> earlier), but it should be fast enough, even walking on all ssa_names should
> be fast enough). If we go to that much trouble, we may as well clean them
> while we are there. It isn't obvious to me how to notice unused new
> ssa_names more easily, to trigger a DCE.
Yeah, well. DCE should in theory also be fast (but it got too many features
so it isn't really fast - maybe guard some stuff in it).
>> Btw, I wonder what this odd folding of variadic builtins is about, and why
>> it is not done in the stdarg pass (and only there), which would be
>> earlier.
>
>
> pass_fold_builtins::execute has:
>
> case BUILT_IN_VA_START:
> case BUILT_IN_VA_END:
> case BUILT_IN_VA_COPY:
> /* These shouldn't be folded before pass_stdarg. */
> result = optimize_stdarg_builtin (stmt);
>
> while pass_stdarg only mentions them as:
>
> /* Don't look at __builtin_va_{start,end}, they are ok. */
>
> It does seem late, but that will be for someone else to look at.
>
>
> By the way, I noticed something strange in tree-into-ssa.c:
>
> static inline bool
> is_old_name (tree name)
> {
> unsigned ver = SSA_NAME_VERSION (name);
> if (!new_ssa_names)
> return false;
> return (ver < SBITMAP_SIZE (new_ssa_names)
> && bitmap_bit_p (old_ssa_names, ver));
> }
>
> There are a lot of "new" in this function about "old".
Eh - indeed. Patch to fix that pre-approved.
Richard.
>
> --
> Marc Glisse
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: update address taken: don't drop clobbers
2014-10-16 11:20 ` Richard Biener
2014-10-16 14:11 ` Marc Glisse
@ 2014-10-16 17:23 ` Jeff Law
1 sibling, 0 replies; 48+ messages in thread
From: Jeff Law @ 2014-10-16 17:23 UTC (permalink / raw)
To: Richard Biener; +Cc: Marc Glisse, GCC Patches
On 10/16/14 05:20, Richard Biener wrote:
>
> It doesn't even know that it has no uses (the variable still needs to be
> written into SSA form). OTOH it is a missed DSE opportunity before
> update-address-taken?
Perhaps it's a missed DSE prior to update-address-taken, but I suspect
there aren't many of those in general... It wouldn't be hard to shove
in a DSE pass to get some instrumentation across a wider range of code.
>
> As of premature optimization - into-SSA could notice it created SSA
> names with no uses and trigger a fast DCE.
Yea, I'd been pondering that as well. I'm curious how often we have
trivially dead code after into-ssa. If it's most of the time, then
maybe we just schedule the DCE right after and not even bother to check
if we've got SSA_NAMEs without any uses.
>
> Btw, I wonder what this odd folding of variadic builtins is about, and why
> it is not done in the stdarg pass (and only there), which would be earlier.
No clue. I've tried pretty hard not to think about the lowering of
variadic builtins. Perhaps the author of that code was trying to keep
things simple and let the optimizers to the optimization? :-)
jeff
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: update address taken: don't drop clobbers
2014-10-16 14:11 ` Marc Glisse
2014-10-16 14:34 ` Richard Biener
@ 2014-10-16 17:29 ` Jeff Law
2014-10-16 17:58 ` Richard Biener
1 sibling, 1 reply; 48+ messages in thread
From: Jeff Law @ 2014-10-16 17:29 UTC (permalink / raw)
To: Marc Glisse, Richard Biener; +Cc: GCC Patches
On 10/16/14 08:11, Marc Glisse wrote:
>
> I am looking into that, it must be doable. It seems not too hard, in
> maybe_register_def, to push all results from make_ssa_name to some
> data-structure (I don't think new_ssa_names gives me that list, but
> there may be other ways to get it without introducing yet another list),
> and either mark them as used in maybe_replace_use or get_reaching_def,
> or better loop through them at the end, checking has_zero_uses (it is a
> bit wasteful, only those coming from clobbers may have 0 uses (or we
> missed a dce/dse earlier), but it should be fast enough, even walking on
> all ssa_names should be fast enough). If we go to that much trouble, we
> may as well clean them while we are there. It isn't obvious to me how to
> notice unused new ssa_names more easily, to trigger a DCE.
I'd walk the SSA_NAMEs at the end checking for zero uses. I'm curious
how often that will trigger :-)
jeff
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: update address taken: don't drop clobbers
2014-10-16 17:29 ` Jeff Law
@ 2014-10-16 17:58 ` Richard Biener
2014-10-16 18:37 ` Jeff Law
0 siblings, 1 reply; 48+ messages in thread
From: Richard Biener @ 2014-10-16 17:58 UTC (permalink / raw)
To: Jeff Law, Marc Glisse; +Cc: GCC Patches
On October 16, 2014 7:26:48 PM CEST, Jeff Law <law@redhat.com> wrote:
>On 10/16/14 08:11, Marc Glisse wrote:
>
>>
>> I am looking into that, it must be doable. It seems not too hard, in
>> maybe_register_def, to push all results from make_ssa_name to some
>> data-structure (I don't think new_ssa_names gives me that list, but
>> there may be other ways to get it without introducing yet another
>list),
>> and either mark them as used in maybe_replace_use or
>get_reaching_def,
>> or better loop through them at the end, checking has_zero_uses (it is
>a
>> bit wasteful, only those coming from clobbers may have 0 uses (or we
>> missed a dce/dse earlier), but it should be fast enough, even walking
>on
>> all ssa_names should be fast enough). If we go to that much trouble,
>we
>> may as well clean them while we are there. It isn't obvious to me how
>to
>> notice unused new ssa_names more easily, to trigger a DCE.
>I'd walk the SSA_NAMEs at the end checking for zero uses. I'm curious
>how often that will trigger :-)
Most often for the initial into SSA I guess. After that only for the cases we rename a variable which does not happen often. SRA and update-address-taken cone to my mind.
BTW, I dislike having multiple DCE implementations...
Richard.
>jeff
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: update address taken: don't drop clobbers
2014-10-16 17:58 ` Richard Biener
@ 2014-10-16 18:37 ` Jeff Law
2014-10-17 20:46 ` Marc Glisse
0 siblings, 1 reply; 48+ messages in thread
From: Jeff Law @ 2014-10-16 18:37 UTC (permalink / raw)
To: Richard Biener, Marc Glisse; +Cc: GCC Patches
On 10/16/14 11:52, Richard Biener wrote:
>> I'd walk the SSA_NAMEs at the end checking for zero uses. I'm
>> curious how often that will trigger :-)
>
> Most often for the initial into SSA I guess. After that only for the
> cases we rename a variable which does not happen often. SRA and
> update-address-taken cone to my mind.
That'd be my guess too. Jump threading probably creates them as well
since we don't try to find any of the code that becomes dead in the
duplicates after we remove the conditional. Which means after DOM and VRP.
I don't suppose we have a map of newly created names handy anywhere :-)
>
> BTW, I dislike having multiple DCE implementations...
Similarly. The proposal above was just to determine if we should
schedule DCE or not.
jeff
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: update address taken: don't drop clobbers
2014-10-16 18:37 ` Jeff Law
@ 2014-10-17 20:46 ` Marc Glisse
2014-10-24 20:22 ` Jeff Law
0 siblings, 1 reply; 48+ messages in thread
From: Marc Glisse @ 2014-10-17 20:46 UTC (permalink / raw)
To: Jeff Law; +Cc: Richard Biener, GCC Patches
[-- Attachment #1: Type: TEXT/PLAIN, Size: 724 bytes --]
On Thu, 16 Oct 2014, Jeff Law wrote:
>> BTW, I dislike having multiple DCE implementations...
> Similarly. The proposal above was just to determine if we should schedule
> DCE or not.
Thinking about it some more, I don't think we should need any kind of DCE
here. The rewriting in update_ssa already does a form of forward
propagation that avoids generating dead assignments, the problem only
occurs if we explicitly introduce this new assignment. So I believe we
should go back to an earlier version, like the attached, which is less
work for the compiler.
And now I can go re-read the old discussion (apparently I should avoid
gsi_replace, and there may be other ways to handle the coalescing).
--
Marc Glisse
[-- Attachment #2: Type: TEXT/PLAIN, Size: 8326 bytes --]
Index: testsuite/gcc.dg/tree-ssa/pr60770-1.c
===================================================================
--- testsuite/gcc.dg/tree-ssa/pr60770-1.c (revision 0)
+++ testsuite/gcc.dg/tree-ssa/pr60770-1.c (working copy)
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-options "-O -Wall" } */
+
+int f(int n){
+ int*p;
+ {
+ int yyy=n;
+ p=&yyy;
+ }
+ return *p; /* { dg-warning "yyy" } */
+}
Index: tree-into-ssa.c
===================================================================
--- tree-into-ssa.c (revision 216384)
+++ tree-into-ssa.c (working copy)
@@ -1837,26 +1837,35 @@ maybe_register_def (def_operand_p def_p,
{
tree def = DEF_FROM_PTR (def_p);
tree sym = DECL_P (def) ? def : SSA_NAME_VAR (def);
/* If DEF is a naked symbol that needs renaming, create a new
name for it. */
if (marked_for_renaming (sym))
{
if (DECL_P (def))
{
- tree tracked_var;
-
- def = make_ssa_name (def, stmt);
+ if (gimple_clobber_p (stmt) && is_gimple_reg (sym))
+ {
+ /* Replace clobber stmts with a default def. This new use of a
+ default definition may make it look like SSA_NAMEs have
+ conflicting lifetimes, so we need special code to let them
+ coalesce properly. */
+ unlink_stmt_vdef (stmt);
+ gsi_replace (&gsi, gimple_build_nop (), true);
+ def = get_or_create_ssa_default_def (cfun, sym);
+ }
+ else
+ def = make_ssa_name (def, stmt);
SET_DEF (def_p, def);
- tracked_var = target_for_debug_bind (sym);
+ tree tracked_var = target_for_debug_bind (sym);
if (tracked_var)
{
gimple note = gimple_build_debug_bind (tracked_var, def, stmt);
/* If stmt ends the bb, insert the debug stmt on the single
non-EH edge from the stmt. */
if (gsi_one_before_end_p (gsi) && stmt_ends_bb_p (stmt))
{
basic_block bb = gsi_bb (gsi);
edge_iterator ei;
edge e, ef = NULL;
Index: tree-ssa-live.c
===================================================================
--- tree-ssa-live.c (revision 216384)
+++ tree-ssa-live.c (working copy)
@@ -40,20 +40,21 @@ along with GCC; see the file COPYING3.
#include "stringpool.h"
#include "tree-ssanames.h"
#include "expr.h"
#include "tree-dfa.h"
#include "timevar.h"
#include "dumpfile.h"
#include "tree-ssa-live.h"
#include "diagnostic-core.h"
#include "debug.h"
#include "flags.h"
+#include "tree-ssa.h"
#ifdef ENABLE_CHECKING
static void verify_live_on_entry (tree_live_info_p);
#endif
/* VARMAP maintains a mapping from SSA version number to real variables.
All SSA_NAMES are divided into partitions. Initially each ssa_name is the
only member of it's own partition. Coalescing will attempt to group any
@@ -1086,20 +1087,24 @@ set_var_live_on_entry (tree ssa_name, tr
if (stmt)
{
def_bb = gimple_bb (stmt);
/* Mark defs in liveout bitmap temporarily. */
if (def_bb)
bitmap_set_bit (&live->liveout[def_bb->index], p);
}
else
def_bb = ENTRY_BLOCK_PTR_FOR_FN (cfun);
+ /* An undefined local variable does not need to be very alive. */
+ if (ssa_undefined_value_p (ssa_name, false))
+ return;
+
/* Visit each use of SSA_NAME and if it isn't in the same block as the def,
add it to the list of live on entry blocks. */
FOR_EACH_IMM_USE_FAST (use, imm_iter, ssa_name)
{
gimple use_stmt = USE_STMT (use);
basic_block add_block = NULL;
if (gimple_code (use_stmt) == GIMPLE_PHI)
{
/* Uses in PHI's are considered to be live at exit of the SRC block
@@ -1422,20 +1427,25 @@ verify_live_on_entry (tree_live_info_p l
fprintf (stderr, "\n");
}
else
fprintf (stderr, " and there is no default def.\n");
}
}
}
else
if (d == var)
{
+ /* An undefined local variable does not need to be very
+ alive. */
+ if (ssa_undefined_value_p (var, false))
+ continue;
+
/* The only way this var shouldn't be marked live on entry is
if it occurs in a PHI argument of the block. */
size_t z;
bool ok = false;
gimple_stmt_iterator gsi;
for (gsi = gsi_start_phis (e->dest);
!gsi_end_p (gsi) && !ok;
gsi_next (&gsi))
{
gimple phi = gsi_stmt (gsi);
Index: tree-ssa.c
===================================================================
--- tree-ssa.c (revision 216384)
+++ tree-ssa.c (working copy)
@@ -1178,24 +1178,25 @@ tree_ssa_useless_type_conversion (tree e
tree
tree_ssa_strip_useless_type_conversions (tree exp)
{
while (tree_ssa_useless_type_conversion (exp))
exp = TREE_OPERAND (exp, 0);
return exp;
}
-/* Return true if T, an SSA_NAME, has an undefined value. */
+/* Return true if T, an SSA_NAME, has an undefined value. PARTIAL is what
+ should be returned if the value is only partially undefined. */
bool
-ssa_undefined_value_p (tree t)
+ssa_undefined_value_p (tree t, bool partial)
{
gimple def_stmt;
tree var = SSA_NAME_VAR (t);
if (!var)
;
/* Parameters get their initial value from the function entry. */
else if (TREE_CODE (var) == PARM_DECL)
return false;
/* When returning by reference the return address is actually a hidden
@@ -1205,21 +1206,21 @@ ssa_undefined_value_p (tree t)
/* Hard register variables get their initial value from the ether. */
else if (TREE_CODE (var) == VAR_DECL && DECL_HARD_REGISTER (var))
return false;
/* The value is undefined iff its definition statement is empty. */
def_stmt = SSA_NAME_DEF_STMT (t);
if (gimple_nop_p (def_stmt))
return true;
/* Check if the complex was not only partially defined. */
- if (is_gimple_assign (def_stmt)
+ if (partial && is_gimple_assign (def_stmt)
&& gimple_assign_rhs_code (def_stmt) == COMPLEX_EXPR)
{
tree rhs1, rhs2;
rhs1 = gimple_assign_rhs1 (def_stmt);
rhs2 = gimple_assign_rhs2 (def_stmt);
return (TREE_CODE (rhs1) == SSA_NAME && ssa_undefined_value_p (rhs1))
|| (TREE_CODE (rhs2) == SSA_NAME && ssa_undefined_value_p (rhs2));
}
return false;
@@ -1551,32 +1552,20 @@ execute_update_addresses_taken (void)
rhs = gimple_assign_rhs1 (stmt);
if (gimple_assign_lhs (stmt) != lhs
&& !useless_type_conversion_p (TREE_TYPE (lhs),
TREE_TYPE (rhs)))
rhs = fold_build1 (VIEW_CONVERT_EXPR,
TREE_TYPE (lhs), rhs);
if (gimple_assign_lhs (stmt) != lhs)
gimple_assign_set_lhs (stmt, lhs);
- /* For var ={v} {CLOBBER}; where var lost
- TREE_ADDRESSABLE just remove the stmt. */
- if (DECL_P (lhs)
- && TREE_CLOBBER_P (rhs)
- && bitmap_bit_p (suitable_for_renaming, DECL_UID (lhs)))
- {
- unlink_stmt_vdef (stmt);
- gsi_remove (&gsi, true);
- release_defs (stmt);
- continue;
- }
-
if (gimple_assign_rhs1 (stmt) != rhs)
{
gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
gimple_assign_set_rhs_from_tree (&gsi, rhs);
}
}
else if (gimple_code (stmt) == GIMPLE_CALL)
{
unsigned i;
Index: tree-ssa.h
===================================================================
--- tree-ssa.h (revision 216384)
+++ tree-ssa.h (working copy)
@@ -44,21 +44,21 @@ extern tree target_for_debug_bind (tree)
extern void insert_debug_temp_for_var_def (gimple_stmt_iterator *, tree);
extern void insert_debug_temps_for_defs (gimple_stmt_iterator *);
extern void reset_debug_uses (gimple);
extern void release_defs_bitset (bitmap toremove);
extern void verify_ssa (bool, bool);
extern void init_tree_ssa (struct function *);
extern void delete_tree_ssa (void);
extern bool tree_ssa_useless_type_conversion (tree);
extern tree tree_ssa_strip_useless_type_conversions (tree);
-extern bool ssa_undefined_value_p (tree);
+extern bool ssa_undefined_value_p (tree, bool = true);
extern void execute_update_addresses_taken (void);
/* Given an edge_var_map V, return the PHI arg definition. */
static inline tree
redirect_edge_var_map_def (edge_var_map *v)
{
return v->def;
}
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: update address taken: don't drop clobbers
2014-07-10 15:10 ` Richard Biener
` (3 preceding siblings ...)
2014-07-27 18:01 ` Marc Glisse
@ 2014-10-18 22:23 ` Marc Glisse
2014-10-24 20:19 ` Jeff Law
4 siblings, 1 reply; 48+ messages in thread
From: Marc Glisse @ 2014-10-18 22:23 UTC (permalink / raw)
To: Richard Biener; +Cc: GCC Patches
[-- Attachment #1: Type: TEXT/PLAIN, Size: 2699 bytes --]
On Thu, 10 Jul 2014, Richard Biener wrote:
> On Sun, Jun 29, 2014 at 12:33 AM, Marc Glisse <marc.glisse@inria.fr> wrote:
>>
>> we currently drop clobbers on variables whose address is not taken anymore.
>> However, rewrite_stmt has code to replace them with an SSA_NAME with a
>> default definition (an uninitialized variable), and I believe
>> rewrite_update_stmt should do the same. This allows us to warn sometimes
>> (see testcase), but during the debugging I also noticed several places where
>> it allowed CCP to simplify further PHIs, so this is also an optimization.
>>
>> In an earlier version of the patch, I was using
>> get_or_create_ssa_default_def (cfun, sym);
>> (I was reusing the same variable). This passed bootstrap+testsuite on all
>> languages except for ada. Indeed, the compiler wanted to coalesce several
>> SSA_NAMEs, including those new ones, in out-of-ssa, but couldn't. There are
>> abnormal PHIs involved. Maybe it shouldn't have insisted on coalescing an
>> undefined ssa_name, maybe something should have prevented us from reaching
>> such a situation, but creating a new variable was the simplest workaround.
>
> Hmm. We indeed notice "late" that the new names are used in abnormal
> PHIs. Note that usually rewriting a symbol into SSA form does not
> create overlapping life-ranges - but of course you are possibly introducing
> those by the new use of the default definitions.
>
> Apart from the out-of-SSA patch you proposed elsewhere a possibility
> is to simply never mark undefined SSA names as
> SSA_NAME_OCCURS_IN_ABNORMAL_PHI ... (or not mark those
> as must-coalesce).
For "not mark those as must-coalesce", replacing the liveness patch with
the attached patch also passed the testsuite: I skip undefined variables
when handling must-coalesce, and let the regular coalescing code handle
them. I am not sure what happens during expansion though, and bootstrap
only hits this issue a couple times in ada so it doesn't prove much.
This patch doesn't conflict with the liveness patch, they are rather
complementary. I didn't test but I am quite confident that having both
patches would also pass bootstrap+testsuite.
Of course that all becomes unnecessary if we use default definitions of
new variables instead of always the same old variable, but I can
understand not wanting all those new artificial variables.
I would be ok with the patch at
https://gcc.gnu.org/ml/gcc-patches/2014-10/msg01787.html
(minus the line with unlink_stmt_vdef, which is indeed unnecessary)
(I looked at what gsi_replace does when replacing a clobber by a nop, and
it seems harmless, but I can manually inline the non-dead parts of it if
you want)
--
Marc Glisse
[-- Attachment #2: Type: TEXT/PLAIN, Size: 3512 bytes --]
Index: tree-ssa-coalesce.c
===================================================================
--- tree-ssa-coalesce.c (revision 216415)
+++ tree-ssa-coalesce.c (working copy)
@@ -36,20 +36,21 @@ along with GCC; see the file COPYING3.
#include "gimple.h"
#include "gimple-iterator.h"
#include "gimple-ssa.h"
#include "tree-phinodes.h"
#include "ssa-iterators.h"
#include "stringpool.h"
#include "tree-ssanames.h"
#include "tree-ssa-live.h"
#include "tree-ssa-coalesce.h"
#include "diagnostic-core.h"
+#include "tree-ssa.h"
/* This set of routines implements a coalesce_list. This is an object which
is used to track pairs of ssa_names which are desirable to coalesce
together to avoid copies. Costs are associated with each pair, and when
all desired information has been collected, the object can be used to
order the pairs for processing. */
/* This structure defines a pair entry. */
@@ -962,20 +963,22 @@ create_outofssa_var_map (coalesce_list_p
saw_copy = true;
bitmap_set_bit (used_in_copy, SSA_NAME_VERSION (arg));
if ((e->flags & EDGE_ABNORMAL) == 0)
{
int cost = coalesce_cost_edge (e);
if (cost == 1 && has_single_use (arg))
add_cost_one_coalesce (cl, ver, SSA_NAME_VERSION (arg));
else
add_coalesce (cl, ver, SSA_NAME_VERSION (arg), cost);
}
+ else if (ssa_undefined_value_p (arg, false))
+ add_coalesce (cl, ver, SSA_NAME_VERSION (arg), MUST_COALESCE_COST - 1);
}
}
if (saw_copy)
bitmap_set_bit (used_in_copy, ver);
}
for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
{
stmt = gsi_stmt (gsi);
@@ -1189,20 +1192,22 @@ coalesce_partitions (var_map map, ssa_co
FOR_EACH_EDGE (e, ei, bb->preds)
if (e->flags & EDGE_ABNORMAL)
{
gimple_stmt_iterator gsi;
for (gsi = gsi_start_phis (bb); !gsi_end_p (gsi);
gsi_next (&gsi))
{
gimple phi = gsi_stmt (gsi);
tree res = PHI_RESULT (phi);
tree arg = PHI_ARG_DEF (phi, e->dest_idx);
+ if (ssa_undefined_value_p (arg, false))
+ continue;
int v1 = SSA_NAME_VERSION (res);
int v2 = SSA_NAME_VERSION (arg);
if (debug)
fprintf (debug, "Abnormal coalesce: ");
if (!attempt_coalesce (map, graph, v1, v2, debug))
fail_abnormal_edge_coalesce (v1, v2);
}
}
@@ -1287,21 +1292,22 @@ coalesce_ssa_name (void)
{
/* If the variable is a PARM_DECL or a RESULT_DECL, we
_require_ that all the names originating from it be
coalesced, because there must be a single partition
containing all the names so that it can be assigned
the canonical RTL location of the DECL safely.
If in_lto_p, a function could have been compiled
originally with optimizations and only the link
performed at -O0, so we can't actually require it. */
const int cost
- = (TREE_CODE (SSA_NAME_VAR (a)) == VAR_DECL || in_lto_p)
+ = (TREE_CODE (SSA_NAME_VAR (a)) == VAR_DECL || in_lto_p
+ || ssa_undefined_value_p (a, false))
? MUST_COALESCE_COST - 1 : MUST_COALESCE_COST;
add_coalesce (cl, SSA_NAME_VERSION (a),
SSA_NAME_VERSION (*slot), cost);
bitmap_set_bit (used_in_copies, SSA_NAME_VERSION (a));
bitmap_set_bit (used_in_copies, SSA_NAME_VERSION (*slot));
}
}
}
}
if (dump_file && (dump_flags & TDF_DETAILS))
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: update address taken: don't drop clobbers
2014-10-18 22:23 ` Marc Glisse
@ 2014-10-24 20:19 ` Jeff Law
0 siblings, 0 replies; 48+ messages in thread
From: Jeff Law @ 2014-10-24 20:19 UTC (permalink / raw)
To: Marc Glisse, Richard Biener; +Cc: GCC Patches
On 10/18/14 15:59, Marc Glisse wrote:
> On Thu, 10 Jul 2014, Richard Biener wrote:
>
>> On Sun, Jun 29, 2014 at 12:33 AM, Marc Glisse <marc.glisse@inria.fr>
>> wrote:
>>>
>>> we currently drop clobbers on variables whose address is not taken
>>> anymore.
>>> However, rewrite_stmt has code to replace them with an SSA_NAME with a
>>> default definition (an uninitialized variable), and I believe
>>> rewrite_update_stmt should do the same. This allows us to warn sometimes
>>> (see testcase), but during the debugging I also noticed several
>>> places where
>>> it allowed CCP to simplify further PHIs, so this is also an
>>> optimization.
>>>
>>> In an earlier version of the patch, I was using
>>> get_or_create_ssa_default_def (cfun, sym);
>>> (I was reusing the same variable). This passed bootstrap+testsuite on
>>> all
>>> languages except for ada. Indeed, the compiler wanted to coalesce
>>> several
>>> SSA_NAMEs, including those new ones, in out-of-ssa, but couldn't.
>>> There are
>>> abnormal PHIs involved. Maybe it shouldn't have insisted on
>>> coalescing an
>>> undefined ssa_name, maybe something should have prevented us from
>>> reaching
>>> such a situation, but creating a new variable was the simplest
>>> workaround.
>>
>> Hmm. We indeed notice "late" that the new names are used in abnormal
>> PHIs. Note that usually rewriting a symbol into SSA form does not
>> create overlapping life-ranges - but of course you are possibly
>> introducing
>> those by the new use of the default definitions.
>>
>> Apart from the out-of-SSA patch you proposed elsewhere a possibility
>> is to simply never mark undefined SSA names as
>> SSA_NAME_OCCURS_IN_ABNORMAL_PHI ... (or not mark those
>> as must-coalesce).
>
> For "not mark those as must-coalesce", replacing the liveness patch with
> the attached patch also passed the testsuite: I skip undefined variables
> when handling must-coalesce, and let the regular coalescing code handle
> them. I am not sure what happens during expansion though, and bootstrap
> only hits this issue a couple times in ada so it doesn't prove much.
>
> This patch doesn't conflict with the liveness patch, they are rather
> complementary. I didn't test but I am quite confident that having both
> patches would also pass bootstrap+testsuite.
>
> Of course that all becomes unnecessary if we use default definitions of
> new variables instead of always the same old variable, but I can
> understand not wanting all those new artificial variables.
>
> I would be ok with the patch at
> https://gcc.gnu.org/ml/gcc-patches/2014-10/msg01787.html
> (minus the line with unlink_stmt_vdef, which is indeed unnecessary)
> (I looked at what gsi_replace does when replacing a clobber by a nop,
> and it seems harmless, but I can manually inline the non-dead parts of
> it if you want)
So I'm still trying to get comfortable with this patch. I guess my
concerns about having one of the undefined value SSA_NAMEs appearing in
two conflicting coalesce lists are alleviated by the twiddle to
coalesce_partitions where we essentially ignore them.
So in the end, they don't end up a part of any partition? What happens
when we expand them? I guess they get a new pseudo since they're a
distinct partition? If we had a sensible story for expansion, then I
could probably get on board with this patch.
And I'm still going to look at the other as well -- as you mention,
they're independent.
jeff
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: update address taken: don't drop clobbers
2014-10-17 20:46 ` Marc Glisse
@ 2014-10-24 20:22 ` Jeff Law
2014-10-25 8:06 ` Marc Glisse
2014-10-25 17:14 ` Marc Glisse
0 siblings, 2 replies; 48+ messages in thread
From: Jeff Law @ 2014-10-24 20:22 UTC (permalink / raw)
To: Marc Glisse; +Cc: Richard Biener, GCC Patches
On 10/17/14 14:41, Marc Glisse wrote:
> On Thu, 16 Oct 2014, Jeff Law wrote:
>
>>> BTW, I dislike having multiple DCE implementations...
>> Similarly. The proposal above was just to determine if we should
>> schedule DCE or not.
>
> Thinking about it some more, I don't think we should need any kind of
> DCE here. The rewriting in update_ssa already does a form of forward
> propagation that avoids generating dead assignments, the problem only
> occurs if we explicitly introduce this new assignment. So I believe we
> should go back to an earlier version, like the attached, which is less
> work for the compiler.
>
> And now I can go re-read the old discussion (apparently I should avoid
> gsi_replace, and there may be other ways to handle the coalescing).
>
I'm starting to agree -- a later message indicated you wanted to drop
the unlink_stmt_vdef call and you wanted to avoid gsi_replace, that
seems fine. I'll approve once those things are taken care of.
jeff
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: update address taken: don't drop clobbers
2014-10-24 20:22 ` Jeff Law
@ 2014-10-25 8:06 ` Marc Glisse
2014-10-31 21:06 ` Jeff Law
2014-10-25 17:14 ` Marc Glisse
1 sibling, 1 reply; 48+ messages in thread
From: Marc Glisse @ 2014-10-25 8:06 UTC (permalink / raw)
To: Jeff Law; +Cc: GCC Patches
(replying to both messages)
On Fri, 24 Oct 2014, Jeff Law wrote:
[ https://gcc.gnu.org/ml/gcc-patches/2014-10/msg01830.html ]
> So I'm still trying to get comfortable with this patch. I guess my concerns
> about having one of the undefined value SSA_NAMEs appearing in two
> conflicting coalesce lists are alleviated by the twiddle to
> coalesce_partitions where we essentially ignore them.
>
> So in the end, they don't end up a part of any partition?
Without the liveness patch, their lifetime should mean that they don't
coalesce with anything. But I would expect they get their own partition
then (I am forgetting these details way too fast...). With the liveness
patch which gives them an empty lifetime, they should all coalesce with at
least one other ssa_name.
> What happens when we expand them? I guess they get a new pseudo since
> they're a distinct partition? If we had a sensible story for expansion,
> then I could probably get on board with this patch.
As I mentioned in my message, I don't know. Last time I looked I couldn't
find how coalescing was actually performed. tree-outof-ssa.c has a
function rewrite_trees with a promising comment but an empty body :-/
I agree that we need to understand what happens at expansion time when the
variables are not coalesced before pushing a patch that prevents
coalescing. I was kind of hoping someone would have a pointer...
On Fri, 24 Oct 2014, Jeff Law wrote:
[ https://gcc.gnu.org/ml/gcc-patches/2014-10/msg01787.html ]
> On 10/17/14 14:41, Marc Glisse wrote:
>>
>> Thinking about it some more, I don't think we should need any kind of
>> DCE here. The rewriting in update_ssa already does a form of forward
>> propagation that avoids generating dead assignments, the problem only
>> occurs if we explicitly introduce this new assignment. So I believe we
>> should go back to an earlier version, like the attached, which is less
>> work for the compiler.
>>
>> And now I can go re-read the old discussion (apparently I should avoid
>> gsi_replace, and there may be other ways to handle the coalescing).
>>
> I'm starting to agree -- a later message indicated you wanted to drop the
> unlink_stmt_vdef call and you wanted to avoid gsi_replace, that seems fine.
> I'll approve once those things are taken care of.
I don't really want to avoid gsi_replace, but I am willing to do it if it
makes reviewers nervous to call such a high-level function in the cleanup
code between passes (Richard in particular was unhappy about it).
To clarify things so I know what to test and re-post, we are talking
about the patch in
https://gcc.gnu.org/ml/gcc-patches/2014-10/msg01787.html and we can forget
about the coalescing thing in
https://gcc.gnu.org/ml/gcc-patches/2014-10/msg01830.html ?
(I'd be happy with that :-)
Thanks,
--
Marc Glisse
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: update address taken: don't drop clobbers
2014-10-24 20:22 ` Jeff Law
2014-10-25 8:06 ` Marc Glisse
@ 2014-10-25 17:14 ` Marc Glisse
2014-10-31 11:12 ` Richard Biener
2014-10-31 21:16 ` Jeff Law
1 sibling, 2 replies; 48+ messages in thread
From: Marc Glisse @ 2014-10-25 17:14 UTC (permalink / raw)
To: Jeff Law; +Cc: GCC Patches
[-- Attachment #1: Type: TEXT/PLAIN, Size: 1187 bytes --]
On Fri, 24 Oct 2014, Jeff Law wrote:
> I'm starting to agree -- a later message indicated you wanted to drop the
> unlink_stmt_vdef call and you wanted to avoid gsi_replace, that seems fine.
> I'll approve once those things are taken care of.
The following passed bootstrap+testsuite. I wasn't sure what exactly a
clobber is guaranteed not to have (no histograms for instance? At least I
assumed it doesn't throw) so I may have kept some unnecessary calls when I
inlined gsi_replace. I'd be happy to remove any you feel is useless.
2014-10-26 Marc Glisse <marc.glisse@inria.fr>
PR tree-optimization/60770
gcc/
* tree-into-ssa.c: Include value-prof.h.
(maybe_register_def): Replace clobbers with default definitions.
* tree-ssa-live.c: Include tree-ssa.h.
(set_var_live_on_entry): Do not mark undefined variables as live.
(verify_live_on_entry): Do not check undefined variables.
* tree-ssa.h (ssa_undefined_value_p): New parameter for the case
of partially undefined variables.
* tree-ssa.c (ssa_undefined_value_p): Likewise.
(execute_update_addresses_taken): Do not drop clobbers.
gcc/testsuite/
* gcc.dg/tree-ssa/pr60770-1.c: New file.
--
Marc Glisse
[-- Attachment #2: Type: TEXT/PLAIN, Size: 9455 bytes --]
Index: gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c
===================================================================
--- gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c (revision 0)
+++ gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c (working copy)
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-options "-O -Wall" } */
+
+int f(int n){
+ int*p;
+ {
+ int yyy=n;
+ p=&yyy;
+ }
+ return *p; /* { dg-warning "yyy" } */
+}
Index: gcc/tree-into-ssa.c
===================================================================
--- gcc/tree-into-ssa.c (revision 216689)
+++ gcc/tree-into-ssa.c (working copy)
@@ -52,20 +52,21 @@ along with GCC; see the file COPYING3.
#include "expr.h"
#include "tree-dfa.h"
#include "tree-ssa.h"
#include "tree-inline.h"
#include "tree-pass.h"
#include "cfgloop.h"
#include "domwalk.h"
#include "params.h"
#include "diagnostic-core.h"
#include "tree-into-ssa.h"
+#include "value-prof.h"
#define PERCENT(x,y) ((float)(x) * 100.0 / (float)(y))
/* This file builds the SSA form for a function as described in:
R. Cytron, J. Ferrante, B. Rosen, M. Wegman, and K. Zadeck. Efficiently
Computing Static Single Assignment Form and the Control Dependence
Graph. ACM Transactions on Programming Languages and Systems,
13(4):451-490, October 1991. */
/* Structure to map a variable VAR to the set of blocks that contain
@@ -1837,26 +1838,42 @@ maybe_register_def (def_operand_p def_p,
{
tree def = DEF_FROM_PTR (def_p);
tree sym = DECL_P (def) ? def : SSA_NAME_VAR (def);
/* If DEF is a naked symbol that needs renaming, create a new
name for it. */
if (marked_for_renaming (sym))
{
if (DECL_P (def))
{
- tree tracked_var;
+ if (gimple_clobber_p (stmt) && is_gimple_reg (sym))
+ {
+ /* Replace clobber stmts with a default def. This new use of a
+ default definition may make it look like SSA_NAMEs have
+ conflicting lifetimes, so we need special code to let them
+ coalesce properly. */
+ /* Hand-inlined version of the following, for safety
+ gsi_replace (&gsi, gimple_build_nop (), true); */
+ gimple nop = gimple_build_nop ();
+ gimple_set_bb (nop, gsi_bb (gsi));
+ gimple_set_bb (stmt, NULL);
+ gimple_remove_stmt_histograms (cfun, stmt);
+ delink_stmt_imm_use (stmt);
+ gsi_set_stmt (&gsi, nop);
- def = make_ssa_name (def, stmt);
+ def = get_or_create_ssa_default_def (cfun, sym);
+ }
+ else
+ def = make_ssa_name (def, stmt);
SET_DEF (def_p, def);
- tracked_var = target_for_debug_bind (sym);
+ tree tracked_var = target_for_debug_bind (sym);
if (tracked_var)
{
gimple note = gimple_build_debug_bind (tracked_var, def, stmt);
/* If stmt ends the bb, insert the debug stmt on the single
non-EH edge from the stmt. */
if (gsi_one_before_end_p (gsi) && stmt_ends_bb_p (stmt))
{
basic_block bb = gsi_bb (gsi);
edge_iterator ei;
edge e, ef = NULL;
Index: gcc/tree-ssa-live.c
===================================================================
--- gcc/tree-ssa-live.c (revision 216689)
+++ gcc/tree-ssa-live.c (working copy)
@@ -40,20 +40,21 @@ along with GCC; see the file COPYING3.
#include "stringpool.h"
#include "tree-ssanames.h"
#include "expr.h"
#include "tree-dfa.h"
#include "timevar.h"
#include "dumpfile.h"
#include "tree-ssa-live.h"
#include "diagnostic-core.h"
#include "debug.h"
#include "flags.h"
+#include "tree-ssa.h"
#ifdef ENABLE_CHECKING
static void verify_live_on_entry (tree_live_info_p);
#endif
/* VARMAP maintains a mapping from SSA version number to real variables.
All SSA_NAMES are divided into partitions. Initially each ssa_name is the
only member of it's own partition. Coalescing will attempt to group any
@@ -1086,20 +1087,24 @@ set_var_live_on_entry (tree ssa_name, tr
if (stmt)
{
def_bb = gimple_bb (stmt);
/* Mark defs in liveout bitmap temporarily. */
if (def_bb)
bitmap_set_bit (&live->liveout[def_bb->index], p);
}
else
def_bb = ENTRY_BLOCK_PTR_FOR_FN (cfun);
+ /* An undefined local variable does not need to be very alive. */
+ if (ssa_undefined_value_p (ssa_name, false))
+ return;
+
/* Visit each use of SSA_NAME and if it isn't in the same block as the def,
add it to the list of live on entry blocks. */
FOR_EACH_IMM_USE_FAST (use, imm_iter, ssa_name)
{
gimple use_stmt = USE_STMT (use);
basic_block add_block = NULL;
if (gimple_code (use_stmt) == GIMPLE_PHI)
{
/* Uses in PHI's are considered to be live at exit of the SRC block
@@ -1422,20 +1427,25 @@ verify_live_on_entry (tree_live_info_p l
fprintf (stderr, "\n");
}
else
fprintf (stderr, " and there is no default def.\n");
}
}
}
else
if (d == var)
{
+ /* An undefined local variable does not need to be very
+ alive. */
+ if (ssa_undefined_value_p (var, false))
+ continue;
+
/* The only way this var shouldn't be marked live on entry is
if it occurs in a PHI argument of the block. */
size_t z;
bool ok = false;
gimple_stmt_iterator gsi;
for (gsi = gsi_start_phis (e->dest);
!gsi_end_p (gsi) && !ok;
gsi_next (&gsi))
{
gimple phi = gsi_stmt (gsi);
Index: gcc/tree-ssa.c
===================================================================
--- gcc/tree-ssa.c (revision 216689)
+++ gcc/tree-ssa.c (working copy)
@@ -1178,24 +1178,25 @@ tree_ssa_useless_type_conversion (tree e
tree
tree_ssa_strip_useless_type_conversions (tree exp)
{
while (tree_ssa_useless_type_conversion (exp))
exp = TREE_OPERAND (exp, 0);
return exp;
}
-/* Return true if T, an SSA_NAME, has an undefined value. */
+/* Return true if T, an SSA_NAME, has an undefined value. PARTIAL is what
+ should be returned if the value is only partially undefined. */
bool
-ssa_undefined_value_p (tree t)
+ssa_undefined_value_p (tree t, bool partial)
{
gimple def_stmt;
tree var = SSA_NAME_VAR (t);
if (!var)
;
/* Parameters get their initial value from the function entry. */
else if (TREE_CODE (var) == PARM_DECL)
return false;
/* When returning by reference the return address is actually a hidden
@@ -1205,21 +1206,21 @@ ssa_undefined_value_p (tree t)
/* Hard register variables get their initial value from the ether. */
else if (TREE_CODE (var) == VAR_DECL && DECL_HARD_REGISTER (var))
return false;
/* The value is undefined iff its definition statement is empty. */
def_stmt = SSA_NAME_DEF_STMT (t);
if (gimple_nop_p (def_stmt))
return true;
/* Check if the complex was not only partially defined. */
- if (is_gimple_assign (def_stmt)
+ if (partial && is_gimple_assign (def_stmt)
&& gimple_assign_rhs_code (def_stmt) == COMPLEX_EXPR)
{
tree rhs1, rhs2;
rhs1 = gimple_assign_rhs1 (def_stmt);
rhs2 = gimple_assign_rhs2 (def_stmt);
return (TREE_CODE (rhs1) == SSA_NAME && ssa_undefined_value_p (rhs1))
|| (TREE_CODE (rhs2) == SSA_NAME && ssa_undefined_value_p (rhs2));
}
return false;
@@ -1551,32 +1552,20 @@ execute_update_addresses_taken (void)
rhs = gimple_assign_rhs1 (stmt);
if (gimple_assign_lhs (stmt) != lhs
&& !useless_type_conversion_p (TREE_TYPE (lhs),
TREE_TYPE (rhs)))
rhs = fold_build1 (VIEW_CONVERT_EXPR,
TREE_TYPE (lhs), rhs);
if (gimple_assign_lhs (stmt) != lhs)
gimple_assign_set_lhs (stmt, lhs);
- /* For var ={v} {CLOBBER}; where var lost
- TREE_ADDRESSABLE just remove the stmt. */
- if (DECL_P (lhs)
- && TREE_CLOBBER_P (rhs)
- && bitmap_bit_p (suitable_for_renaming, DECL_UID (lhs)))
- {
- unlink_stmt_vdef (stmt);
- gsi_remove (&gsi, true);
- release_defs (stmt);
- continue;
- }
-
if (gimple_assign_rhs1 (stmt) != rhs)
{
gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
gimple_assign_set_rhs_from_tree (&gsi, rhs);
}
}
else if (gimple_code (stmt) == GIMPLE_CALL)
{
unsigned i;
Index: gcc/tree-ssa.h
===================================================================
--- gcc/tree-ssa.h (revision 216689)
+++ gcc/tree-ssa.h (working copy)
@@ -44,21 +44,21 @@ extern tree target_for_debug_bind (tree)
extern void insert_debug_temp_for_var_def (gimple_stmt_iterator *, tree);
extern void insert_debug_temps_for_defs (gimple_stmt_iterator *);
extern void reset_debug_uses (gimple);
extern void release_defs_bitset (bitmap toremove);
extern void verify_ssa (bool, bool);
extern void init_tree_ssa (struct function *);
extern void delete_tree_ssa (void);
extern bool tree_ssa_useless_type_conversion (tree);
extern tree tree_ssa_strip_useless_type_conversions (tree);
-extern bool ssa_undefined_value_p (tree);
+extern bool ssa_undefined_value_p (tree, bool = true);
extern void execute_update_addresses_taken (void);
/* Given an edge_var_map V, return the PHI arg definition. */
static inline tree
redirect_edge_var_map_def (edge_var_map *v)
{
return v->def;
}
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: update address taken: don't drop clobbers
2014-10-25 17:14 ` Marc Glisse
@ 2014-10-31 11:12 ` Richard Biener
2014-11-02 10:34 ` Marc Glisse
2014-10-31 21:16 ` Jeff Law
1 sibling, 1 reply; 48+ messages in thread
From: Richard Biener @ 2014-10-31 11:12 UTC (permalink / raw)
To: Marc Glisse; +Cc: Jeff Law, GCC Patches
On Sat, Oct 25, 2014 at 6:29 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
> On Fri, 24 Oct 2014, Jeff Law wrote:
>
>> I'm starting to agree -- a later message indicated you wanted to drop the
>> unlink_stmt_vdef call and you wanted to avoid gsi_replace, that seems fine.
>> I'll approve once those things are taken care of.
>
>
> The following passed bootstrap+testsuite. I wasn't sure what exactly a
> clobber is guaranteed not to have (no histograms for instance? At least I
> assumed it doesn't throw) so I may have kept some unnecessary calls when I
> inlined gsi_replace. I'd be happy to remove any you feel is useless.
>
> 2014-10-26 Marc Glisse <marc.glisse@inria.fr>
>
> PR tree-optimization/60770
> gcc/
> * tree-into-ssa.c: Include value-prof.h.
> (maybe_register_def): Replace clobbers with default definitions.
> * tree-ssa-live.c: Include tree-ssa.h.
> (set_var_live_on_entry): Do not mark undefined variables as live.
> (verify_live_on_entry): Do not check undefined variables.
> * tree-ssa.h (ssa_undefined_value_p): New parameter for the case
> of partially undefined variables.
> * tree-ssa.c (ssa_undefined_value_p): Likewise.
> (execute_update_addresses_taken): Do not drop clobbers.
>
> gcc/testsuite/
> * gcc.dg/tree-ssa/pr60770-1.c: New file.
>
> --
> Marc Glisse
>
> Index: gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c (revision 0)
> +++ gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c (working copy)
> @@ -0,0 +1,11 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O -Wall" } */
> +
> +int f(int n){
> + int*p;
> + {
> + int yyy=n;
> + p=&yyy;
> + }
> + return *p; /* { dg-warning "yyy" } */
> +}
> Index: gcc/tree-into-ssa.c
> ===================================================================
> --- gcc/tree-into-ssa.c (revision 216689)
> +++ gcc/tree-into-ssa.c (working copy)
> @@ -52,20 +52,21 @@ along with GCC; see the file COPYING3.
> #include "expr.h"
> #include "tree-dfa.h"
> #include "tree-ssa.h"
> #include "tree-inline.h"
> #include "tree-pass.h"
> #include "cfgloop.h"
> #include "domwalk.h"
> #include "params.h"
> #include "diagnostic-core.h"
> #include "tree-into-ssa.h"
> +#include "value-prof.h"
>
> #define PERCENT(x,y) ((float)(x) * 100.0 / (float)(y))
>
> /* This file builds the SSA form for a function as described in:
> R. Cytron, J. Ferrante, B. Rosen, M. Wegman, and K. Zadeck. Efficiently
> Computing Static Single Assignment Form and the Control Dependence
> Graph. ACM Transactions on Programming Languages and Systems,
> 13(4):451-490, October 1991. */
>
> /* Structure to map a variable VAR to the set of blocks that contain
> @@ -1837,26 +1838,42 @@ maybe_register_def (def_operand_p def_p,
> {
> tree def = DEF_FROM_PTR (def_p);
> tree sym = DECL_P (def) ? def : SSA_NAME_VAR (def);
>
> /* If DEF is a naked symbol that needs renaming, create a new
> name for it. */
> if (marked_for_renaming (sym))
> {
> if (DECL_P (def))
> {
> - tree tracked_var;
> + if (gimple_clobber_p (stmt) && is_gimple_reg (sym))
I think you know that sym is a gimple-reg as the code previously
unconditionally generated an SSA name for it.
> + {
> + /* Replace clobber stmts with a default def. This new use of a
> + default definition may make it look like SSA_NAMEs have
> + conflicting lifetimes, so we need special code to let them
> + coalesce properly. */
> + /* Hand-inlined version of the following, for safety
> + gsi_replace (&gsi, gimple_build_nop (), true); */
> + gimple nop = gimple_build_nop ();
> + gimple_set_bb (nop, gsi_bb (gsi));
> + gimple_set_bb (stmt, NULL);
> + gimple_remove_stmt_histograms (cfun, stmt);
> + delink_stmt_imm_use (stmt);
> + gsi_set_stmt (&gsi, nop);
Is there any reason for this dance? I'd rather have maybe_register_def
return a bool whether to remove the stmt... passing it down to the
single caller of rewrite_update_stmt which can then gsi_remove the
stmt.
> - def = make_ssa_name (def, stmt);
> + def = get_or_create_ssa_default_def (cfun, sym);
I think if 'def' turns out to be a PARM_DECL this does the wrong
thing (well, not technically wrong... but maybe unexpected). Not
sure if we ever end up with a PARM = {} clobber though. Maybe
guard all this with TREE_CODE (def) == VAR_DECL for extra
safety.
Otherwise the patch looks ok.
Thanks for your patience.
Richard.
> + }
> + else
> + def = make_ssa_name (def, stmt);
> SET_DEF (def_p, def);
>
> - tracked_var = target_for_debug_bind (sym);
> + tree tracked_var = target_for_debug_bind (sym);
> if (tracked_var)
> {
> gimple note = gimple_build_debug_bind (tracked_var, def,
> stmt);
> /* If stmt ends the bb, insert the debug stmt on the single
> non-EH edge from the stmt. */
> if (gsi_one_before_end_p (gsi) && stmt_ends_bb_p (stmt))
> {
> basic_block bb = gsi_bb (gsi);
> edge_iterator ei;
> edge e, ef = NULL;
> Index: gcc/tree-ssa-live.c
> ===================================================================
> --- gcc/tree-ssa-live.c (revision 216689)
> +++ gcc/tree-ssa-live.c (working copy)
> @@ -40,20 +40,21 @@ along with GCC; see the file COPYING3.
> #include "stringpool.h"
> #include "tree-ssanames.h"
> #include "expr.h"
> #include "tree-dfa.h"
> #include "timevar.h"
> #include "dumpfile.h"
> #include "tree-ssa-live.h"
> #include "diagnostic-core.h"
> #include "debug.h"
> #include "flags.h"
> +#include "tree-ssa.h"
>
> #ifdef ENABLE_CHECKING
> static void verify_live_on_entry (tree_live_info_p);
> #endif
>
>
> /* VARMAP maintains a mapping from SSA version number to real variables.
>
> All SSA_NAMES are divided into partitions. Initially each ssa_name is
> the
> only member of it's own partition. Coalescing will attempt to group any
> @@ -1086,20 +1087,24 @@ set_var_live_on_entry (tree ssa_name, tr
> if (stmt)
> {
> def_bb = gimple_bb (stmt);
> /* Mark defs in liveout bitmap temporarily. */
> if (def_bb)
> bitmap_set_bit (&live->liveout[def_bb->index], p);
> }
> else
> def_bb = ENTRY_BLOCK_PTR_FOR_FN (cfun);
>
> + /* An undefined local variable does not need to be very alive. */
> + if (ssa_undefined_value_p (ssa_name, false))
> + return;
> +
> /* Visit each use of SSA_NAME and if it isn't in the same block as the
> def,
> add it to the list of live on entry blocks. */
> FOR_EACH_IMM_USE_FAST (use, imm_iter, ssa_name)
> {
> gimple use_stmt = USE_STMT (use);
> basic_block add_block = NULL;
>
> if (gimple_code (use_stmt) == GIMPLE_PHI)
> {
> /* Uses in PHI's are considered to be live at exit of the SRC
> block
> @@ -1422,20 +1427,25 @@ verify_live_on_entry (tree_live_info_p l
> fprintf (stderr, "\n");
> }
> else
> fprintf (stderr, " and there is no default def.\n");
> }
> }
> }
> else
> if (d == var)
> {
> + /* An undefined local variable does not need to be very
> + alive. */
> + if (ssa_undefined_value_p (var, false))
> + continue;
> +
> /* The only way this var shouldn't be marked live on entry
> is
> if it occurs in a PHI argument of the block. */
> size_t z;
> bool ok = false;
> gimple_stmt_iterator gsi;
> for (gsi = gsi_start_phis (e->dest);
> !gsi_end_p (gsi) && !ok;
> gsi_next (&gsi))
> {
> gimple phi = gsi_stmt (gsi);
> Index: gcc/tree-ssa.c
> ===================================================================
> --- gcc/tree-ssa.c (revision 216689)
> +++ gcc/tree-ssa.c (working copy)
> @@ -1178,24 +1178,25 @@ tree_ssa_useless_type_conversion (tree e
>
> tree
> tree_ssa_strip_useless_type_conversions (tree exp)
> {
> while (tree_ssa_useless_type_conversion (exp))
> exp = TREE_OPERAND (exp, 0);
> return exp;
> }
>
>
> -/* Return true if T, an SSA_NAME, has an undefined value. */
> +/* Return true if T, an SSA_NAME, has an undefined value. PARTIAL is what
> + should be returned if the value is only partially undefined. */
>
> bool
> -ssa_undefined_value_p (tree t)
> +ssa_undefined_value_p (tree t, bool partial)
> {
> gimple def_stmt;
> tree var = SSA_NAME_VAR (t);
>
> if (!var)
> ;
> /* Parameters get their initial value from the function entry. */
> else if (TREE_CODE (var) == PARM_DECL)
> return false;
> /* When returning by reference the return address is actually a hidden
> @@ -1205,21 +1206,21 @@ ssa_undefined_value_p (tree t)
> /* Hard register variables get their initial value from the ether. */
> else if (TREE_CODE (var) == VAR_DECL && DECL_HARD_REGISTER (var))
> return false;
>
> /* The value is undefined iff its definition statement is empty. */
> def_stmt = SSA_NAME_DEF_STMT (t);
> if (gimple_nop_p (def_stmt))
> return true;
>
> /* Check if the complex was not only partially defined. */
> - if (is_gimple_assign (def_stmt)
> + if (partial && is_gimple_assign (def_stmt)
> && gimple_assign_rhs_code (def_stmt) == COMPLEX_EXPR)
> {
> tree rhs1, rhs2;
>
> rhs1 = gimple_assign_rhs1 (def_stmt);
> rhs2 = gimple_assign_rhs2 (def_stmt);
> return (TREE_CODE (rhs1) == SSA_NAME && ssa_undefined_value_p (rhs1))
> || (TREE_CODE (rhs2) == SSA_NAME && ssa_undefined_value_p
> (rhs2));
> }
> return false;
> @@ -1551,32 +1552,20 @@ execute_update_addresses_taken (void)
> rhs = gimple_assign_rhs1 (stmt);
> if (gimple_assign_lhs (stmt) != lhs
> && !useless_type_conversion_p (TREE_TYPE (lhs),
> TREE_TYPE (rhs)))
> rhs = fold_build1 (VIEW_CONVERT_EXPR,
> TREE_TYPE (lhs), rhs);
>
> if (gimple_assign_lhs (stmt) != lhs)
> gimple_assign_set_lhs (stmt, lhs);
>
> - /* For var ={v} {CLOBBER}; where var lost
> - TREE_ADDRESSABLE just remove the stmt. */
> - if (DECL_P (lhs)
> - && TREE_CLOBBER_P (rhs)
> - && bitmap_bit_p (suitable_for_renaming, DECL_UID (lhs)))
> - {
> - unlink_stmt_vdef (stmt);
> - gsi_remove (&gsi, true);
> - release_defs (stmt);
> - continue;
> - }
> -
> if (gimple_assign_rhs1 (stmt) != rhs)
> {
> gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
> gimple_assign_set_rhs_from_tree (&gsi, rhs);
> }
> }
>
> else if (gimple_code (stmt) == GIMPLE_CALL)
> {
> unsigned i;
> Index: gcc/tree-ssa.h
> ===================================================================
> --- gcc/tree-ssa.h (revision 216689)
> +++ gcc/tree-ssa.h (working copy)
> @@ -44,21 +44,21 @@ extern tree target_for_debug_bind (tree)
> extern void insert_debug_temp_for_var_def (gimple_stmt_iterator *, tree);
> extern void insert_debug_temps_for_defs (gimple_stmt_iterator *);
> extern void reset_debug_uses (gimple);
> extern void release_defs_bitset (bitmap toremove);
> extern void verify_ssa (bool, bool);
> extern void init_tree_ssa (struct function *);
> extern void delete_tree_ssa (void);
> extern bool tree_ssa_useless_type_conversion (tree);
> extern tree tree_ssa_strip_useless_type_conversions (tree);
>
> -extern bool ssa_undefined_value_p (tree);
> +extern bool ssa_undefined_value_p (tree, bool = true);
> extern void execute_update_addresses_taken (void);
>
> /* Given an edge_var_map V, return the PHI arg definition. */
>
> static inline tree
> redirect_edge_var_map_def (edge_var_map *v)
> {
> return v->def;
> }
>
>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: update address taken: don't drop clobbers
2014-10-25 8:06 ` Marc Glisse
@ 2014-10-31 21:06 ` Jeff Law
0 siblings, 0 replies; 48+ messages in thread
From: Jeff Law @ 2014-10-31 21:06 UTC (permalink / raw)
To: Marc Glisse; +Cc: GCC Patches
On 10/25/14 01:48, Marc Glisse wrote:
>
> Without the liveness patch, their lifetime should mean that they don't
> coalesce with anything. But I would expect they get their own partition
> then (I am forgetting these details way too fast...). With the liveness
> patch which gives them an empty lifetime, they should all coalesce with
> at least one other ssa_name.
>
>> What happens when we expand them? I guess they get a new pseudo since
>> they're a distinct partition? If we had a sensible story for
>> expansion, then I could probably get on board with this patch.
>
> As I mentioned in my message, I don't know. Last time I looked I
> couldn't find how coalescing was actually performed. tree-outof-ssa.c
> has a function rewrite_trees with a promising comment but an empty body :-/
> I agree that we need to understand what happens at expansion time when
> the variables are not coalesced before pushing a patch that prevents
> coalescing. I was kind of hoping someone would have a pointer...
Matz changed all the out-of-ssa stuff a while back. Basically instead
of actually rewriting things to reflect going out of SSA, we just record
the partitions and refer back to the partitions representative element
during expansion.
So all we really need to do is verify reasonable partitions are built
and that should answer any questions around expansion.
> I don't really want to avoid gsi_replace, but I am willing to do it if
> it makes reviewers nervous to call such a high-level function in the
> cleanup code between passes (Richard in particular was unhappy about it).
Ah, I thought it was something you'd wanted, but sounds like it was
Richi's request.
>
> To clarify things so I know what to test and re-post, we are talking
> about the patch in
> https://gcc.gnu.org/ml/gcc-patches/2014-10/msg01787.html and we can
> forget about the coalescing thing in
> https://gcc.gnu.org/ml/gcc-patches/2014-10/msg01830.html ?
> (I'd be happy with that :-)
Yes, those are the ones I was referring to.
jeff
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: update address taken: don't drop clobbers
2014-10-25 17:14 ` Marc Glisse
2014-10-31 11:12 ` Richard Biener
@ 2014-10-31 21:16 ` Jeff Law
1 sibling, 0 replies; 48+ messages in thread
From: Jeff Law @ 2014-10-31 21:16 UTC (permalink / raw)
To: Marc Glisse; +Cc: GCC Patches
On 10/25/14 10:29, Marc Glisse wrote:
> On Fri, 24 Oct 2014, Jeff Law wrote:
>
>> I'm starting to agree -- a later message indicated you wanted to drop
>> the unlink_stmt_vdef call and you wanted to avoid gsi_replace, that
>> seems fine. I'll approve once those things are taken care of.
>
> The following passed bootstrap+testsuite. I wasn't sure what exactly a
> clobber is guaranteed not to have (no histograms for instance? At least
> I assumed it doesn't throw) so I may have kept some unnecessary calls
> when I inlined gsi_replace. I'd be happy to remove any you feel is useless.
>
> 2014-10-26 Marc Glisse <marc.glisse@inria.fr>
>
> PR tree-optimization/60770
> gcc/
> * tree-into-ssa.c: Include value-prof.h.
> (maybe_register_def): Replace clobbers with default definitions.
> * tree-ssa-live.c: Include tree-ssa.h.
> (set_var_live_on_entry): Do not mark undefined variables as live.
> (verify_live_on_entry): Do not check undefined variables.
> * tree-ssa.h (ssa_undefined_value_p): New parameter for the case
> of partially undefined variables.
> * tree-ssa.c (ssa_undefined_value_p): Likewise.
> (execute_update_addresses_taken): Do not drop clobbers.
> gcc/testsuite/
> * gcc.dg/tree-ssa/pr60770-1.c: New file.
A clobber doesn't generate any code, it's really best to think of it as
a marker. It doesn't throw, shouldn't have histrograms or anything else
of importance.
Approved.
Jeff
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: update address taken: don't drop clobbers
2014-10-31 11:12 ` Richard Biener
@ 2014-11-02 10:34 ` Marc Glisse
2014-11-03 9:06 ` Richard Biener
0 siblings, 1 reply; 48+ messages in thread
From: Marc Glisse @ 2014-11-02 10:34 UTC (permalink / raw)
To: Richard Biener; +Cc: Jeff Law, GCC Patches
[-- Attachment #1: Type: TEXT/PLAIN, Size: 7345 bytes --]
On Fri, 31 Oct 2014, Richard Biener wrote:
> On Sat, Oct 25, 2014 at 6:29 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
>> On Fri, 24 Oct 2014, Jeff Law wrote:
>>
>>> I'm starting to agree -- a later message indicated you wanted to drop the
>>> unlink_stmt_vdef call and you wanted to avoid gsi_replace, that seems fine.
>>> I'll approve once those things are taken care of.
>>
>>
>> The following passed bootstrap+testsuite. I wasn't sure what exactly a
>> clobber is guaranteed not to have (no histograms for instance? At least I
>> assumed it doesn't throw) so I may have kept some unnecessary calls when I
>> inlined gsi_replace. I'd be happy to remove any you feel is useless.
>>
>> 2014-10-26 Marc Glisse <marc.glisse@inria.fr>
>>
>> PR tree-optimization/60770
>> gcc/
>> * tree-into-ssa.c: Include value-prof.h.
>> (maybe_register_def): Replace clobbers with default definitions.
>> * tree-ssa-live.c: Include tree-ssa.h.
>> (set_var_live_on_entry): Do not mark undefined variables as live.
>> (verify_live_on_entry): Do not check undefined variables.
>> * tree-ssa.h (ssa_undefined_value_p): New parameter for the case
>> of partially undefined variables.
>> * tree-ssa.c (ssa_undefined_value_p): Likewise.
>> (execute_update_addresses_taken): Do not drop clobbers.
>>
>> gcc/testsuite/
>> * gcc.dg/tree-ssa/pr60770-1.c: New file.
>>
>> --
>> Marc Glisse
>>
>> Index: gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c
>> ===================================================================
>> --- gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c (revision 0)
>> +++ gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c (working copy)
>> @@ -0,0 +1,11 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-O -Wall" } */
>> +
>> +int f(int n){
>> + int*p;
>> + {
>> + int yyy=n;
>> + p=&yyy;
>> + }
>> + return *p; /* { dg-warning "yyy" } */
>> +}
>> Index: gcc/tree-into-ssa.c
>> ===================================================================
>> --- gcc/tree-into-ssa.c (revision 216689)
>> +++ gcc/tree-into-ssa.c (working copy)
>> @@ -52,20 +52,21 @@ along with GCC; see the file COPYING3.
>> #include "expr.h"
>> #include "tree-dfa.h"
>> #include "tree-ssa.h"
>> #include "tree-inline.h"
>> #include "tree-pass.h"
>> #include "cfgloop.h"
>> #include "domwalk.h"
>> #include "params.h"
>> #include "diagnostic-core.h"
>> #include "tree-into-ssa.h"
>> +#include "value-prof.h"
>>
>> #define PERCENT(x,y) ((float)(x) * 100.0 / (float)(y))
>>
>> /* This file builds the SSA form for a function as described in:
>> R. Cytron, J. Ferrante, B. Rosen, M. Wegman, and K. Zadeck. Efficiently
>> Computing Static Single Assignment Form and the Control Dependence
>> Graph. ACM Transactions on Programming Languages and Systems,
>> 13(4):451-490, October 1991. */
>>
>> /* Structure to map a variable VAR to the set of blocks that contain
>> @@ -1837,26 +1838,42 @@ maybe_register_def (def_operand_p def_p,
>> {
>> tree def = DEF_FROM_PTR (def_p);
>> tree sym = DECL_P (def) ? def : SSA_NAME_VAR (def);
>>
>> /* If DEF is a naked symbol that needs renaming, create a new
>> name for it. */
>> if (marked_for_renaming (sym))
>> {
>> if (DECL_P (def))
>> {
>> - tree tracked_var;
>> + if (gimple_clobber_p (stmt) && is_gimple_reg (sym))
>
> I think you know that sym is a gimple-reg as the code previously
> unconditionally generated an SSA name for it.
I checked that in July and it failed:
https://gcc.gnu.org/ml/gcc-patches/2014-07/msg01828.html
>> + {
>> + /* Replace clobber stmts with a default def. This new use of a
>> + default definition may make it look like SSA_NAMEs have
>> + conflicting lifetimes, so we need special code to let them
>> + coalesce properly. */
>> + /* Hand-inlined version of the following, for safety
>> + gsi_replace (&gsi, gimple_build_nop (), true); */
>> + gimple nop = gimple_build_nop ();
>> + gimple_set_bb (nop, gsi_bb (gsi));
>> + gimple_set_bb (stmt, NULL);
>> + gimple_remove_stmt_histograms (cfun, stmt);
>> + delink_stmt_imm_use (stmt);
>> + gsi_set_stmt (&gsi, nop);
>
> Is there any reason for this dance? I'd rather have maybe_register_def
> return a bool whether to remove the stmt... passing it down to the
> single caller of rewrite_update_stmt which can then gsi_remove the
> stmt.
For more context, my starting point was the code in rewrite_stmt, which
I was trying to port to rewrite_update_stmt (and thus
maybe_register_def):
if (gimple_clobber_p (stmt)
&& is_gimple_reg (var))
{
/* If we rewrite a DECL into SSA form then drop its
clobber stmts and replace uses with a new default def. */
gcc_checking_assert (TREE_CODE (var) == VAR_DECL
&& !gimple_vdef (stmt));
gsi_replace (si, gimple_build_nop (), true);
register_new_def (get_or_create_ssa_default_def (cfun, var), var);
break;
}
I would be happy using the same gsi_replace line, but you said that it
is dangerous because it runs update_stmt (though I don't see what bad
thing may happen when replacing a clobber by a nop), so I tried to do
the same thing as gsi_replace without update_stmt... Though now that I
re-read https://gcc.gnu.org/ml/gcc-patches/2014-07/msg01656.html it is not
clear that you are really opposed to the gsi_replace version.
I tried again with gsi_remove, trying to understand why it was failing
before, and that is because I was calling it in maybe_register_def instead
of delegating to whoever calls gsi_next.
>> - def = make_ssa_name (def, stmt);
>> + def = get_or_create_ssa_default_def (cfun, sym);
>
> I think if 'def' turns out to be a PARM_DECL this does the wrong
> thing (well, not technically wrong... but maybe unexpected). Not
> sure if we ever end up with a PARM = {} clobber though. Maybe
> guard all this with TREE_CODE (def) == VAR_DECL for extra
> safety.
Hmm, you are right. The rewrite_stmt version has
gcc_checking_assert (TREE_CODE (var) == VAR_DECL && ...
I don't remember exactly why I removed it, maybe because the second part
of the assertion was failing.
Here is a new version, that passed bootstrap+testsuite on x86_64-linux-gnu.
2014-11-03 Marc Glisse <marc.glisse@inria.fr>
PR tree-optimization/60770
gcc/
* tree-into-ssa.c (rewrite_update_stmt): Return whether the
statement should be removed.
(maybe_register_def): Likewise. Replace clobbers with default
definitions.
(rewrite_dom_walker::before_dom_children): Remove statement if
rewrite_update_stmt says so.
* tree-ssa-live.c: Include tree-ssa.h.
(set_var_live_on_entry): Do not mark undefined variables as live.
(verify_live_on_entry): Do not check undefined variables.
* tree-ssa.h (ssa_undefined_value_p): New parameter for the case
of partially undefined variables.
* tree-ssa.c (ssa_undefined_value_p): Likewise.
(execute_update_addresses_taken): Do not drop clobbers.
gcc/testsuite/
* gcc.dg/tree-ssa/pr60770-1.c: New file.
--
Marc Glisse
[-- Attachment #2: Type: TEXT/PLAIN, Size: 12495 bytes --]
Index: gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c
===================================================================
--- gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c (revision 0)
+++ gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c (working copy)
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-options "-O -Wall" } */
+
+int f(int n){
+ int*p;
+ {
+ int yyy=n;
+ p=&yyy;
+ }
+ return *p; /* { dg-warning "yyy" } */
+}
Index: gcc/tree-into-ssa.c
===================================================================
--- gcc/tree-into-ssa.c (revision 217007)
+++ gcc/tree-into-ssa.c (working copy)
@@ -1826,41 +1826,51 @@ maybe_replace_use_in_debug_stmt (use_ope
if (rdef && rdef != use)
SET_USE (use_p, rdef);
return rdef != NULL_TREE;
}
/* If the operand pointed to by DEF_P is an SSA name in NEW_SSA_NAMES
or OLD_SSA_NAMES, or if it is a symbol marked for renaming,
register it as the current definition for the names replaced by
- DEF_P. */
+ DEF_P. Returns whether the statement should be removed. */
-static inline void
+static inline bool
maybe_register_def (def_operand_p def_p, gimple stmt,
gimple_stmt_iterator gsi)
{
tree def = DEF_FROM_PTR (def_p);
tree sym = DECL_P (def) ? def : SSA_NAME_VAR (def);
+ bool to_delete = false;
/* If DEF is a naked symbol that needs renaming, create a new
name for it. */
if (marked_for_renaming (sym))
{
if (DECL_P (def))
{
- tree tracked_var;
-
- def = make_ssa_name (def, stmt);
+ if (gimple_clobber_p (stmt) && is_gimple_reg (sym))
+ {
+ gcc_checking_assert (TREE_CODE (sym) == VAR_DECL);
+ /* Replace clobber stmts with a default def. This new use of a
+ default definition may make it look like SSA_NAMEs have
+ conflicting lifetimes, so we need special code to let them
+ coalesce properly. */
+ to_delete = true;
+ def = get_or_create_ssa_default_def (cfun, sym);
+ }
+ else
+ def = make_ssa_name (def, stmt);
SET_DEF (def_p, def);
- tracked_var = target_for_debug_bind (sym);
+ tree tracked_var = target_for_debug_bind (sym);
if (tracked_var)
{
gimple note = gimple_build_debug_bind (tracked_var, def, stmt);
/* If stmt ends the bb, insert the debug stmt on the single
non-EH edge from the stmt. */
if (gsi_one_before_end_p (gsi) && stmt_ends_bb_p (stmt))
{
basic_block bb = gsi_bb (gsi);
edge_iterator ei;
edge e, ef = NULL;
@@ -1904,40 +1914,42 @@ maybe_register_def (def_operand_p def_p,
/* If DEF is a new name, register it as a new definition
for all the names replaced by DEF. */
if (is_new_name (def))
register_new_update_set (def, names_replaced_by (def));
/* If DEF is an old name, register DEF as a new
definition for itself. */
if (is_old_name (def))
register_new_update_single (def, def);
}
+
+ return to_delete;
}
/* Update every variable used in the statement pointed-to by SI. The
statement is assumed to be in SSA form already. Names in
OLD_SSA_NAMES used by SI will be updated to their current reaching
definition. Names in OLD_SSA_NAMES or NEW_SSA_NAMES defined by SI
will be registered as a new definition for their corresponding name
- in OLD_SSA_NAMES. */
+ in OLD_SSA_NAMES. Returns whether STMT should be removed. */
-static void
+static bool
rewrite_update_stmt (gimple stmt, gimple_stmt_iterator gsi)
{
use_operand_p use_p;
def_operand_p def_p;
ssa_op_iter iter;
/* Only update marked statements. */
if (!rewrite_uses_p (stmt) && !register_defs_p (stmt))
- return;
+ return false;
if (dump_file && (dump_flags & TDF_DETAILS))
{
fprintf (dump_file, "Updating SSA information for statement ");
print_gimple_stmt (dump_file, stmt, 0, TDF_SLIM);
}
/* Rewrite USES included in OLD_SSA_NAMES and USES whose underlying
symbol is marked for renaming. */
if (rewrite_uses_p (stmt))
@@ -1974,23 +1986,26 @@ rewrite_update_stmt (gimple stmt, gimple
else
{
FOR_EACH_SSA_USE_OPERAND (use_p, stmt, iter, SSA_OP_ALL_USES)
maybe_replace_use (use_p);
}
}
/* Register definitions of names in NEW_SSA_NAMES and OLD_SSA_NAMES.
Also register definitions for names whose underlying symbol is
marked for renaming. */
+ bool to_delete = false;
if (register_defs_p (stmt))
FOR_EACH_SSA_DEF_OPERAND (def_p, stmt, iter, SSA_OP_ALL_DEFS)
- maybe_register_def (def_p, stmt, gsi);
+ to_delete |= maybe_register_def (def_p, stmt, gsi);
+
+ return to_delete;
}
/* Visit all the successor blocks of BB looking for PHI nodes. For
every PHI node found, check if any of its arguments is in
OLD_SSA_NAMES. If so, and if the argument has a current reaching
definition, replace it. */
static void
rewrite_update_phi_arguments (basic_block bb)
@@ -2142,22 +2157,25 @@ rewrite_update_dom_walker::before_dom_ch
}
if (is_abnormal_phi)
SSA_NAME_OCCURS_IN_ABNORMAL_PHI (lhs) = 1;
}
/* Step 2. Rewrite every variable used in each statement in the block. */
if (bitmap_bit_p (interesting_blocks, bb->index))
{
gcc_checking_assert (bitmap_bit_p (blocks_to_update, bb->index));
- for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
- rewrite_update_stmt (gsi_stmt (gsi), gsi);
+ for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); )
+ if (rewrite_update_stmt (gsi_stmt (gsi), gsi))
+ gsi_remove (&gsi, true);
+ else
+ gsi_next (&gsi);
}
/* Step 3. Update PHI nodes. */
rewrite_update_phi_arguments (bb);
}
/* Called after visiting block BB. Unwind BLOCK_DEFS_STACK to restore
the current reaching definition of every name re-written in BB to
the original reaching definition before visiting BB. This
unwinding must be done in the opposite order to what is done in
Index: gcc/tree-ssa-live.c
===================================================================
--- gcc/tree-ssa-live.c (revision 217007)
+++ gcc/tree-ssa-live.c (working copy)
@@ -50,20 +50,21 @@ along with GCC; see the file COPYING3.
#include "stringpool.h"
#include "tree-ssanames.h"
#include "expr.h"
#include "tree-dfa.h"
#include "timevar.h"
#include "dumpfile.h"
#include "tree-ssa-live.h"
#include "diagnostic-core.h"
#include "debug.h"
#include "flags.h"
+#include "tree-ssa.h"
#ifdef ENABLE_CHECKING
static void verify_live_on_entry (tree_live_info_p);
#endif
/* VARMAP maintains a mapping from SSA version number to real variables.
All SSA_NAMES are divided into partitions. Initially each ssa_name is the
only member of it's own partition. Coalescing will attempt to group any
@@ -1096,20 +1097,24 @@ set_var_live_on_entry (tree ssa_name, tr
if (stmt)
{
def_bb = gimple_bb (stmt);
/* Mark defs in liveout bitmap temporarily. */
if (def_bb)
bitmap_set_bit (&live->liveout[def_bb->index], p);
}
else
def_bb = ENTRY_BLOCK_PTR_FOR_FN (cfun);
+ /* An undefined local variable does not need to be very alive. */
+ if (ssa_undefined_value_p (ssa_name, false))
+ return;
+
/* Visit each use of SSA_NAME and if it isn't in the same block as the def,
add it to the list of live on entry blocks. */
FOR_EACH_IMM_USE_FAST (use, imm_iter, ssa_name)
{
gimple use_stmt = USE_STMT (use);
basic_block add_block = NULL;
if (gimple_code (use_stmt) == GIMPLE_PHI)
{
/* Uses in PHI's are considered to be live at exit of the SRC block
@@ -1432,20 +1437,25 @@ verify_live_on_entry (tree_live_info_p l
fprintf (stderr, "\n");
}
else
fprintf (stderr, " and there is no default def.\n");
}
}
}
else
if (d == var)
{
+ /* An undefined local variable does not need to be very
+ alive. */
+ if (ssa_undefined_value_p (var, false))
+ continue;
+
/* The only way this var shouldn't be marked live on entry is
if it occurs in a PHI argument of the block. */
size_t z;
bool ok = false;
gimple_stmt_iterator gsi;
for (gsi = gsi_start_phis (e->dest);
!gsi_end_p (gsi) && !ok;
gsi_next (&gsi))
{
gimple phi = gsi_stmt (gsi);
Index: gcc/tree-ssa.c
===================================================================
--- gcc/tree-ssa.c (revision 217007)
+++ gcc/tree-ssa.c (working copy)
@@ -1181,24 +1181,25 @@ tree_ssa_useless_type_conversion (tree e
tree
tree_ssa_strip_useless_type_conversions (tree exp)
{
while (tree_ssa_useless_type_conversion (exp))
exp = TREE_OPERAND (exp, 0);
return exp;
}
-/* Return true if T, an SSA_NAME, has an undefined value. */
+/* Return true if T, an SSA_NAME, has an undefined value. PARTIAL is what
+ should be returned if the value is only partially undefined. */
bool
-ssa_undefined_value_p (tree t)
+ssa_undefined_value_p (tree t, bool partial)
{
gimple def_stmt;
tree var = SSA_NAME_VAR (t);
if (!var)
;
/* Parameters get their initial value from the function entry. */
else if (TREE_CODE (var) == PARM_DECL)
return false;
/* When returning by reference the return address is actually a hidden
@@ -1208,21 +1209,21 @@ ssa_undefined_value_p (tree t)
/* Hard register variables get their initial value from the ether. */
else if (TREE_CODE (var) == VAR_DECL && DECL_HARD_REGISTER (var))
return false;
/* The value is undefined iff its definition statement is empty. */
def_stmt = SSA_NAME_DEF_STMT (t);
if (gimple_nop_p (def_stmt))
return true;
/* Check if the complex was not only partially defined. */
- if (is_gimple_assign (def_stmt)
+ if (partial && is_gimple_assign (def_stmt)
&& gimple_assign_rhs_code (def_stmt) == COMPLEX_EXPR)
{
tree rhs1, rhs2;
rhs1 = gimple_assign_rhs1 (def_stmt);
rhs2 = gimple_assign_rhs2 (def_stmt);
return (TREE_CODE (rhs1) == SSA_NAME && ssa_undefined_value_p (rhs1))
|| (TREE_CODE (rhs2) == SSA_NAME && ssa_undefined_value_p (rhs2));
}
return false;
@@ -1554,32 +1555,20 @@ execute_update_addresses_taken (void)
rhs = gimple_assign_rhs1 (stmt);
if (gimple_assign_lhs (stmt) != lhs
&& !useless_type_conversion_p (TREE_TYPE (lhs),
TREE_TYPE (rhs)))
rhs = fold_build1 (VIEW_CONVERT_EXPR,
TREE_TYPE (lhs), rhs);
if (gimple_assign_lhs (stmt) != lhs)
gimple_assign_set_lhs (stmt, lhs);
- /* For var ={v} {CLOBBER}; where var lost
- TREE_ADDRESSABLE just remove the stmt. */
- if (DECL_P (lhs)
- && TREE_CLOBBER_P (rhs)
- && bitmap_bit_p (suitable_for_renaming, DECL_UID (lhs)))
- {
- unlink_stmt_vdef (stmt);
- gsi_remove (&gsi, true);
- release_defs (stmt);
- continue;
- }
-
if (gimple_assign_rhs1 (stmt) != rhs)
{
gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
gimple_assign_set_rhs_from_tree (&gsi, rhs);
}
}
else if (gimple_code (stmt) == GIMPLE_CALL)
{
unsigned i;
Index: gcc/tree-ssa.h
===================================================================
--- gcc/tree-ssa.h (revision 217007)
+++ gcc/tree-ssa.h (working copy)
@@ -44,21 +44,21 @@ extern tree target_for_debug_bind (tree)
extern void insert_debug_temp_for_var_def (gimple_stmt_iterator *, tree);
extern void insert_debug_temps_for_defs (gimple_stmt_iterator *);
extern void reset_debug_uses (gimple);
extern void release_defs_bitset (bitmap toremove);
extern void verify_ssa (bool, bool);
extern void init_tree_ssa (struct function *);
extern void delete_tree_ssa (void);
extern bool tree_ssa_useless_type_conversion (tree);
extern tree tree_ssa_strip_useless_type_conversions (tree);
-extern bool ssa_undefined_value_p (tree);
+extern bool ssa_undefined_value_p (tree, bool = true);
extern void execute_update_addresses_taken (void);
/* Given an edge_var_map V, return the PHI arg definition. */
static inline tree
redirect_edge_var_map_def (edge_var_map *v)
{
return v->def;
}
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: update address taken: don't drop clobbers
2014-11-02 10:34 ` Marc Glisse
@ 2014-11-03 9:06 ` Richard Biener
0 siblings, 0 replies; 48+ messages in thread
From: Richard Biener @ 2014-11-03 9:06 UTC (permalink / raw)
To: Marc Glisse; +Cc: Jeff Law, GCC Patches
On Sun, Nov 2, 2014 at 11:34 AM, Marc Glisse <marc.glisse@inria.fr> wrote:
> On Fri, 31 Oct 2014, Richard Biener wrote:
>
>> On Sat, Oct 25, 2014 at 6:29 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
>>>
>>> On Fri, 24 Oct 2014, Jeff Law wrote:
>>>
>>>> I'm starting to agree -- a later message indicated you wanted to drop
>>>> the
>>>> unlink_stmt_vdef call and you wanted to avoid gsi_replace, that seems
>>>> fine.
>>>> I'll approve once those things are taken care of.
>>>
>>>
>>>
>>> The following passed bootstrap+testsuite. I wasn't sure what exactly a
>>> clobber is guaranteed not to have (no histograms for instance? At least I
>>> assumed it doesn't throw) so I may have kept some unnecessary calls when
>>> I
>>> inlined gsi_replace. I'd be happy to remove any you feel is useless.
>>>
>>> 2014-10-26 Marc Glisse <marc.glisse@inria.fr>
>>>
>>> PR tree-optimization/60770
>>> gcc/
>>> * tree-into-ssa.c: Include value-prof.h.
>>> (maybe_register_def): Replace clobbers with default definitions.
>>> * tree-ssa-live.c: Include tree-ssa.h.
>>> (set_var_live_on_entry): Do not mark undefined variables as live.
>>> (verify_live_on_entry): Do not check undefined variables.
>>> * tree-ssa.h (ssa_undefined_value_p): New parameter for the case
>>> of partially undefined variables.
>>> * tree-ssa.c (ssa_undefined_value_p): Likewise.
>>> (execute_update_addresses_taken): Do not drop clobbers.
>>>
>>> gcc/testsuite/
>>> * gcc.dg/tree-ssa/pr60770-1.c: New file.
>>>
>>> --
>>> Marc Glisse
>>>
>>> Index: gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c
>>> ===================================================================
>>> --- gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c (revision 0)
>>> +++ gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c (working copy)
>>> @@ -0,0 +1,11 @@
>>> +/* { dg-do compile } */
>>> +/* { dg-options "-O -Wall" } */
>>> +
>>> +int f(int n){
>>> + int*p;
>>> + {
>>> + int yyy=n;
>>> + p=&yyy;
>>> + }
>>> + return *p; /* { dg-warning "yyy" } */
>>> +}
>>> Index: gcc/tree-into-ssa.c
>>> ===================================================================
>>> --- gcc/tree-into-ssa.c (revision 216689)
>>> +++ gcc/tree-into-ssa.c (working copy)
>>> @@ -52,20 +52,21 @@ along with GCC; see the file COPYING3.
>>> #include "expr.h"
>>> #include "tree-dfa.h"
>>> #include "tree-ssa.h"
>>> #include "tree-inline.h"
>>> #include "tree-pass.h"
>>> #include "cfgloop.h"
>>> #include "domwalk.h"
>>> #include "params.h"
>>> #include "diagnostic-core.h"
>>> #include "tree-into-ssa.h"
>>> +#include "value-prof.h"
>>>
>>> #define PERCENT(x,y) ((float)(x) * 100.0 / (float)(y))
>>>
>>> /* This file builds the SSA form for a function as described in:
>>> R. Cytron, J. Ferrante, B. Rosen, M. Wegman, and K. Zadeck.
>>> Efficiently
>>> Computing Static Single Assignment Form and the Control Dependence
>>> Graph. ACM Transactions on Programming Languages and Systems,
>>> 13(4):451-490, October 1991. */
>>>
>>> /* Structure to map a variable VAR to the set of blocks that contain
>>> @@ -1837,26 +1838,42 @@ maybe_register_def (def_operand_p def_p,
>>> {
>>> tree def = DEF_FROM_PTR (def_p);
>>> tree sym = DECL_P (def) ? def : SSA_NAME_VAR (def);
>>>
>>> /* If DEF is a naked symbol that needs renaming, create a new
>>> name for it. */
>>> if (marked_for_renaming (sym))
>>> {
>>> if (DECL_P (def))
>>> {
>>> - tree tracked_var;
>>> + if (gimple_clobber_p (stmt) && is_gimple_reg (sym))
>>
>>
>> I think you know that sym is a gimple-reg as the code previously
>> unconditionally generated an SSA name for it.
>
>
> I checked that in July and it failed:
> https://gcc.gnu.org/ml/gcc-patches/2014-07/msg01828.html
>
>>> + {
>>> + /* Replace clobber stmts with a default def. This new use
>>> of a
>>> + default definition may make it look like SSA_NAMEs have
>>> + conflicting lifetimes, so we need special code to let
>>> them
>>> + coalesce properly. */
>>> + /* Hand-inlined version of the following, for safety
>>> + gsi_replace (&gsi, gimple_build_nop (), true); */
>>> + gimple nop = gimple_build_nop ();
>>> + gimple_set_bb (nop, gsi_bb (gsi));
>>> + gimple_set_bb (stmt, NULL);
>>> + gimple_remove_stmt_histograms (cfun, stmt);
>>> + delink_stmt_imm_use (stmt);
>>> + gsi_set_stmt (&gsi, nop);
>>
>>
>> Is there any reason for this dance? I'd rather have maybe_register_def
>> return a bool whether to remove the stmt... passing it down to the
>> single caller of rewrite_update_stmt which can then gsi_remove the
>> stmt.
>
>
> For more context, my starting point was the code in rewrite_stmt, which
> I was trying to port to rewrite_update_stmt (and thus
> maybe_register_def):
>
> if (gimple_clobber_p (stmt)
> && is_gimple_reg (var))
> {
> /* If we rewrite a DECL into SSA form then drop its
> clobber stmts and replace uses with a new default def. */
> gcc_checking_assert (TREE_CODE (var) == VAR_DECL
> && !gimple_vdef (stmt));
> gsi_replace (si, gimple_build_nop (), true);
> register_new_def (get_or_create_ssa_default_def (cfun, var),
> var);
> break;
> }
>
> I would be happy using the same gsi_replace line, but you said that it
> is dangerous because it runs update_stmt (though I don't see what bad
> thing may happen when replacing a clobber by a nop), so I tried to do
> the same thing as gsi_replace without update_stmt... Though now that I
> re-read https://gcc.gnu.org/ml/gcc-patches/2014-07/msg01656.html it is not
> clear that you are really opposed to the gsi_replace version.
>
> I tried again with gsi_remove, trying to understand why it was failing
> before, and that is because I was calling it in maybe_register_def instead
> of delegating to whoever calls gsi_next.
>
>>> - def = make_ssa_name (def, stmt);
>>> + def = get_or_create_ssa_default_def (cfun, sym);
>>
>>
>> I think if 'def' turns out to be a PARM_DECL this does the wrong
>> thing (well, not technically wrong... but maybe unexpected). Not
>> sure if we ever end up with a PARM = {} clobber though. Maybe
>> guard all this with TREE_CODE (def) == VAR_DECL for extra
>> safety.
>
>
> Hmm, you are right. The rewrite_stmt version has
>
> gcc_checking_assert (TREE_CODE (var) == VAR_DECL && ...
>
> I don't remember exactly why I removed it, maybe because the second part of
> the assertion was failing.
>
> Here is a new version, that passed bootstrap+testsuite on x86_64-linux-gnu.
Ok.
Thanks,
Richard.
> 2014-11-03 Marc Glisse <marc.glisse@inria.fr>
>
> PR tree-optimization/60770
> gcc/
> * tree-into-ssa.c (rewrite_update_stmt): Return whether the
> statement should be removed.
> (maybe_register_def): Likewise. Replace clobbers with default
> definitions.
> (rewrite_dom_walker::before_dom_children): Remove statement if
> rewrite_update_stmt says so.
>
> * tree-ssa-live.c: Include tree-ssa.h.
> (set_var_live_on_entry): Do not mark undefined variables as live.
> (verify_live_on_entry): Do not check undefined variables.
> * tree-ssa.h (ssa_undefined_value_p): New parameter for the case
> of partially undefined variables.
> * tree-ssa.c (ssa_undefined_value_p): Likewise.
> (execute_update_addresses_taken): Do not drop clobbers.
>
> gcc/testsuite/
> * gcc.dg/tree-ssa/pr60770-1.c: New file.
>
> --
> Marc Glisse
>
> Index: gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c (revision 0)
> +++ gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c (working copy)
> @@ -0,0 +1,11 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O -Wall" } */
> +
> +int f(int n){
> + int*p;
> + {
> + int yyy=n;
> + p=&yyy;
> + }
> + return *p; /* { dg-warning "yyy" } */
> +}
> Index: gcc/tree-into-ssa.c
> ===================================================================
> --- gcc/tree-into-ssa.c (revision 217007)
> +++ gcc/tree-into-ssa.c (working copy)
> @@ -1826,41 +1826,51 @@ maybe_replace_use_in_debug_stmt (use_ope
> if (rdef && rdef != use)
> SET_USE (use_p, rdef);
>
> return rdef != NULL_TREE;
> }
>
>
> /* If the operand pointed to by DEF_P is an SSA name in NEW_SSA_NAMES
> or OLD_SSA_NAMES, or if it is a symbol marked for renaming,
> register it as the current definition for the names replaced by
> - DEF_P. */
> + DEF_P. Returns whether the statement should be removed. */
>
> -static inline void
> +static inline bool
> maybe_register_def (def_operand_p def_p, gimple stmt,
> gimple_stmt_iterator gsi)
> {
> tree def = DEF_FROM_PTR (def_p);
> tree sym = DECL_P (def) ? def : SSA_NAME_VAR (def);
> + bool to_delete = false;
>
> /* If DEF is a naked symbol that needs renaming, create a new
> name for it. */
> if (marked_for_renaming (sym))
> {
> if (DECL_P (def))
> {
> - tree tracked_var;
> -
> - def = make_ssa_name (def, stmt);
> + if (gimple_clobber_p (stmt) && is_gimple_reg (sym))
> + {
> + gcc_checking_assert (TREE_CODE (sym) == VAR_DECL);
> + /* Replace clobber stmts with a default def. This new use of a
> + default definition may make it look like SSA_NAMEs have
> + conflicting lifetimes, so we need special code to let them
> + coalesce properly. */
> + to_delete = true;
> + def = get_or_create_ssa_default_def (cfun, sym);
> + }
> + else
> + def = make_ssa_name (def, stmt);
> SET_DEF (def_p, def);
>
> - tracked_var = target_for_debug_bind (sym);
> + tree tracked_var = target_for_debug_bind (sym);
> if (tracked_var)
> {
> gimple note = gimple_build_debug_bind (tracked_var, def,
> stmt);
> /* If stmt ends the bb, insert the debug stmt on the single
> non-EH edge from the stmt. */
> if (gsi_one_before_end_p (gsi) && stmt_ends_bb_p (stmt))
> {
> basic_block bb = gsi_bb (gsi);
> edge_iterator ei;
> edge e, ef = NULL;
> @@ -1904,40 +1914,42 @@ maybe_register_def (def_operand_p def_p,
> /* If DEF is a new name, register it as a new definition
> for all the names replaced by DEF. */
> if (is_new_name (def))
> register_new_update_set (def, names_replaced_by (def));
>
> /* If DEF is an old name, register DEF as a new
> definition for itself. */
> if (is_old_name (def))
> register_new_update_single (def, def);
> }
> +
> + return to_delete;
> }
>
>
> /* Update every variable used in the statement pointed-to by SI. The
> statement is assumed to be in SSA form already. Names in
> OLD_SSA_NAMES used by SI will be updated to their current reaching
> definition. Names in OLD_SSA_NAMES or NEW_SSA_NAMES defined by SI
> will be registered as a new definition for their corresponding name
> - in OLD_SSA_NAMES. */
> + in OLD_SSA_NAMES. Returns whether STMT should be removed. */
>
> -static void
> +static bool
> rewrite_update_stmt (gimple stmt, gimple_stmt_iterator gsi)
> {
> use_operand_p use_p;
> def_operand_p def_p;
> ssa_op_iter iter;
>
> /* Only update marked statements. */
> if (!rewrite_uses_p (stmt) && !register_defs_p (stmt))
> - return;
> + return false;
>
> if (dump_file && (dump_flags & TDF_DETAILS))
> {
> fprintf (dump_file, "Updating SSA information for statement ");
> print_gimple_stmt (dump_file, stmt, 0, TDF_SLIM);
> }
>
> /* Rewrite USES included in OLD_SSA_NAMES and USES whose underlying
> symbol is marked for renaming. */
> if (rewrite_uses_p (stmt))
> @@ -1974,23 +1986,26 @@ rewrite_update_stmt (gimple stmt, gimple
> else
> {
> FOR_EACH_SSA_USE_OPERAND (use_p, stmt, iter, SSA_OP_ALL_USES)
> maybe_replace_use (use_p);
> }
> }
>
> /* Register definitions of names in NEW_SSA_NAMES and OLD_SSA_NAMES.
> Also register definitions for names whose underlying symbol is
> marked for renaming. */
> + bool to_delete = false;
> if (register_defs_p (stmt))
> FOR_EACH_SSA_DEF_OPERAND (def_p, stmt, iter, SSA_OP_ALL_DEFS)
> - maybe_register_def (def_p, stmt, gsi);
> + to_delete |= maybe_register_def (def_p, stmt, gsi);
> +
> + return to_delete;
> }
>
>
> /* Visit all the successor blocks of BB looking for PHI nodes. For
> every PHI node found, check if any of its arguments is in
> OLD_SSA_NAMES. If so, and if the argument has a current reaching
> definition, replace it. */
>
> static void
> rewrite_update_phi_arguments (basic_block bb)
> @@ -2142,22 +2157,25 @@ rewrite_update_dom_walker::before_dom_ch
> }
>
> if (is_abnormal_phi)
> SSA_NAME_OCCURS_IN_ABNORMAL_PHI (lhs) = 1;
> }
>
> /* Step 2. Rewrite every variable used in each statement in the block.
> */
> if (bitmap_bit_p (interesting_blocks, bb->index))
> {
> gcc_checking_assert (bitmap_bit_p (blocks_to_update, bb->index));
> - for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> - rewrite_update_stmt (gsi_stmt (gsi), gsi);
> + for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); )
> + if (rewrite_update_stmt (gsi_stmt (gsi), gsi))
> + gsi_remove (&gsi, true);
> + else
> + gsi_next (&gsi);
> }
>
> /* Step 3. Update PHI nodes. */
> rewrite_update_phi_arguments (bb);
> }
>
> /* Called after visiting block BB. Unwind BLOCK_DEFS_STACK to restore
> the current reaching definition of every name re-written in BB to
> the original reaching definition before visiting BB. This
> unwinding must be done in the opposite order to what is done in
> Index: gcc/tree-ssa-live.c
> ===================================================================
> --- gcc/tree-ssa-live.c (revision 217007)
> +++ gcc/tree-ssa-live.c (working copy)
> @@ -50,20 +50,21 @@ along with GCC; see the file COPYING3.
> #include "stringpool.h"
> #include "tree-ssanames.h"
> #include "expr.h"
> #include "tree-dfa.h"
> #include "timevar.h"
> #include "dumpfile.h"
> #include "tree-ssa-live.h"
> #include "diagnostic-core.h"
> #include "debug.h"
> #include "flags.h"
> +#include "tree-ssa.h"
>
> #ifdef ENABLE_CHECKING
> static void verify_live_on_entry (tree_live_info_p);
> #endif
>
>
> /* VARMAP maintains a mapping from SSA version number to real variables.
>
> All SSA_NAMES are divided into partitions. Initially each ssa_name is
> the
> only member of it's own partition. Coalescing will attempt to group any
> @@ -1096,20 +1097,24 @@ set_var_live_on_entry (tree ssa_name, tr
> if (stmt)
> {
> def_bb = gimple_bb (stmt);
> /* Mark defs in liveout bitmap temporarily. */
> if (def_bb)
> bitmap_set_bit (&live->liveout[def_bb->index], p);
> }
> else
> def_bb = ENTRY_BLOCK_PTR_FOR_FN (cfun);
>
> + /* An undefined local variable does not need to be very alive. */
> + if (ssa_undefined_value_p (ssa_name, false))
> + return;
> +
> /* Visit each use of SSA_NAME and if it isn't in the same block as the
> def,
> add it to the list of live on entry blocks. */
> FOR_EACH_IMM_USE_FAST (use, imm_iter, ssa_name)
> {
> gimple use_stmt = USE_STMT (use);
> basic_block add_block = NULL;
>
> if (gimple_code (use_stmt) == GIMPLE_PHI)
> {
> /* Uses in PHI's are considered to be live at exit of the SRC
> block
> @@ -1432,20 +1437,25 @@ verify_live_on_entry (tree_live_info_p l
> fprintf (stderr, "\n");
> }
> else
> fprintf (stderr, " and there is no default def.\n");
> }
> }
> }
> else
> if (d == var)
> {
> + /* An undefined local variable does not need to be very
> + alive. */
> + if (ssa_undefined_value_p (var, false))
> + continue;
> +
> /* The only way this var shouldn't be marked live on entry
> is
> if it occurs in a PHI argument of the block. */
> size_t z;
> bool ok = false;
> gimple_stmt_iterator gsi;
> for (gsi = gsi_start_phis (e->dest);
> !gsi_end_p (gsi) && !ok;
> gsi_next (&gsi))
> {
> gimple phi = gsi_stmt (gsi);
> Index: gcc/tree-ssa.c
> ===================================================================
> --- gcc/tree-ssa.c (revision 217007)
> +++ gcc/tree-ssa.c (working copy)
> @@ -1181,24 +1181,25 @@ tree_ssa_useless_type_conversion (tree e
>
> tree
> tree_ssa_strip_useless_type_conversions (tree exp)
> {
> while (tree_ssa_useless_type_conversion (exp))
> exp = TREE_OPERAND (exp, 0);
> return exp;
> }
>
>
> -/* Return true if T, an SSA_NAME, has an undefined value. */
> +/* Return true if T, an SSA_NAME, has an undefined value. PARTIAL is what
> + should be returned if the value is only partially undefined. */
>
> bool
> -ssa_undefined_value_p (tree t)
> +ssa_undefined_value_p (tree t, bool partial)
> {
> gimple def_stmt;
> tree var = SSA_NAME_VAR (t);
>
> if (!var)
> ;
> /* Parameters get their initial value from the function entry. */
> else if (TREE_CODE (var) == PARM_DECL)
> return false;
> /* When returning by reference the return address is actually a hidden
> @@ -1208,21 +1209,21 @@ ssa_undefined_value_p (tree t)
> /* Hard register variables get their initial value from the ether. */
> else if (TREE_CODE (var) == VAR_DECL && DECL_HARD_REGISTER (var))
> return false;
>
> /* The value is undefined iff its definition statement is empty. */
> def_stmt = SSA_NAME_DEF_STMT (t);
> if (gimple_nop_p (def_stmt))
> return true;
>
> /* Check if the complex was not only partially defined. */
> - if (is_gimple_assign (def_stmt)
> + if (partial && is_gimple_assign (def_stmt)
> && gimple_assign_rhs_code (def_stmt) == COMPLEX_EXPR)
> {
> tree rhs1, rhs2;
>
> rhs1 = gimple_assign_rhs1 (def_stmt);
> rhs2 = gimple_assign_rhs2 (def_stmt);
> return (TREE_CODE (rhs1) == SSA_NAME && ssa_undefined_value_p (rhs1))
> || (TREE_CODE (rhs2) == SSA_NAME && ssa_undefined_value_p
> (rhs2));
> }
> return false;
> @@ -1554,32 +1555,20 @@ execute_update_addresses_taken (void)
> rhs = gimple_assign_rhs1 (stmt);
> if (gimple_assign_lhs (stmt) != lhs
> && !useless_type_conversion_p (TREE_TYPE (lhs),
> TREE_TYPE (rhs)))
> rhs = fold_build1 (VIEW_CONVERT_EXPR,
> TREE_TYPE (lhs), rhs);
>
> if (gimple_assign_lhs (stmt) != lhs)
> gimple_assign_set_lhs (stmt, lhs);
>
> - /* For var ={v} {CLOBBER}; where var lost
> - TREE_ADDRESSABLE just remove the stmt. */
> - if (DECL_P (lhs)
> - && TREE_CLOBBER_P (rhs)
> - && bitmap_bit_p (suitable_for_renaming, DECL_UID (lhs)))
> - {
> - unlink_stmt_vdef (stmt);
> - gsi_remove (&gsi, true);
> - release_defs (stmt);
> - continue;
> - }
> -
> if (gimple_assign_rhs1 (stmt) != rhs)
> {
> gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
> gimple_assign_set_rhs_from_tree (&gsi, rhs);
> }
> }
>
> else if (gimple_code (stmt) == GIMPLE_CALL)
> {
> unsigned i;
> Index: gcc/tree-ssa.h
> ===================================================================
> --- gcc/tree-ssa.h (revision 217007)
> +++ gcc/tree-ssa.h (working copy)
> @@ -44,21 +44,21 @@ extern tree target_for_debug_bind (tree)
> extern void insert_debug_temp_for_var_def (gimple_stmt_iterator *, tree);
> extern void insert_debug_temps_for_defs (gimple_stmt_iterator *);
> extern void reset_debug_uses (gimple);
> extern void release_defs_bitset (bitmap toremove);
> extern void verify_ssa (bool, bool);
> extern void init_tree_ssa (struct function *);
> extern void delete_tree_ssa (void);
> extern bool tree_ssa_useless_type_conversion (tree);
> extern tree tree_ssa_strip_useless_type_conversions (tree);
>
> -extern bool ssa_undefined_value_p (tree);
> +extern bool ssa_undefined_value_p (tree, bool = true);
> extern void execute_update_addresses_taken (void);
>
> /* Given an edge_var_map V, return the PHI arg definition. */
>
> static inline tree
> redirect_edge_var_map_def (edge_var_map *v)
> {
> return v->def;
> }
>
>
^ permalink raw reply [flat|nested] 48+ messages in thread
end of thread, other threads:[~2014-11-03 9:06 UTC | newest]
Thread overview: 48+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-06-28 22:33 update address taken: don't drop clobbers Marc Glisse
2014-06-29 23:38 ` SRA: " Marc Glisse
2014-07-07 8:56 ` Richard Biener
2014-07-07 9:32 ` Marc Glisse
2014-07-07 18:32 ` Richard Biener
2014-07-07 20:15 ` Marc Glisse
2014-07-07 16:59 ` Jeff Law
2014-07-10 14:55 ` Richard Biener
2014-07-10 15:01 ` Jakub Jelinek
2014-06-30 19:31 ` update address taken: " Jeff Law
2014-07-06 14:24 ` Marc Glisse
2014-07-06 14:54 ` pinskia
2014-07-06 15:01 ` Marc Glisse
2014-07-07 10:21 ` Richard Biener
2014-07-07 17:20 ` Jeff Law
2014-07-08 13:31 ` Marc Glisse
2014-07-10 15:10 ` Richard Biener
2014-07-10 15:49 ` Michael Matz
2014-07-10 18:23 ` Jeff Law
2014-07-11 8:10 ` Richard Biener
2014-07-11 8:14 ` Richard Biener
2014-07-11 12:06 ` Michael Matz
2014-07-11 17:16 ` Jeff Law
2014-07-12 6:15 ` Marc Glisse
2014-07-24 13:06 ` Richard Biener
2014-07-27 11:53 ` Marc Glisse
2014-07-27 18:01 ` Marc Glisse
2014-09-07 15:28 ` Marc Glisse
2014-10-15 14:36 ` Marc Glisse
2014-10-15 16:12 ` Jeff Law
2014-10-16 11:20 ` Richard Biener
2014-10-16 14:11 ` Marc Glisse
2014-10-16 14:34 ` Richard Biener
2014-10-16 17:29 ` Jeff Law
2014-10-16 17:58 ` Richard Biener
2014-10-16 18:37 ` Jeff Law
2014-10-17 20:46 ` Marc Glisse
2014-10-24 20:22 ` Jeff Law
2014-10-25 8:06 ` Marc Glisse
2014-10-31 21:06 ` Jeff Law
2014-10-25 17:14 ` Marc Glisse
2014-10-31 11:12 ` Richard Biener
2014-11-02 10:34 ` Marc Glisse
2014-11-03 9:06 ` Richard Biener
2014-10-31 21:16 ` Jeff Law
2014-10-16 17:23 ` Jeff Law
2014-10-18 22:23 ` Marc Glisse
2014-10-24 20:19 ` Jeff Law
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).