[llvm-dev] Move InlineCost.cpp out of Analysis?

Tue Apr 19 10:21:04 PDT 2016

On Mon, Apr 18, 2016 at 4:36 PM, Philip Reames <listmail at philipreames.com>
wrote:

>
>
> On 04/18/2016 04:05 PM, Easwaran Raman via llvm-dev wrote:
>
>
>
> On Mon, Apr 18, 2016 at 3:25 PM, Chandler Carruth < <chandlerc at gmail.com>
> chandlerc at gmail.com> wrote:
>
>> On Mon, Apr 18, 2016 at 3:20 PM Easwaran Raman <eraman at google.com> wrote:
>>
>>> On Mon, Apr 18, 2016 at 3:00 PM, Chandler Carruth via llvm-dev <
>>> llvm-dev at lists.llvm.org> wrote:
>>>
>>>> On Mon, Apr 18, 2016 at 2:48 PM Hal Finkel < <hfinkel at anl.gov>
>>>> hfinkel at anl.gov> wrote:
>>>>
>>>>>
>>>>>
>>>>> ------------------------------
>>>>>
>>>>> *From: *"Xinliang David Li" < <davidxl at google.com>davidxl at google.com>
>>>>>
>>>>> On Mon, Apr 18, 2016 at 2:33 PM, Mehdi Amini < <mehdi.amini at apple.com>
>>>>> mehdi.amini at apple.com> wrote:
>>>>>>
>>>>>> In the current case at stake: the issue is that we can't make the
>>>>>> Analysis library using anything from the ProfileData library. Conceptually
>>>>>> there is a problem IMO.
>>>>>>
>>>>>
>>>>>
>>>>> Yes -- this is a very good point.
>>>>>
>>>>> Independent of anything else, +1.
>>>>>
>>>>
>>>> The design of ProfileData and reading profile information in the entire
>>>> middle end had a really fundamental invariant that folks seem to have lost
>>>> track of:
>>>>
>>>> a) There is exactly *one* way to get at profile information from
>>>> general analyses and transforms: a dedicated analysis pass that manages
>>>> access to the profile info.
>>>>
>>>> b) There is exactly *one* way for this analysis to compute this
>>>> information from an *external* profile source: profile metadata attached to
>>>> the IR.
>>>>
>>>> c) There could be many external profile sources, but all of them should
>>>> be read and then translated into metadata annotations on the IR so that
>>>> serialization / deserialization preserve them in a common format and we can
>>>> reason about how they work.
>>>>
>>>>
>>>> This layering is why it is only a transform that accesses ProfileData
>>>> -- it is responsible for annotating the IR and nothing else. Then the
>>>> analysis uses these annotations and never reads the data directly.
>>>>
>>>> I think this is a really important separation of concerns as it ensures
>>>> that we don't get an explosion of different analyses supporting various
>>>> different subsets of profile sources.
>>>>
>>>>
>>>> Now, the original design only accounted for profile information
>>>> *within* a function body, clearly it needs to be extended to support
>>>> intraprocedural information. But I would still expect that to follow a
>>>> similar layering where we first read the data into IR annotations, then
>>>> have an analysis pass (this time a module analysis pass in all likelihood)
>>>> that brokers access to these annotations through an API that can do
>>>> intelligent things like synthesizing it from the "cold" attribute or
>>>> whatever when missing.
>>>>
>>>>
>>> Invariants b) and c) above are still true, but a) is not since
>>> InlineCost directly calls ProfileSummary instead of through an analysis
>>> pass.
>>>
>>
>> Not quite -- ProfileSummary seems to only exist in the profile *reading*
>> code, so it isn't *just* an annotation on the IR.
>>
> Sorry, I'm lost here. There is an IR annotation (module flag) called
> ProfileSummary and this data is represented in memory by the ProfileSummary
> class. . This class  provides methods to serialize/de-serialize  this data
> into/from metadata. This class has methods to compute this summary, but
> this is used only by the profile readers and writers. I am not sure what
> you mean by "it isn't just* an annotation on the IR".
>
> If this is true, why is this class currently part of ProfileData?
> Shouldn't this live in IR?  Or to phrase this differently, why is an
> "analysis" over IR mixed in with the parsing code?
>

ProfileSummary does not have any analysis over IR, so I don't understand
that part of your question. As to why this is part of ProfileData, it is
partly because initially summary was something that was just displayed by
the llvm-profdata tool and partly because it felt natural to keep all
profile related code together.  Having said that, I have nothing against
moving it to IR if that is the appropriate place.

- Easwaran

>
> - Easwaran
>
>
>>
>> I think the problem here is that we have failed to build a proper
>> separate abstraction around the interprocedural profile data that is
>> extracted from IR annotations. That abstraction should have nothing to do
>> with reading profile data and so shouldn't live in the ProifileData
>> library, it should (IMO) live in the Analysis library.
>>
>>   I'll add a module level pass as you suggest, but that still needs
>>> breaking the ProfileData->Analysis dependence chain.
>>>
>>
>> Well, this will also re-highlight the fundamental pass manager problem as
>> you won't have easy access to this analysis pass.
>>
>>
>>
>>>
>>> - Easwaran
>>>
>>>> -Chandler
>>>>
>>>
>>>> _______________________________________________
>>>> LLVM Developers mailing list
>>>> llvm-dev at lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>>
>>>>
>
>
> _______________________________________________
> LLVM Developers mailing listllvm-dev at lists.llvm.orghttp://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160419/810b11c4/attachment.html>