Re: [PATCH 1/2] Pattern matching improvements in ipa-prop.c

[Richard Guenther <rguenther at suse dot de>]

On Mon, 21 Jun 2010, Martin Jambor wrote:

> Hi,
> 
> this patch addresses two deficiencies in ipa-prop.c.  Perhaps the more
> important one is that the pattern matching code
> ipa_analyze_indirect_call_uses which identifies calls to C++
> member-pointer parameters required that the SRA-generated loads from
> the parameter are in the same BB as the condition determining the
> virtual-ness of its value.  That is of course true only if this
> branching happens to be in the first basic block.  That is way too
> limiting and not necessary so I adjusted the code to look for the BB
> with the condition appropriately.
> 
> The second deficiency is in code identifying constant member pointer
> actual arguments.  The code is very simple, it basically only scans
> the BB with the call from the call backwards and looks for
> initializations of the structure.  However, currently it bails out
> whenever it sees a statement other than a single assignment.  That can
> be too restrictive, given that the compiler generated member pointers
> happen to be non-addressable.  So I adjusted the code to check for
> non-addressability and ignore the non-assignment statements.
> 
> I have also slightly modified the testcase that we have for exercising
> these code paths to properly test these two improvements.
> 
> I have bootstrapped and tested the patch on x86_64-linux without any
> issues, OK for trunk?

Instead of changing the existing testcase please add a new one.

Also I'm a bit nervous about

>        if (!gimple_assign_single_p (stmt))
> -     return;
> +     continue;

what is 'arg' usually?  Is it a non-SSA name?  Why can't it appear
on the lhs of a call?  Why can't it appear as operand to an
asm output?

Thus, the scanning in determine_cst_member_ptr looks non-conservative
(maybe the followup patch fixes that).  I'd have used

    if (!stmt_may_clobber_ref (stmt, arg))
      continue;

and then

      if (TREE_CODE (lhs) != COMPONENT_REF
          || TREE_OPERAND (lhs, 0) != arg)
        return;

Ok with that changes.

Thanks,
Richard.

> Thanks,
> 
> Martin
> 
> 
> 
> 2010-06-18  Martin Jambor  <mjambor@suse.cz>
> 
> 	* ipa-prop.c (determine_cst_member_ptr): Ignore non-assignments
> 	instead of bailing out on them.  Updated comments.
> 	(compute_cst_member_ptr_arguments): Check that arg is not
> 	addressable.
> 	(ipa_analyze_indirect_call_uses): Do not require that loads from the
> 	parameter are in the same BB as the condition.  Update comments.
> 
> 	* testsuite/g++.dg/ipa/iinline-1.C: Adjusted.
> 
> Index: icln/gcc/ipa-prop.c
> ===================================================================
> --- icln.orig/gcc/ipa-prop.c
> +++ icln/gcc/ipa-prop.c
> @@ -781,10 +781,11 @@ get_ssa_def_if_simple_copy (tree rhs)
>  }
>  
>  /* Traverse statements from CALL backwards, scanning whether the argument ARG
> -   which is a member pointer is filled in with constant values.  If it is, fill
> -   the jump function JFUNC in appropriately.  METHOD_FIELD and DELTA_FIELD are
> -   fields of the record type of the member pointer.  To give an example, we
> -   look for a pattern looking like the following:
> +   which is a member pointer and is not addressable is filled in with constant
> +   values.  If it is, fill the jump function JFUNC in appropriately.
> +   METHOD_FIELD and DELTA_FIELD are fields of the record type of the member
> +   pointer.  To give an example, we look for a pattern looking like the
> +   following:
>  
>       D.2515.__pfn ={v} printStuff;
>       D.2515.__delta ={v} 0;
> @@ -807,7 +808,7 @@ determine_cst_member_ptr (gimple call, t
>        tree lhs, rhs, fld;
>  
>        if (!gimple_assign_single_p (stmt))
> -	return;
> +	continue;
>  
>        lhs = gimple_assign_lhs (stmt);
>        rhs = gimple_assign_rhs1 (stmt);
> @@ -872,6 +873,7 @@ compute_cst_member_ptr_arguments (struct
>        arg = gimple_call_arg (call, num);
>  
>        if (functions[num].type == IPA_JF_UNKNOWN
> +	  && !TREE_ADDRESSABLE (arg)
>  	  && type_like_member_ptr_p (TREE_TYPE (arg), &method_field,
>  				     &delta_field))
>  	determine_cst_member_ptr (call, arg, method_field, delta_field,
> @@ -1030,6 +1032,10 @@ ipa_note_param_call (struct cgraph_node
>       <bb 2>:
>         f$__delta_5 = f.__delta;
>         f$__pfn_24 = f.__pfn;
> +
> +     ...
> +
> +     <bb 5>
>         D.2496_3 = (int) f$__pfn_24;
>         D.2497_4 = D.2496_3 & 1;
>         if (D.2497_4 != 0)
> @@ -1037,7 +1043,7 @@ ipa_note_param_call (struct cgraph_node
>         else
>           goto <bb 4>;
>  
> -     <bb 3>:
> +     <bb 6>:
>         D.2500_7 = (unsigned int) f$__delta_5;
>         D.2501_8 = &S + D.2500_7;
>         D.2502_9 = (int (*__vtbl_ptr_type) (void) * *) D.2501_8;
> @@ -1048,7 +1054,7 @@ ipa_note_param_call (struct cgraph_node
>         D.2507_15 = *D.2506_14;
>         iftmp.11_16 = (String:: *) D.2507_15;
>  
> -     <bb 4>:
> +     <bb 7>:
>         # iftmp.11_1 = PHI <iftmp.11_16(3), f$__pfn_24(2)>
>         D.2500_19 = (unsigned int) f$__delta_5;
>         D.2508_20 = &S + D.2500_19;
> @@ -1109,17 +1115,18 @@ ipa_analyze_indirect_call_uses (struct c
>    d1 = SSA_NAME_DEF_STMT (n1);
>    d2 = SSA_NAME_DEF_STMT (n2);
>  
> +  join = gimple_bb (def);
>    if ((rec = ipa_get_stmt_member_ptr_load_param (d1, false)))
>      {
>        if (ipa_get_stmt_member_ptr_load_param (d2, false))
>  	return;
>  
> -      bb = gimple_bb (d1);
> +      bb = EDGE_PRED (join, 0)->src;
>        virt_bb = gimple_bb (d2);
>      }
>    else if ((rec = ipa_get_stmt_member_ptr_load_param (d2, false)))
>      {
> -      bb = gimple_bb (d2);
> +      bb = EDGE_PRED (join, 1)->src;
>        virt_bb = gimple_bb (d1);
>      }
>    else
> @@ -1128,7 +1135,6 @@ ipa_analyze_indirect_call_uses (struct c
>    /* Second, we need to check that the basic blocks are laid out in the way
>       corresponding to the pattern. */
>  
> -  join = gimple_bb (def);
>    if (!single_pred_p (virt_bb) || !single_succ_p (virt_bb)
>        || single_pred (virt_bb) != bb
>        || single_succ (virt_bb) != join)
> @@ -1138,7 +1144,7 @@ ipa_analyze_indirect_call_uses (struct c
>       significant bit of the pfn. */
>  
>    branch = last_stmt (bb);
> -  if (gimple_code (branch) != GIMPLE_COND)
> +  if (!bb || gimple_code (branch) != GIMPLE_COND)
>      return;
>  
>    if (gimple_cond_code (branch) != NE_EXPR
> Index: icln/gcc/testsuite/g++.dg/ipa/iinline-1.C
> ===================================================================
> --- icln.orig/gcc/testsuite/g++.dg/ipa/iinline-1.C
> +++ icln/gcc/testsuite/g++.dg/ipa/iinline-1.C
> @@ -29,18 +29,30 @@ int String::funcOne (int delim) const
>    return 1;
>  }
>  
> -int docalling (int (String::* f)(int delim) const)
> +extern int global;
> +
> +int docalling (int c, int (String::* f)(int delim) const)
>  {
>    String S ("muhehehe");
>  
> +  if (c > 2)
> +    global = 3;
> +  else
> +    global = 5;
> +
>    return (S.*f)(4);
>  }
>  
> +int __attribute__ ((noinline,noclone)) get_input (void)
> +{
> +  return 1;
> +}
> +
>  int main (int argc, char *argv[])
>  {
>    int i = 0;
>    while (i < 1000)
> -    i += docalling (&String::funcOne);
> +    i += docalling (get_input (), &String::funcOne);
>    non_existent ("done", i);
>    return 0;
>  }
> 
> 

-- 
Richard Guenther <rguenther@suse.de>
Novell / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746 - GF: Markus Rex