Re: SSA_NAMES: should there be an unused, un-free limbo?

["Daniel Berlin" <dberlin at dberlin dot org>]

On 12/21/06, Diego Novillo <dnovillo@redhat.com> wrote:
> Daniel Berlin wrote on 12/21/06 12:21:
>
> > for (i = 0; i < num_ssa_names; i++)
> > {
> >   tree name = ssa_name (i);
> >   if (name && !SSA_NAME_IN_FREELIST (name)
> >    DFS (name)
>  >
> I see that you are not checking for IS_EMPTY_STMT.  Does DFS need to
> access things like bb_for_stmt?

I avoided including that part, but yes, we check for it.

> In any case, that is not important.  I agree that every SSA name in the
> SSA table needs to have a DEF_STMT that is either (a) an empty
> statement, or, (b) a valid statement still present in the IL.

Then we've got a bug here :)

> Note that this is orthogonal to the problem of whether we free up unused
> names from this list.  Every time a statement S disappears, we should
> make sure that the names defined by S get their SSA_NAME_DEF_STMT set to
>   NOP.
>
> Frankly, I'm a bit surprised that we are running into this. I'd like to
> see a test case, if you have one.
Robert, can you attach the testcase you've been working with?
I'm not surprised, but only because I hit it before.   It's pretty rare.

IIRC, what happens it this:

1. We replace all uses of a phi node with something else
2. We then call remove_phi_node with false as the last parameter (only
3 places in the compiler), which ends up destroying the phi node but
not releasing the LHS name (since this is what the last parameter says
whether to do).


void
remove_phi_node (tree phi, tree prev, bool release_lhs_p)
{
...
 release_phi_node (phi);
 if (release_lhs_p)
   release_ssa_name (PHI_RESULT (phi));
}

3. PHI_RESULT (phi) is now in the ssa name list, but SSA_NAME_DEF_STMT
points to a released phi node.

4. We try to walk this at some point later, and crash.

You can see this happens in tree_merge_blocks:

 /* Remove all single-valued PHI nodes from block B of the form
    V_i = PHI <V_j> by propagating V_j to all the uses of V_i.  */
 bsi = bsi_last (a);
 for (phi = phi_nodes (b); phi; phi = phi_nodes (b))
   {
     tree def = PHI_RESULT (phi), use = PHI_ARG_DEF (phi, 0);
...
else
     replace_uses_by (def, use);

     remove_phi_node (phi, NULL, false);
   }

Whenever we hit the else block we end up with a phi node result that
points to a released phi node.  It won't appear in the IR (sine the
phi node has been removed and all the result uses replaced), but will
appear in the ssa_names list.

There are only two other places that call remove_phi_node with false
as the last parameter.
One is moving a phi node, the other appears to be a bug just like the above.