This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 2/2] Replace the modified flag with AA-queries


On Thu, Jun 24, 2010 at 4:05 PM, Martin Jambor <mjambor@suse.cz> wrote:
> Hi,
>
> On Mon, Jun 21, 2010 at 02:21:41PM +0200, Richard Guenther wrote:
>> On Mon, 21 Jun 2010, Richard Guenther wrote:
>>
>> > On Mon, 21 Jun 2010, Martin Jambor wrote:
>> >
>> > > Hi,
>> > >
>> > > indirect inlining was written before we had AA-info at our disposal in
>> > > the "early" passes and so when confronted with the necessity to
>> > > guarantee that a member pointer parameter was not changed before it
>> > > was used I have resorted to requiring it is never modified in the
>> > > whole function.
>> > >
>> > > With alias analysis information this is no longer necessary. ?This
>> > > code therefore does away with the flag and uses AA instead. ?To big
>> > > extent I have copied the clobbering identification code from IPA-SRA
>> > > and hope I got it correct.
>> > >
>> > > I added a new testcase checking that we so not indirect-inline when we
>> > > shouldn't because a member-pointer parameter is modified before being
>> > > passed on.
>> > >
>> > > I am aware that there are no hunks removing testcase
>> > > gcc.dg/ipa/modif-1.c in the patch, that is because I generate my
>> > > patches with quilt rather than svn. ?I will remember to svn-delete the
>> > > file.
>> > >
>> > > I have bootstrapped and tested this patch (on top of the previous one)
>> > > on x86_64-linux without any issues, ?OK for trunk?
>> > >
>> > > Thanks,
>> > >
>> > > Martin
>> > >
>> > >
>> > > 2010-06-18 ?Martin Jambor ?<mjambor@suse.cz>
>> > >
>> > > ? * ipa-prop.h (struct ipa_param_descriptor): Remove the modified flag.
>> > > ? (ipa_is_param_modified): Removed.
>> > > ? * ipa-prop.c (visit_store_addr_for_mod_analysis): Do not set the
>> > > ? modified flag.
>> > > ? (mark_modified): New function.
>> > > ? (is_parm_modified_at_call): Likewise.
>> > > ? (compute_pass_through_member_ptrs): Use is_parm_modified_at_call
>> > > ? instead of ipa_is_param_modified.
>> > > ? (ipa_analyze_indirect_call_uses): Likewise.
>> > > ? (ipa_print_node_params): Do not dump the modified flag.
>> > > ? (ipa_write_node_info): Do not stream the modified flag.
>> > > ? (ipa_read_node_info): Likewise.
>> > >
>> > > ? * testsuite/g++.dg/ipa/iinline-2.C: New test.
>> > > ? * testsuite/gcc.dg/ipa/modif-1.c: Removed.
>> > >
>> > > Index: icln/gcc/ipa-prop.c
>> > > ===================================================================
>> > > --- icln.orig/gcc/ipa-prop.c ? ? ?2010-06-20 18:23:13.000000000 +0200
>> > > +++ icln/gcc/ipa-prop.c ? 2010-06-20 18:24:32.000000000 +0200
>> > > @@ -212,7 +212,6 @@ visit_store_addr_for_mod_analysis (gimpl
>> > > ? ? ?{
>> > > ? ? ? ?int index = ipa_get_param_decl_index (info, op);
>> > > ? ? ? ?gcc_assert (index >= 0);
>> > > - ? ? ?info->params[index].modified = true;
>> > > ? ? ? ?info->params[index].used = true;
>> > > ? ? ?}
>> > >
>> > > @@ -707,6 +706,34 @@ type_like_member_ptr_p (tree type, tree
>> > > ? ?return true;
>> > > ?}
>> > >
>> > > +/* Callback of walk_aliased_vdefs. ?Flags that it has been invoked to the
>> > > + ? boolean variable pointed to by DATA. ?*/
>> > > +
>> > > +static bool
>> > > +mark_modified (ao_ref *ao ATTRIBUTE_UNUSED, tree vdef ATTRIBUTE_UNUSED,
>> > > + ? ? ? ? ? ? ?void *data)
>> > > +{
>> > > + ?bool *b = (bool *) data;
>> > > + ?*b = true;
>> > > + ?return true;
>> > > +}
>> > > +
>> > > +/* Return true if the formal parameter PARM might have been modified in this
>> > > + ? function before reaching the statement CALL. ?*/
>> > > +
>> > > +static bool
>> > > +is_parm_modified_at_call (gimple call, tree parm)
>> > > +{
>> > > + ?bool modified = false;
>> > > + ?ao_ref refd;
>> > > +
>> > > + ?ao_ref_init (&refd, parm);
>> > > + ?walk_aliased_vdefs (&refd, gimple_vuse (call), mark_modified,
>> > > + ? ? ? ? ? ? ? &modified, NULL);
>> > > +
>> > > + ?return modified;
>> > > +}
>> > > +
>> > > ?/* Go through arguments of the CALL and for every one that looks like a member
>> > > ? ? pointer, check whether it can be safely declared pass-through and if so,
>> > > ? ? mark that to the corresponding item of jump FUNCTIONS. ?Return true iff
>> > > @@ -733,7 +760,7 @@ compute_pass_through_member_ptrs (struct
>> > > ? ? ? ? int index = ipa_get_param_decl_index (info, arg);
>> > >
>> > > ? ? ? ? gcc_assert (index >=0);
>> > > - ? ? ? if (!ipa_is_param_modified (info, index))
>> > > + ? ? ? if (!is_parm_modified_at_call (call, arg))
>> >
>> > Hm, I do not understand this. ?Do you just want to ask whether
>> > arg is modified _by_ the call? ?Or do you want to know whether
>> > arg is modified on any path leading from function entry to
>> > the call (which is what you implement)? ?In that case I suggest
>> > a better name for the function, like is_parm_modified_before_call.
>
> OK, I have renamed the function, among other things.
>
>> >
>> > Note that you are not implementing any kind of limiting, doing many
>> > of these walk can be very expensive.
>
> Right, I wanted to start with something simple but limiting should be
> done.
>
>> > ?For example in the above loop
>> > I'd first check whether arg is a PARM_DECL before checking
>> > type_like_member_ptr_p.
>
> In compute_pass_through_member_ptrs, the result of
> type_like_member_ptr_p is then used to determine whether we will scan
> for constant member pointer arguments, hoping that most of the time we
> won't.
>
>> > And you can use one caching bitmap
>> > per PARM_DECL for the visited parms to walk_aliased_vdefs which
>> > will bound complexity to number of cfun arguments times number
>> > of stores in the function (much better than number of cfun arguments
>> > times calls in the function times number of stores in the function).
>>
>> Thinking about this again you'd need to manage this caching
>> yourself somehow (and stop walking in the callback if you hit it).
>> You can keep a reaches-modification and doesn't-reach-modification
>> bitmap and OR them from the is_parm_modified_at_call local bitmap
>> depending on result.
>>
>
> Well, I thought about this and eventually came to the conclusion that
> trying to do member pointer indirect inlining (and these AA queries
> are not used for anything else) in presence of complex modifications
> of the parameters probably isn't worth the complexity. ?Therefore I
> decided to use a single visited_stmts bitmap and if
> is_parm_modified_before_call returns true once, make it return true
> for that parameter always.
>
> This might not bring much more accuracy than we have now but has the
> benefit of being able to avoid one complete walk of the function.
> Currently, we need to get the modified flag correct by walking the
> whole function before we embark on examining call statements looking
> for indirect calls (including those of member pointers) and
> calculating jump functions. ?If we replace the modified flag with AA,
> this is not necessary and the whole analysis can be done in one walk.
>
> Moreover, we currently do two sweeps over all nodes just to get all
> parameter descriptors and parameter counts initialized and to
> calculate the modified flag for all nodes before computing the jump
> functions. ?If we do the initialization lazily, that is not necessary
> either.
>
> And that is what the patch below does. ?It still includes a removal of
> gcc.dg/ipa/modif-1.c because that is just a dump scan for the result
> of modification analysis which we do not perform with this patch. ?I
> did add a new testcase that actually tests we get AA-queries right so
> the coverage should not get worse.
>
> I have bootstrapped and tested a very similar variant on x86-64-linux
> without any problems, a bootstrap and test of exactly this version is
> underway. ?What so you think now?
>
> Thanks,
>
> Martin
>
>
> 2010-06-24 ?Martin Jambor ?<mjambor@suse.cz>
>
> ? ? ? ?* ipa-prop.h (struct ipa_param_descriptor): Removed the modified flag.
> ? ? ? ?(struct ipa_node_params): Removed the modification_analysis_done flag.
> ? ? ? ?(ipa_is_param_modified): Removed.
> ? ? ? ?(ipa_analyze_node): Declare.
> ? ? ? ?(ipa_compute_jump_functions): Remove declaration.
> ? ? ? ?(ipa_count_arguments): Likewise.
> ? ? ? ?(ipa_detect_param_modifications): Likewise.
> ? ? ? ?(ipa_analyze_params_uses): Likewise.
> ? ? ? ?* ipa-prop.c (struct param_analysis_info): New type.
> ? ? ? ?(visit_store_addr_for_mod_analysis): Removed.
> ? ? ? ?(visit_load_for_mod_analysis): Renamed to visit_ref_for_mod_analysis,
> ? ? ? ?moved down in the file.
> ? ? ? ?(ipa_detect_param_modifications): Merged into ipa_analyze_params_uses.
> ? ? ? ?(ipa_count_arguments): Made static.
> ? ? ? ?(mark_modified): New function.
> ? ? ? ?(is_parm_modified_before_call): New function.
> ? ? ? ?(compute_pass_through_member_ptrs): New parameter parms_info, call
> ? ? ? ?is_parm_modified_before_call instead of ipa_is_param_modified.
> ? ? ? ?(ipa_compute_jump_functions_for_edge): New parameter parms_info, pass
> ? ? ? ?it to compute_pass_through_member_ptrs.
> ? ? ? ?(ipa_compute_jump_functions): New parameter parms_info, pass it to
> ? ? ? ?ipa_compute_jump_functions_for_edge. ?Call ipa_initialize_node_params
> ? ? ? ?on the callee if it is analyzed. ?Made static.
> ? ? ? ?(ipa_analyze_indirect_call_uses): New parameter parms_info, call
> ? ? ? ?is_parm_modified_before_call instead of ipa_is_param_modified.
> ? ? ? ?(ipa_analyze_call_uses): New parameter parms_info, pass it to
> ? ? ? ?ipa_analyze_indirect_call_uses.
> ? ? ? ?(ipa_analyze_stmt_uses): New parameter parms_info, pass it to
> ? ? ? ?ipa_analyze_call_uses.
> ? ? ? ?(ipa_analyze_params_uses): New parameter parms_info, pass it to
> ? ? ? ?ipa_analyze_stmt_uses. ?Also perform the used analysis. ?Made static.
> ? ? ? ?(ipa_analyze_node): New function.
> ? ? ? ?(ipa_print_node_params): Do not dump the modified flag.
> ? ? ? ?(ipa_write_node_info): Assert uses_analysis_done rather than streaming
> ? ? ? ?it. ?Do not stream the modified parameter flag.
> ? ? ? ?(ipa_read_node_info): Set uses_analysis_done to 1 instead of streaming
> ? ? ? ?it. ?Do not stream the modified parameter flag.
> ? ? ? ?* ipa-cp.c (ipcp_analyze_node): Removed.
> ? ? ? ?(ipcp_init_stage): Iterate only once over the nodes, analyze each one
> ? ? ? ?with only a call to ipa_analyze_node.
> ? ? ? ?* ipa-inline.c (inline_indirect_intraprocedural_analysis): Analyze the
> ? ? ? ?node with only a call to ipa_analyze_node.
>
> ? ? ? ?* testsuite/g++.dg/ipa/iinline-3.C: New test.
> ? ? ? ?* testsuite/gcc.dg/ipa/modif-1.c: Removed.
>
>

This caused:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44915

-- 
H.J.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]