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

[Jeffrey A Law <law at redhat dot com>]

On Wed, 2004-10-20 at 07:14, Andrew MacLeod wrote:
> On Tue, 2004-10-19 at 19:13, Jeffrey A Law wrote:
> > On Tue, 2004-10-19 at 16:36, Steven Bosscher wrote:
> > 
> > > > Err, if we remove the last argument to a PHI node then by definition
> > > > the block containing the PHI is unreachable as it has no incoming
> > > > edges.
> > > >
> > > > There is no such thing as an "undefined" PHI node.
> > > 
> > > I didn't say we do.  I said we may have an undefined PHI result,
> > > i.e. and SSA_NAME without a definition.
> > OK.  And that is something that is, unfortunately, normal in our
> > implementation.  If I look at the SSA_NAME manager that's by far
> > its largest wart.
> > 
> 
> If I understand the problem properly, the fundamental problem here is
> that a block becomes unreachable by removing all the incoming edges. PHI
> arguments for those edges are removed as the edge is removed. When the
> last edge is removed, the PHI node has no more arguments, and thus gets
> deleted.  None of the code in the dead block is removed until later when
> delete_unreachable_blocks is called. Meanwhile, we end up pawing through
> one or more stmts in the dead block which contains uses of the PHI
> result.
> 
> I have a couple of questions.
> 
> 1 - Why does cleanup_control_flow() ever look at a stmts inside an
> unreachable block? Its seems rather, umm, unnecessary, for starters. Why
> not just call delete_unreachable_blocks() as the *first* thing in
> cfg_cleanup(). It doesn't look like its an expensive call... 
That doesn't help solve this problem.  The unreachable blocks are
created by cleanup_control_flow itself.  So calling
delete_unreachable_blocks before or after cleanup_control_flow
doesn't solve the problem.


> 2 - The issue may also exist outside cleanup_control_flow, so why does
> remove_phi_arg_num have to remove the PHI node when is removes the last
> argument. Why not allow the PHI node to exist with no arguments. It'll
> get removed and free'd if the block remains unreachable.
An excellent question and suggestion.  It may actually be the best
proposed solution.

I think we would all agree that a PHI with no arguments can only occur
in an unreachable block.  Can someone verify that we release PHI nodes
when we remove unreachable blocks?  I suspect we don't right now
because we rely upon generic code to identify and remove unreachable
blocks.  IIRC that does doesn't know about PHIs at all.



>  Maybe someone
> will add another edge back in at some point during the optimization,
> then there are PHI nodes ready to take the arguments.  I presume right
> now we make sure we add new edges before deleting old ones when
> redirecting things (to avoid deleting the PHI).
I'm not aware of any code that does this.  But your solution certainly
avoids this kind of problem in the future.  It's certainly the case
that we've had code elsewhere in the compiler which would temporarily
bump up a counter to avoid having objects disappear.  This was
pretty common with labels in the RTL optimizers.
 
Jeff