Xinliang David Li via llvm-dev
2016-Apr-18 21:38 UTC
[llvm-dev] Move InlineCost.cpp out of Analysis?
On Mon, Apr 18, 2016 at 2:33 PM, Mehdi Amini <mehdi.amini at apple.com> wrote:> > On Apr 18, 2016, at 2:28 PM, Xinliang David Li <davidxl at google.com> wrote: > > > > On Mon, Apr 18, 2016 at 2:18 PM, Chandler Carruth <chandlerc at gmail.com> > wrote: > >> The difference between Analysis and Transforms is *not* about passes, but >> about what the code *does*. >> >> Code for mutating the IR should be in Transforms, and code that analyzes >> the IR without mutating it should be in Analysis. This is why, for example, >> InstructionSimplify is in Analysis -- it does not mutate the IR in any way. >> >> So I think InlineCost and similar things should stay in the Analysis >> library regardless of whether they are passes or not. >> > > Is that the only criteria (IR mod or not) ? Most of the transformations > have pass specific analysis (that are not shared with other clients) -- > those code stay with the transformation -- it does not make sense to move > those code in to Analysis. For the same reason, we need to ask first if > InlineCost is intended to be a shared utility? > > > 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. David> > Moving something from Analysis to TransformUtils just because it needs to > access ProfileData is a slippery slope... > > -- > Mehdi > > > > David > > > >> >> On Mon, Apr 18, 2016 at 2:14 PM Mehdi Amini via llvm-dev < >> llvm-dev at lists.llvm.org> wrote: >> >>> >>> > On Apr 18, 2016, at 2:07 PM, Hal Finkel via llvm-dev < >>> llvm-dev at lists.llvm.org> wrote: >>> > >>> > ----- Original Message ----- >>> >> From: "Easwaran Raman" <eraman at google.com> >>> >> To: "via llvm-dev" <llvm-dev at lists.llvm.org> >>> >> Cc: "Chandler Carruth" <chandlerc at gmail.com>, "Hal Finkel" < >>> hfinkel at anl.gov>, "Philip Reames" >>> >> <listmail at philipreames.com>, "David Li" <davidxl at google.com> >>> >> Sent: Monday, April 18, 2016 2:39:49 PM >>> >> Subject: Move InlineCost.cpp out of Analysis? >>> >> >>> >> >>> >> Hi, >>> >> >>> >> >>> >> After r256521 - which removes InlineCostAnalysis class - I think >>> >> there is no strong reason for InlineCost.cpp to be part of the >>> >> Analysis library. Is it fine to make it part of TransformUtils? >>> >> >>> > >>> > Given that InlineCost is not really an analysis any longer, I think >>> this is fine. >>> >>> Isn't it? It is not a pass, but I see it as an analysis utils. >>> >>> > >>> >> >>> >> I submitted r266477 (which has now been reverted) that made Analysis >>> >> depend on ProfileData in order to obtain ProfileSummary for the >>> >> module, but there is an existing dependency of ProfileData on >>> >> Analysis (through Object and BitCode). >>> >>> The real issue is that BitCode depends on Analysis I think. >>> I'm not sure about ProfileData that depends on Bitcode, do you know why? >>> >>> -- >>> Mehdi >>> >>> >>> >> Moving InlineCost.cpp under >>> >> Transforms/Utils will fix this issue. There are other ways to fix >>> >> this (make Inliner.cpp get the ProfileSummary and pass it to >>> >> InlineCost, for example), but I think it makes sense to move >>> >> InlineCost. >>> >> >>> >> >>> >> Thoughts? >>> >> >>> >> >>> >> Thanks, >>> >> Easwaran >>> >> >>> >> >>> > >>> > -- >>> > Hal Finkel >>> > Assistant Computational Scientist >>> > Leadership Computing Facility >>> > Argonne National Laboratory >>> > _______________________________________________ >>> > LLVM Developers mailing list >>> > 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/79e622b5/attachment.html>
Hal Finkel via llvm-dev
2016-Apr-18 21:48 UTC
[llvm-dev] Move InlineCost.cpp out of Analysis?
----- Original Message -----> From: "Xinliang David Li" <davidxl at google.com> > To: "Mehdi Amini" <mehdi.amini at apple.com> > Cc: "Chandler Carruth" <chandlerc at gmail.com>, "Hal Finkel" > <hfinkel at anl.gov>, "via llvm-dev" <llvm-dev at lists.llvm.org> > Sent: Monday, April 18, 2016 4:38:12 PM > Subject: Re: [llvm-dev] Move InlineCost.cpp out of Analysis?> On Mon, Apr 18, 2016 at 2:33 PM, Mehdi Amini < mehdi.amini at apple.com > > wrote:> > > On Apr 18, 2016, at 2:28 PM, Xinliang David Li < > > > davidxl at google.com > > > > > > > wrote: > > >> > > On Mon, Apr 18, 2016 at 2:18 PM, Chandler Carruth < > > > chandlerc at gmail.com > wrote: > > >> > > > The difference between Analysis and Transforms is *not* about > > > > passes, > > > > but about what the code *does*. > > > > > >> > > > Code for mutating the IR should be in Transforms, and code that > > > > analyzes the IR without mutating it should be in Analysis. This > > > > is > > > > why, for example, InstructionSimplify is in Analysis -- it does > > > > not > > > > mutate the IR in any way. > > > > > >> > > > So I think InlineCost and similar things should stay in the > > > > Analysis > > > > library regardless of whether they are passes or not. > > > > > > > > > Is that the only criteria (IR mod or not) ? Most of the > > > transformations have pass specific analysis (that are not shared > > > with other clients) -- those code stay with the transformation -- > > > it > > > does not make sense to move those code in to Analysis. For the > > > same > > > reason, we need to ask first if InlineCost is intended to be a > > > shared utility? > > > > > 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. -Hal> David> > Moving something from Analysis to TransformUtils just because it > > needs to access ProfileData is a slippery slope... >> > -- > > > Mehdi >> > > David > > >> > > > On Mon, Apr 18, 2016 at 2:14 PM Mehdi Amini via llvm-dev < > > > > llvm-dev at lists.llvm.org > wrote: > > > > > >> > > > > > On Apr 18, 2016, at 2:07 PM, Hal Finkel via llvm-dev < > > > > > > llvm-dev at lists.llvm.org > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > ----- Original Message ----- > > > > > > > > > >> > > > > >> From: "Easwaran Raman" < eraman at google.com > > > > > > > > > > > > > > > > >> To: "via llvm-dev" < llvm-dev at lists.llvm.org > > > > > > > > > > > > > > > > >> Cc: "Chandler Carruth" < chandlerc at gmail.com >, "Hal > > > > > >> Finkel" > > > > > >> < > > > > > >> hfinkel at anl.gov >, "Philip Reames" > > > > > > > > > > > > > > > >> < listmail at philipreames.com >, "David Li" < > > > > > >> davidxl at google.com > > > > > >> > > > > > > > > > > > > > > > > >> Sent: Monday, April 18, 2016 2:39:49 PM > > > > > > > > > > > > > > > >> Subject: Move InlineCost.cpp out of Analysis? > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > >> Hi, > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > >> After r256521 - which removes InlineCostAnalysis class - I > > > > > >> think > > > > > > > > > > > > > > > >> there is no strong reason for InlineCost.cpp to be part of > > > > > >> the > > > > > > > > > > > > > > > >> Analysis library. Is it fine to make it part of > > > > > >> TransformUtils? > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Given that InlineCost is not really an analysis any longer, > > > > > > I > > > > > > think > > > > > > this is fine. > > > > > > > > > >> > > > > Isn't it? It is not a pass, but I see it as an analysis > > > > > utils. > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > >> I submitted r266477 (which has now been reverted) that > > > > > >> made > > > > > >> Analysis > > > > > > > > > > > > > > > >> depend on ProfileData in order to obtain ProfileSummary > > > > > >> for > > > > > >> the > > > > > > > > > > > > > > > >> module, but there is an existing dependency of ProfileData > > > > > >> on > > > > > > > > > > > > > > > >> Analysis (through Object and BitCode). > > > > > > > > > >> > > > > The real issue is that BitCode depends on Analysis I think. > > > > > > > > > > > > > > > I'm not sure about ProfileData that depends on Bitcode, do > > > > > you > > > > > know > > > > > why? > > > > > > > > > >> > > > > -- > > > > > > > > > > > > > > > Mehdi > > > > > > > > > >> > > > > >> Moving InlineCost.cpp under > > > > > > > > > > > > > > > >> Transforms/Utils will fix this issue. There are other ways > > > > > >> to > > > > > >> fix > > > > > > > > > > > > > > > >> this (make Inliner.cpp get the ProfileSummary and pass it > > > > > >> to > > > > > > > > > > > > > > > >> InlineCost, for example), but I think it makes sense to > > > > > >> move > > > > > > > > > > > > > > > >> InlineCost. > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > >> Thoughts? > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > >> Thanks, > > > > > > > > > > > > > > > >> Easwaran > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > > > > > > > Hal Finkel > > > > > > > > > > > > > > > > Assistant Computational Scientist > > > > > > > > > > > > > > > > Leadership Computing Facility > > > > > > > > > > > > > > > > Argonne National Laboratory > > > > > > > > > > > > > > > > _______________________________________________ > > > > > > > > > > > > > > > > LLVM Developers mailing list > > > > > > > > > > > > > > > > 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 > > > > > > > > > >-- Hal Finkel Assistant Computational Scientist Leadership Computing Facility Argonne National Laboratory -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160418/fd9f985d/attachment.html>
Chandler Carruth via llvm-dev
2016-Apr-18 22:00 UTC
[llvm-dev] Move InlineCost.cpp out of Analysis?
On Mon, Apr 18, 2016 at 2:48 PM Hal Finkel <hfinkel at anl.gov> wrote:> > > ------------------------------ > > *From: *"Xinliang David Li" <davidxl at google.com> > > On Mon, Apr 18, 2016 at 2:33 PM, Mehdi Amini <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. -Chandler -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160418/42a635cc/attachment.html>