On Mon, 19 May 2014, Richard Biener wrote:
> On May 19, 2014 6:57:52 PM CEST, Jeff Law <l...@redhat.com> wrote:
> >On 05/19/14 06:54, Richard Biener wrote:
> >>
> >> In this PR we run into the issue that releasing SSA names from
> >> FRE/PRE elimination corrupts the VN lattice and thus the VN lookup
> >> we perform for removing redudnant stores ICEs. The patch works
> >> around the particular case by making unreachable code detection
> >> in SCCVN more optimistic by ignoring backedges during reachability
> >> computation and by not doing any elimination on unreachable blocks.
> >>
> >> I still have to think about a "proper" fix to the underlying issue
> >> (propagate_value_into_stmt updating stmts on our back).
> >>
> >> Bootstrap and regtest running on x86_64-unknown-linux-gnu.
> >>
> >> Richard.
> >>
> >> 2014-05-19 Richard Biener <rguent...@suse.de>
> >>
> >> PR tree-optimization/61221
> >> * tree-ssa-pre.c (eliminate_dom_walker::before_dom_children):
> >> Do nothing for unreachable blocks.
> >> * tree-ssa-sccvn.c (cond_dom_walker::before_dom_children):
> >> Improve unreachability detection.
> >>
> >> * gcc.dg/torture/pr61221.c: New testcase.
> >Is this another case where having two lists rather than just one in the
> >
> >SSA_NAME manager would help? It's something I really need to sit down
> >and just do.
>
> No, it's unfortunately not that easy.
Fixed with the following. The issue is that while we avoid to
release any SSA names during FRE/PRE eliminate
propagate_tree_value_into_stmt still will do that behind our
backs very deep rooted inside a gsi_replace call. This will
release virtual operands of a call replaced by a copy
(also detaching the defining call stmt from the IL), disrupting
the SCCVN lattice and making the alias walk done during VN
lookup for redundant stores (during that very same eliminate)
crash (it will also need to look at the defining stmts basic-block,
so simply only not releasing SSA names isn't enough to fix that - we
also may not remove any stmts).
The following implements a machinery for dealing with released
virtual SSA names in the VN machinery to mitigate this, also
removing the need for delayed stmt updating during eliminate.
We're still going to have problems if SSA names are re-used
(as even released SSA names have to remain valid lattice
entries after this). But currently nothing will create new
SSA names (fingers crossing) during eliminate. A followup
patch of mine will though, thus I'll do that two-staged
SSA name release/reuse list you thought would maybe fix this
issue.
Bootstrapped and tested on x86_64-unknown-linux-gnu, applied to trunk.
Richard.
2014-05-20 Richard Biener <rguent...@suse.de>
PR tree-optimization/61221
* tree-ssa-pre.c (el_to_update): Remove.
(eliminate_dom_walker::before_dom_children): Handle released
VDEFs by value-numbering them to the associated VUSE. Update
stmt immediately for substituted call address.
(eliminate): Remove delayed stmt updating code.
* tree-ssa-sccvn.c (vuse_ssa_val): New function valueizing
possibly late re-numbered vuses.
(vn_reference_lookup_2): Adjust.
(vn_reference_lookup_pieces): Likewise.
(vn_reference_lookup): Likewise.
Index: gcc/tree-ssa-pre.c
===================================================================
*** gcc/tree-ssa-pre.c (revision 210611)
--- gcc/tree-ssa-pre.c (working copy)
*************** compute_avail (void)
*** 3915,3921 ****
/* Local state for the eliminate domwalk. */
static vec<gimple> el_to_remove;
- static vec<gimple> el_to_update;
static unsigned int el_todo;
static vec<tree> el_avail;
static vec<tree> el_avail_stack;
--- 3915,3920 ----
*************** eliminate_dom_walker::before_dom_childre
*** 4146,4154 ****
--- 4145,4158 ----
print_gimple_stmt (dump_file, stmt, 0, 0);
}
pre_stats.eliminations++;
+
+ tree vdef = gimple_vdef (stmt);
+ tree vuse = gimple_vuse (stmt);
propagate_tree_value_into_stmt (&gsi, sprime);
stmt = gsi_stmt (gsi);
update_stmt (stmt);
+ if (vdef != gimple_vdef (stmt))
+ VN_INFO (vdef)->valnum = vuse;
/* If we removed EH side-effects from the statement, clean
its EH information. */
*************** eliminate_dom_walker::before_dom_childre
*** 4246,4254 ****
--- 4250,4263 ----
sprime = fold_convert (gimple_expr_type (stmt), sprime);
pre_stats.eliminations++;
+
+ tree vdef = gimple_vdef (stmt);
+ tree vuse = gimple_vuse (stmt);
propagate_tree_value_into_stmt (&gsi, sprime);
stmt = gsi_stmt (gsi);
update_stmt (stmt);
+ if (vdef != gimple_vdef (stmt))
+ VN_INFO (vdef)->valnum = vuse;
/* If we removed EH side-effects from the statement, clean
its EH information. */
*************** eliminate_dom_walker::before_dom_childre
*** 4362,4368 ****
}
gimple_call_set_fn (stmt, fn);
! el_to_update.safe_push (stmt);
/* When changing a call into a noreturn call, cfg cleanup
is needed to fix up the noreturn call. */
--- 4371,4381 ----
}
gimple_call_set_fn (stmt, fn);
! tree vdef = gimple_vdef (stmt);
! tree vuse = gimple_vuse (stmt);
! update_stmt (stmt);
! if (vdef != gimple_vdef (stmt))
! VN_INFO (vdef)->valnum = vuse;
/* When changing a call into a noreturn call, cfg cleanup
is needed to fix up the noreturn call. */
*************** eliminate (void)
*** 4421,4427 ****
need_ab_cleanup = BITMAP_ALLOC (NULL);
el_to_remove.create (0);
- el_to_update.create (0);
el_todo = 0;
el_avail.create (0);
el_avail_stack.create (0);
--- 4434,4439 ----
*************** eliminate (void)
*** 4473,4485 ****
}
el_to_remove.release ();
- /* We cannot update call statements with virtual operands during
- SSA walk. This might remove them which in turn makes our
- VN lattice invalid. */
- FOR_EACH_VEC_ELT (el_to_update, i, stmt)
- update_stmt (stmt);
- el_to_update.release ();
-
return el_todo;
}
--- 4485,4490 ----
Index: gcc/tree-ssa-sccvn.c
===================================================================
*** gcc/tree-ssa-sccvn.c (revision 210611)
--- gcc/tree-ssa-sccvn.c (working copy)
*************** static int *rpo_numbers;
*** 318,323 ****
--- 318,342 ----
#define SSA_VAL(x) (VN_INFO ((x))->valnum)
+ /* Return the SSA value of the VUSE x, supporting released VDEFs
+ during elimination which will value-number the VDEF to the
+ associated VUSE (but not substitute in the whole lattice). */
+
+ static inline tree
+ vuse_ssa_val (tree x)
+ {
+ if (!x)
+ return NULL_TREE;
+
+ do
+ {
+ x = SSA_VAL (x);
+ }
+ while (SSA_NAME_IN_FREE_LIST (x));
+
+ return x;
+ }
+
/* This represents the top of the VN lattice, which is the universal
value. */
*************** vn_reference_lookup_2 (ao_ref *op ATTRIB
*** 1495,1501 ****
/* Fixup vuse and hash. */
if (vr->vuse)
vr->hashcode = vr->hashcode - SSA_NAME_VERSION (vr->vuse);
! vr->vuse = SSA_VAL (vuse);
if (vr->vuse)
vr->hashcode = vr->hashcode + SSA_NAME_VERSION (vr->vuse);
--- 1514,1520 ----
/* Fixup vuse and hash. */
if (vr->vuse)
vr->hashcode = vr->hashcode - SSA_NAME_VERSION (vr->vuse);
! vr->vuse = vuse_ssa_val (vuse);
if (vr->vuse)
vr->hashcode = vr->hashcode + SSA_NAME_VERSION (vr->vuse);
*************** vn_reference_lookup_pieces (tree vuse, a
*** 2035,2041 ****
vnresult = &tmp;
*vnresult = NULL;
! vr1.vuse = vuse ? SSA_VAL (vuse) : NULL_TREE;
shared_lookup_references.truncate (0);
shared_lookup_references.safe_grow (operands.length ());
memcpy (shared_lookup_references.address (),
--- 2054,2060 ----
vnresult = &tmp;
*vnresult = NULL;
! vr1.vuse = vuse_ssa_val (vuse);
shared_lookup_references.truncate (0);
shared_lookup_references.safe_grow (operands.length ());
memcpy (shared_lookup_references.address (),
*************** vn_reference_lookup (tree op, tree vuse,
*** 2090,2096 ****
if (vnresult)
*vnresult = NULL;
! vr1.vuse = vuse ? SSA_VAL (vuse) : NULL_TREE;
vr1.operands = operands
= valueize_shared_reference_ops_from_ref (op, &valuezied_anything);
vr1.type = TREE_TYPE (op);
--- 2109,2115 ----
if (vnresult)
*vnresult = NULL;
! vr1.vuse = vuse_ssa_val (vuse);
vr1.operands = operands
= valueize_shared_reference_ops_from_ref (op, &valuezied_anything);
vr1.type = TREE_TYPE (op);
