Hi, On Sat, Aug 29 2020, Jan Hubicka wrote: >> Hi, >> >> On Sat, Aug 29 2020, Jan Hubicka wrote: >> >> Hi, >> >> >> >> this patch changes ipa_call_context::estimate_size_and_time to store >> >> its results into member fields of the ipa_call_context class instead >> >> into pointers it receives as parameters so that it can compute ore >> >> stuff without cluttering the interface even further. >> >> >> >> Bootstrapped and tested on x86_64-linux. OK for master on top of the >> >> previous two patches? >> > >> > ipa_call_context is intended to be structure holding all parameters that >> > are needed to produce the estimates (size/time/hints). >> >> even today it only "holds" the data when it resides in the RCU cache, >> otherwise it points to data "owned" by the caller. Admittedly, my first >> patch makes the cache data structure separate, making ipa_class_context >> only a utility for calculating the estimates - but given how all the >> code is structured, it does not really work as the grand encapsulator of >> all context data when passing it from a function to function. >> >> > Adding the actual estimates there would duplicate it with cache. >> >> The first patch in the series makes the cache items not contain >> ipa_call_context directly, so in my patch series at least, the estimates >> are not duplicated. > > ipa_call_context defines the context estimates depends on. This puts > all the info to one place and makes the cache well defined - it assigns > contexts to estimates. From this point of view I do not quite like > duplicating this logic (copying things into different datastructure) and > making contxt to also contain the esitmates since these are, well, not > context of the call. > > I am happy with merging the analysis results into something like > class function_body_estimate holding all the values. > > Games with the ownerhip you mention above was not original plan. > While perfing inliner I noticed that we spend measurable time in malloc > and that mostly goes to alocaitng the vectors (which we did for long > time). Perhaps cleaner solution is to have > ipa_context_base which is derived by ipa_context and ipa_cached_context > where first preallocats on stack while second allocates on heap?
All right, but let's start from the basic objective of the patch. Do we want to have something like ipa_call_arg_values? I hope that the answer to this question is yes. I certainly hope that we want to get rid of passing around each vector as an individual parameter. The one alternative I can think of would be to make each function that now receives the three or four vectors either a method of ipa_call_context or receive ipa_call_context as the parameter. That however does not fit naturally to uses in 1) ipa_fn_summary_t::duplicate, 2) ipa_merge_fn_summary_after_inlining and 3) anywhere in IPA-CP where I'd like the pass to continue with the pass owning the vectors and constructing contexts for each change. If the answer to the above question is yes, then we can have ipa_call_arg_values containing pointers to the vectors - which would be part of ipa_call_context or it can be even derived from it - and which would be constructible from ipa_auto_call_arg_values, which would contain the vectors as autovecs. This would also help me unify the information for IPA-CP clones. Then ipa_cached_call_context would not need to be a different type, it would only allocate the vectors almost like before although I'd prefer it to be, just so that the release function is specific to this kind of context. Just the vec structures themselves would be on the heap. Would that work? Note that, however, I still think that the most context-consumer friendly interface would be to have static function ipa_call_context::for_inlined_edge (see my 2nd patch) which would fetch a context either from a cache or create it without the user knowing anything about the cache at all. But then in order to cache the estimates, they would have to be part of the context... but I do not insist, we can leave the caching explicit. Anyway, please let me know what you think about the above plan. Martin > > Honza >> >> > What >> > about keeping them separate and inventing ipa_call_estimates structure >> > to hold the reults? >> >> I can but unless you do not like the first patch and want me to re-write >> it or just not do anything like it, I don't think it matters because the >> structures will almost always lie next to each other on the user's >> stack. >> >> Martin >> >> >> >> >> >> >> gcc/ChangeLog: >> >> >> >> 2020-08-28 Martin Jambor <mjam...@suse.cz> >> >> >> >> * ipa-fnsummary.h (class ipa_call_context): Changed declaration of >> >> estimate_size_and_time to accept two booleans. Added an overload >> >> of the method without any parameters. New fields m_size, >> >> m_min_size, m_time, m_nonspecialized_time and m_hints. >> >> * ipa-cp.c (hint_time_bonus): Changed the second parameter from >> >> just hints to a const reference to ipa_call_context. >> >> (perform_estimation_of_a_value): Adjusted to the new interface of >> >> ipa_call_context::estimate_size_and_time. >> >> * ipa-fnsummary.c (ipa_call_context::estimate_size_and_time): >> >> Modified to store results into member fields of the class. >> >> * ipa-inline-analysis.c (do_estimate_edge_time): Adjusted to the >> >> new interface of ipa_call_context::estimate_size_and_time. >> >> (do_estimate_edge_size): Likewise. >> >> (do_estimate_edge_hints): Likewise.
