Philip Reames via llvm-dev
2016-Apr-18 23:36 UTC
[llvm-dev] Move InlineCost.cpp out of Analysis?
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 > <mailto:chandlerc at gmail.com>> wrote: > > On Mon, Apr 18, 2016 at 3:20 PM Easwaran Raman <eraman at google.com > <mailto: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 <mailto:llvm-dev at lists.llvm.org>> wrote: > > On Mon, Apr 18, 2016 at 2:48 PM Hal Finkel > <hfinkel at anl.gov <mailto:hfinkel at anl.gov>> wrote: > > > > ------------------------------------------------------------------------ > > *From: *"Xinliang David Li" <davidxl at google.com > <mailto:davidxl at google.com>> > > On Mon, Apr 18, 2016 at 2:33 PM, Mehdi Amini > <mehdi.amini at apple.com > <mailto: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?> > - 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 <mailto:llvm-dev at lists.llvm.org> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev > > > > > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > http://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/20160418/cb4e593e/attachment.html>
Easwaran Raman via llvm-dev
2016-Apr-19 17:21 UTC
[llvm-dev] Move InlineCost.cpp out of Analysis?
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>
Philip Reames via llvm-dev
2016-Apr-19 21:00 UTC
[llvm-dev] Move InlineCost.cpp out of Analysis?
On 04/19/2016 10:21 AM, Easwaran Raman wrote:> > > On Mon, Apr 18, 2016 at 4:36 PM, Philip Reames > <listmail at philipreames.com <mailto: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 <mailto:chandlerc at gmail.com>> wrote: >> >> On Mon, Apr 18, 2016 at 3:20 PM Easwaran Raman >> <eraman at google.com <mailto: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 >> <mailto:llvm-dev at lists.llvm.org>> wrote: >> >> On Mon, Apr 18, 2016 at 2:48 PM Hal Finkel >> <hfinkel at anl.gov <mailto:hfinkel at anl.gov>> wrote: >> >> >> >> ------------------------------------------------------------------------ >> >> *From: *"Xinliang David Li" >> <davidxl at google.com <mailto:davidxl at google.com>> >> >> On Mon, Apr 18, 2016 at 2:33 PM, Mehdi Amini >> <mehdi.amini at apple.com >> <mailto: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. >Looking at the declaration of ProfileSummary in ProfileCommon.h, it definitely looks like it contains some information which should simply be associated with the Module/Function. As a side note, the use of inheritance to represent each profiling "kind" seems highly suspicious. Outside of the parsing, nothing should need to know the "kind" of profiling used. That's the key point of the profiling metadata abstraction. Actually, can you give a pointer to where this metadata is defined? Looking at the definition of getMD, I'm worried we have a far bigger problem here. It looks like this expects to have different information stored in metadata based on the kind of profiling. If that's true, I see that as a major problem. This is something which would need to be very well justified and I haven't seen that discussion take place. More minor: isFunctionHot and isFunctionUnlikely should be part of an analysis file.> - 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 <mailto:llvm-dev at lists.llvm.org> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >> >> >> >> >> _______________________________________________ >> LLVM Developers mailing list >> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org> >> http://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/89a573d8/attachment.html>