Mehdi Amini via llvm-dev
2016-Apr-18 21:13 UTC
[llvm-dev] Move InlineCost.cpp out of Analysis?
> 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
Chandler Carruth via llvm-dev
2016-Apr-18 21:18 UTC
[llvm-dev] Move InlineCost.cpp out of Analysis?
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. 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/3d78e3af/attachment.html>
Easwaran Raman via llvm-dev
2016-Apr-18 21:24 UTC
[llvm-dev] Move InlineCost.cpp out of Analysis?
Thanks for the comments. On Mon, Apr 18, 2016 at 2:13 PM, Mehdi Amini <mehdi.amini at apple.com> 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. >Yes, I meant that it is not an analysis pass. It does perform analysis. I see one such example of something that performs analysis being added under Transforms/Utils: CmpInstAnalysis.cpp.> > > > >> > >> 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 think that is due to ThinLTO's use of getBlockProfileCount. I'm not sure about ProfileData that depends on Bitcode, do you know why?>ProfileData (specifically CoverageMapping{Reader|Writer}) depends on Object which depends on Bitcode. - Easwaran> -- > 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 > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160418/1729d61b/attachment.html>
Xinliang David Li via llvm-dev
2016-Apr-18 21:28 UTC
[llvm-dev] Move InlineCost.cpp out of Analysis?
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? 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/4786ef46/attachment.html>
Mehdi Amini via llvm-dev
2016-Apr-18 21:28 UTC
[llvm-dev] Move InlineCost.cpp out of Analysis?
> On Apr 18, 2016, at 2:24 PM, Easwaran Raman <eraman at google.com> wrote: > > Thanks for the comments. > > On Mon, Apr 18, 2016 at 2:13 PM, Mehdi Amini <mehdi.amini at apple.com <mailto:mehdi.amini at apple.com>> wrote: > > > On Apr 18, 2016, at 2:07 PM, Hal Finkel via llvm-dev <llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote: > > > > ----- Original Message ----- > >> From: "Easwaran Raman" <eraman at google.com <mailto:eraman at google.com>> > >> To: "via llvm-dev" <llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> > >> Cc: "Chandler Carruth" <chandlerc at gmail.com <mailto:chandlerc at gmail.com>>, "Hal Finkel" <hfinkel at anl.gov <mailto:hfinkel at anl.gov>>, "Philip Reames" > >> <listmail at philipreames.com <mailto:listmail at philipreames.com>>, "David Li" <davidxl at google.com <mailto: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. > Yes, I meant that it is not an analysis pass. It does perform analysis. I see one such example of something that performs analysis being added under Transforms/Utils: CmpInstAnalysis.cpp.If CmpInstAnalysis.cpp is not mutating the IR, it shouldn't sit here in my opinion.> > > > >> > >> 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 think that is due to ThinLTO's use of getBlockProfileCount.Yeah, I know but we should find a way to break this...> I'm not sure about ProfileData that depends on Bitcode, do you know why? > ProfileData (specifically CoverageMapping{Reader|Writer}) depends on Object which depends on Bitcode.Some refactoring or split could help here, this does not seem desirable to me. -- 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 <mailto:llvm-dev at lists.llvm.org> > > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev <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/7b3343f7/attachment.html>
Philip Reames via llvm-dev
2016-Apr-18 23:29 UTC
[llvm-dev] Move InlineCost.cpp out of Analysis?
+1 to what Chandler said here and most of his argument in the following thread. Philip On 04/18/2016 02:18 PM, Chandler Carruth via llvm-dev 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. > > On Mon, Apr 18, 2016 at 2:14 PM Mehdi Amini via llvm-dev > <llvm-dev at lists.llvm.org <mailto: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 <mailto:llvm-dev at lists.llvm.org>> wrote: > > > > ----- Original Message ----- > >> From: "Easwaran Raman" <eraman at google.com > <mailto:eraman at google.com>> > >> To: "via llvm-dev" <llvm-dev at lists.llvm.org > <mailto:llvm-dev at lists.llvm.org>> > >> Cc: "Chandler Carruth" <chandlerc at gmail.com > <mailto:chandlerc at gmail.com>>, "Hal Finkel" <hfinkel at anl.gov > <mailto:hfinkel at anl.gov>>, "Philip Reames" > >> <listmail at philipreames.com <mailto:listmail at philipreames.com>>, > "David Li" <davidxl at google.com <mailto: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 <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 > > > > _______________________________________________ > 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/4bd0f9cc/attachment.html>