Message ID | 20100621095442.714746858@virgil.suse.cz |
---|---|
State | New |
Headers | show |
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; > } > >
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; }