On Tue, 2 Apr 2013, Jakub Jelinek wrote:
> On Tue, Apr 02, 2013 at 01:44:46PM +0200, Richard Biener wrote:
> > > @@ -412,7 +412,8 @@ get_mem_ref_of_assignment (const gimple
> > > {
> > > gcc_assert (gimple_assign_single_p (assignment));
> > >
> > > - if (gimple_store_p (assignment))
> > > + if (gimple_store_p (assignment)
> > > + && !gimple_clobber_p (assignment))
> >
> > hmm, maybe gimple_store_p should not consider a clobber a store?
>
> gimple_store_p is young, so perhaps we can tweak its meaning.
> By changing gimple_store_p, we could drop this hunk, but perhaps
> would want to add early return for gimple_clobber_p
> into will_be_nonconstant_predicate? Your call.
I've just come along 56787 and decided that not collecting
CLOBBERs during data-reference gathering would be wrong.
Thus changing gimple_store_p would be wrong, too. So in the
end the above hunk looks ok and we shouldn't change gimple_store_p.
(well, for now at least)
> > > {
> > > ref->start = gimple_assign_lhs (assignment);
> > > *ref_is_store = true;
> > > --- gcc/tree-ssa-dse.c.jj 2013-01-11 09:02:37.000000000 +0100
> > > +++ gcc/tree-ssa-dse.c 2013-03-27 17:14:27.424466023 +0100
> > > @@ -218,7 +218,10 @@ dse_optimize_stmt (gimple_stmt_iterator
> > > if (is_gimple_call (stmt) && gimple_call_fndecl (stmt))
> > > return;
> > >
> > > - if (gimple_has_volatile_ops (stmt))
> > > + /* Don't return early on *this_2(D) ={v} {CLOBBER}. */
> > > + if (gimple_has_volatile_ops (stmt)
> > > + && (!gimple_clobber_p (stmt)
> > > + || TREE_CODE (gimple_assign_lhs (stmt)) != MEM_REF))
> > > return;
> >
> > But this only means the clobber was not considered as "dead"
> > if there was a later store to the same location. So, why would
> > that change be needed?
>
> Say with -O2:
> struct A { virtual ~A (); char a[10]; };
> struct B : public A { virtual ~B (); char b[10]; };
> A::~A () { a[5] = 7; }
> B::~B () { b[5] = 8; }
> B b;
> we end up in ~B with:
> this_3(D)->D.2229._vptr.A = &MEM[(void *)&_ZTV1B + 16B];
> this_3(D)->b[5] = 8;
> _6 = &this_3(D)->D.2229;
> MEM[(struct A *)this_3(D)]._vptr.A = &MEM[(void *)&_ZTV1A + 16B];
> MEM[(struct A *)this_3(D)].a[5] = 7;
> MEM[(struct A *)this_3(D)] ={v} {CLOBBER};
> *this_3(D) ={v} {CLOBBER};
> and the intent of the above hunk (and the one immediately below this)
> is that we DSE the first clobber (with smaller clobbered size), something
> that IMHO happens very frequently in C++ code, and allows us to better
> DSEthe other stores. There is no point in keeping the smaller clobbers
> around, on the other side, without the second hunk we'd remove pretty much
> all the MEM_REF clobbers at the end of functions, which is undesriable.
Ah, ok.
> > > if (is_gimple_assign (stmt))
> > > @@ -228,6 +231,12 @@ dse_optimize_stmt (gimple_stmt_iterator
> > > if (!dse_possible_dead_store_p (stmt, &use_stmt))
> > > return;
> > >
> > > + /* But only remove *this_2(D) ={v} {CLOBBER} if killed by
> > > + another clobber stmt. */
> > > + if (gimple_clobber_p (stmt)
> > > + && !gimple_clobber_p (use_stmt))
> > > + return;
> >
> > Ah. Hmm, can that ever happen? I presume the issue with non-decl
> > clobbers is that we will never remove them, even if the storage
> > becomes "dead"?
>
> See above.
>
> > > @@ -827,7 +827,26 @@ remove_unused_locals (void)
> > > if (gimple_clobber_p (stmt))
> > > {
> > > tree lhs = gimple_assign_lhs (stmt);
> > > + bool remove = false;
> > > + /* Remove clobbers referencing unused vars, or clobbers
> > > + with MEM_REF lhs referencing unused vars or uninitialized
> > > + pointers. */
> > > if (TREE_CODE (lhs) == VAR_DECL && !is_used_p (lhs))
> > > + remove = true;
> > > + else if (TREE_CODE (lhs) == MEM_REF)
> > > + {
> > > + ssa_op_iter iter;
> > > + tree op;
> > > + FOR_EACH_SSA_TREE_OPERAND (op, stmt, iter, SSA_OP_USE)
> >
> > The only SSA use on a clobber stmt can possibly be TREE_OPERAND (lhs, 0)
> > when lhs is a MEM_REF, no need to use FOR_EACH_SSA_TREE_OPERAND.
> >
> > So just
> >
> > > + else if (TREE_CODE (lhs) == MEM_REF
> > && TREE_CODE (TREE_OPERAND (lhs, 0)) == SSA_NAME)
> > ...
>
> If MEM_REF[&MEM_REF[...], 0] and similar won't ever get through, perhaps.
It won't, that's not valid GIMPLE. See is_gimple_mem_ref_addr ().
> >
> > > + if (SSA_NAME_VAR (op)
> > > + && TREE_CODE (SSA_NAME_VAR (op)) == VAR_DECL
> > > + && !is_used_p (SSA_NAME_VAR (op)))
> > > + remove = true;
> >
> > I'm not sure this is really desired ... isn't a better check to do
> > has_single_use (op)? (which means it would be better suited to
> > be handled in DCE?)
>
> The above change fixed tons of ICEs, fnsplit pass ignored clobber stmts
> (desirable) when deciding what to split, and end up copying the clobber
> into the *.part.0 function, but because nothing but the clobber used
> the this parameter, it wasn't passed. So, we ended up first referencing
> a default definition of a local this variable (just useless stmt), and later
> on when tree-ssa-live.c run we've ignored the clobber again for decisions,
> so that local this variable became !is_used_p and we've ended up referencing
> in-free-list SSA_NAME, which triggered assertion failure. See a few lines
> above this where we similarly remove !is_used_p VAR_DECLs.
> So, IMHO the !is_used_p code belongs to this spot, we can do the clobber
> with
> SSA_NAME_IS_DEFAULT_DEF (op) && TREE_CODE (SSA_NAME_VAR (op)) != PARM_DECL)
> MEM_REF removal in DCE (or both DCE and here).
> Without this code in tree-ssa-live.c we couldn't do:
> if (gimple_clobber_p (stmt))
> {
> have_local_clobbers = true;
> continue;
> }
> in remove_unused_locals safely (i.e. don't bother with mark_all_vars_used
> on the lhs of the clobber).
Ok, note that I didn't object to the SSA_NAME_DEFAULT_DEF handling
but to checking something on SSA_NAME_VAR - sure, if SSA_NAME_VAR
is unused then each remaining SSA name must be a default definition.
So ... what about removing the whole case handling unused
SSA_NAME_VAR?
> > Btw, due to propagation you can end up with MEM[&decl] = {CLOBBER};
> > which should be handled the same as the VAR_DECL case. Eventually
> > just use lhs = get_base_address (gimple_assign_lhs (stmt)) here.
>
> Sure, MEM_REF[&decl] I've seen frequently, but unless it is a whole
> var access and not say clobbering of just a portion of the var, we want
> to treat it as the patch does for DSE reasons, but not for variable
> coalescing reasons during expansion. Perhaps for MEM_REF[&decl, 0] with
> the size of access the same as decl's size cfgexpand.c could treat it as a
> clobber for the whole decl (but then, won't we have there a non-MEM_REF
> clobber in that case after it too and DSE supposedly already removed the
> first MEM_REF clobber?). I mean like adding
> void bar (B &);
> void
> foo ()
> {
> { B b; bar (b); } { B c; bar (c); } { B d; bar (d); }
> }
> to the above testcase.
I was only suggesting that because you only handle DECL = CLOBBER
and MEM[ssa_ptr_2] = CLOBBER in tree-ssa-live.c you miss removing
(partial) clobbers of the MEM[&decl, offset] = CLOBBER kind when DECL is
otherwise unused. I believe this can easily occur with in-place
construction / destruction. A very easy fix for that is to
use get_base_address on the LHS of the CLOBBER.
Richard.
