Re: Fix a tcb crash and a potential bug on mainline

[Steven Bosscher <stevenb at suse dot de>]

On Tuesday 19 October 2004 22:48, Jeffrey A Law wrote:
> On Mon, 2004-10-18 at 15:03, Steven Bosscher wrote:
> > Later on cleanup_control_flow() tries to do its thing on basic block 4.
> > It tries to fold "(c_13 != 0B)", but c_13 is on the free list, so it
> > does not have a TREE_TYPE, so fold crashes.
>
> Hmm.  I wonder if this is the same bug Andrew Pinski was trying to
> fix by not clearing TREE_TYPE.  Your description matches a lot of
> the tidbits Andrew mentioned.

Yes, it is.


> > I think the root cause of this bug is that c_13 is put on the free list,
> > and I think its SSA_NAME_DEF_STMT should be set to the empty statement,
> > because remove_phi_arg_num() cannot know if there are any immediate
> > uses of it still left.
>
> ???  Putting c_13 on the free list is the right thing to do.

I believe it is not.  It is *wrong* to put an SSA_NAME on the free list
unless you're absolutely positively sure that the thing is not used
anywhere in the code.

The free list is for SSA names that are available for recycling.  An
SSA_NAME that still appears in the code, even if it is in unreachable
code, is *not* elligible for recycling until that unreachable code is
actually removed.

I really don't understand how you can think this is the right thing to
do, it's the most broken idea I've seen in a long time.


>  I'm not
> sure why you think setting SSA_NAME_DEF_STMT to NULL is particularly
> interesting though.  Whether or not the name has any uses left in the
> IL isn't something we use to determine whether or not to set
> SSA_NAME_DEF_STMT to any particular value IIRC.

Not usually.  But this case is special.  Normally we don't just throw
away a statement, we first update the code or we delete entire blocks
of code.  In this particular case we're only deleting a PHI node and
we don't know if the block we're deleting the PHI from is unreachable
(we could, but it's not the business of remove_phi_arg_num() to remove
unreachable blocks).  So conceptually the PHI_RESULT of the deleted
PHI becomes undefined.  But it is not dead until all its uses are also
removed.  We express "undefined-ness" by setting the SSA_NAME_DEF_STMT
to the empty statement.


> >  I don't know if making that change could have
> > some unforseen effects, so I decided to re-hide the the bug by just not
> > looking at unreachable blocks in cleanup_control_flow().
>
> I'm not sure this is sufficient to hide the bug well enough though.

Me neither.  But I am fairly sure that making the PHI_RESULT undefined,
instead of putting it on the free list, will not just hide but actually
*fix* it.

I would have posted a patch to do that if I had had more time and was
less unsure about unexpected effects of such a change.  I might still
give it a try.


> >   This may be something Andrew may want to look into as part
> > of his immediate uses work??
>
> It's something we've pondered and talked about a little.  Basically
> with the immediate use work we would know where all the uses are
> and we could probably arrange to delete those uses.

How would you delete those uses?  You don't know if those statements
are dead, so you really can't do that.

What I mean with this is that perhaps we should not put the SSA_NAME on
the free list if there still are immediate uses.  On Andrew's branch
that is very easy to do, if we decide to fix the bug this way.


> > This fix is completely untested at the moment, but it's quite obvious
> > and it fixes the ICE I was seeing.  Still, I am a bit surprised that we
> > have not been in trouble over this bug before - perhaps we should still
> > fix the remove_phi_arg_num() issue instead of papering it over this way?
>
> I'm not sure what you mean by fix remove_phi_arg_num

I mean to fix it by making it set the SSA_NAME_DEF_STMT of PHIs that
it is deleting to the empty statement...


> except to compute
> the immediate uses of the result and delete those statements, which
> in turn can cause other statements to get deleted.  Basically it turns
> into immediate use based DCE.  Roughly the same thing we'd end up
> doing if we tried to do integrated dead code removal using
> immediate uses.

...and this is what I don't mean and don't want.

(Not for the CFG cleanup anyway.  Perhaps doing immediate uses based DCE
is a good idea for the mythical "fast DCE".  LLVM used this algorithm
last time I looked.  I don't know which is faster in practice, classic
DCE like we have now, or immediate uses based DCE.  Something we can try
on Andrews branch.  </OT>)


> Alternately we could split up cleanup_control_expr_graph into two
> phases.  The first identifies all the COND_EXPRs and SWITCH_EXPRs
> which need updating, but does not do any edge removal.   Once we
> have done our full IL scan, we go back to the nodes where we know
> which outgoing edge will be taken and proceed to remove the
> COND_EXPR/SWITCH_EXPR and the untaken edges.

Talk about killing a bug with a sledge hammer :-)

Take a step back, and consider what is really happening:
Conceptually, after removing the PHI node, the PHI_RESULT c_13 simply
becomes an SSA_NAME without a defining statement.  The obviously correct
way to handle this is by treating it just like we treat any other SSA
name without defining statement: Setting SSA_NAME_DEF_STMT to the empty
statement.  The whole infrastructure already knows how to deal with such
SSA_NAMEs, so except a change in remove_phi_arg_num() it would probably 
not need any changes anywhere else.  At least that's what I hope, because
I'm going to try this approach now...

Gr.
Steven