Re: [PATCH] Updating IPCP to SSA

[Jan Hubicka <jh at suse dot cz>]

> 
> I'll use single_succ_edge() which alreay asserts if there isn't exactly 
> one.

OK
> > 
> > If you use push/pop infrastructure, you should not need to clear cfun by
> > hand here.  Or do you have some insight from where the non-NULL is
> > comming?
> 
> yes. Versioning creates redundant cfuns. Once with 
> allocate_struct_function 
> and once with initialize_cfun.
> Here's how I think it should be changed:
> 
> Index: tree-inline.c
> ===================================================================
> --- tree-inline.c       (revision 120790)
> +++ tree-inline.c       (working copy)
> @@ -3202,9 +3203,6 @@
>    old_version_node = cgraph_node (old_decl);
>    new_version_node = cgraph_node (new_decl);
> 
> -  allocate_struct_function (new_decl);
> -  /* Cfun points to the new allocated function struct at this point.  */
> -  cfun->function_end_locus = DECL_SOURCE_LOCATION (new_decl);
> 
>    DECL_ARTIFICIAL (new_decl) = 1;
>    DECL_ABSTRACT_ORIGIN (new_decl) = DECL_ORIGIN (old_decl);
> @@ -3242,7 +3240,9 @@
>                    old_entry_block->count,
>                    old_entry_block->frequency);
>    push_cfun (DECL_STRUCT_FUNCTION (new_decl));
> -
> +   /* Cfun points to the new allocated function struct at this point.  */
> +  cfun->function_end_locus = DECL_SOURCE_LOCATION (new_decl);
> +
>    /* Copy the function's static chain.  */
>    p = DECL_STRUCT_FUNCTION (old_decl)->static_chain_decl;
>    if (p)

Yes, i've noticed this bug while working on early inlining yesterday
too. We also clobber current_function_decl:

Index: tree-inline.c
===================================================================
*** tree-inline.c	(revision 120777)
--- tree-inline.c	(working copy)
*************** tree_function_versioning (tree old_decl,
*** 3194,3199 ****
--- 3191,3197 ----
    struct ipa_replace_map *replace_info;
    basic_block old_entry_block;
    tree t_step;
+   tree old_current_function_decl = current_function_decl;
  
    gcc_assert (TREE_CODE (old_decl) == FUNCTION_DECL
  	      && TREE_CODE (new_decl) == FUNCTION_DECL);
*************** tree_function_versioning (tree old_decl,
*** 3202,3211 ****
    old_version_node = cgraph_node (old_decl);
    new_version_node = cgraph_node (new_decl);
  
-   allocate_struct_function (new_decl);
-   /* Cfun points to the new allocated function struct at this point.  */
-   cfun->function_end_locus = DECL_SOURCE_LOCATION (new_decl);
- 
    DECL_ARTIFICIAL (new_decl) = 1;
    DECL_ABSTRACT_ORIGIN (new_decl) = DECL_ORIGIN (old_decl);
  
--- 3200,3205 ----
*************** tree_function_versioning (tree old_decl,
*** 3322,3326 ****
    free_dominance_info (CDI_DOMINATORS);
    free_dominance_info (CDI_POST_DOMINATORS);
    pop_cfun ();
!   current_function_decl = NULL;
    return;
  }
  
--- 3316,3324 ----
    free_dominance_info (CDI_DOMINATORS);
    free_dominance_info (CDI_POST_DOMINATORS);
    pop_cfun ();
!   current_function_decl = old_current_function_decl;
    return;
  }
  
The end locus code is redundant, since initialize_cfun copies whole cfun
anyway.  I've sent the patch for separate testing and will commit it as
obvious if it passes.
> > 
> > Why this code is not written in a way exploring PHI_NODEs identically to
> > the way MODIFY_STMT is explored here (i.e. ipa_method_compute_modify is
> > not calling ipa_method_modify_stmt (well or ipa_method_phi_stmt) for
> > each PHI node it sees to figure out what ADDRESS_EXPRs appear on RHS?
> > From your POV PHI_NODE is just modify_stmt with multiple source
> > arguments.
> > 
> 
> You're right about recognizing PHI nodes exactly how we do it for
> MODFIY_STMT.
> The part which I don't get is what you're saying about ADDRESS_EXPR.
> If the parameter's address is taken, then it is not in ssa form.
> (I check it by TREE_ADDRESSABLE(param) in ipa_method_compute_modify()) 
> Should I be checking ADDR_EXPR explicitly?

What I was referring to is that you can have

  ssa_var = PHI (&non_ssa_var, ...)

since ADDR_EXPR of variable is considered to be function wide constant.
So you simple need to look into operands of PHIs for possible ADDR_EXPRs
same way as you look into RHS of MODIFY_STMT.
ADDR_EXPR of SSA_NAME is wrong, so you don't need to worry about it.

Thank you,
Honza