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

Hal Finkel via llvm-dev llvm-dev at lists.llvm.org
Mon Apr 18 14:46:48 PDT 2016 

----- Original Message -----

> From: "Chandler Carruth" <chandlerc at gmail.com>
> To: "Xinliang David Li" <davidxl at google.com>
> Cc: "Mehdi Amini" <mehdi.amini at apple.com>, "Hal Finkel"
> <hfinkel at anl.gov>, "via llvm-dev" <llvm-dev at lists.llvm.org>
> Sent: Monday, April 18, 2016 4:31:05 PM
> Subject: Re: [llvm-dev] Move InlineCost.cpp out of Analysis?

> On Mon, 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) ?
> 
> Given a public API, yes, that should be the only criteria IMO.

> > 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.
> 
> But I would expect these to also not expose public APIs to the
> analyses. Provided the public API is limited to the transformation,
> I think the code be closely located makes sense.
I'm not sure the situation is as clear cut as you make it out to be. We can factor common components out of different transformations (e.g. out of different inliner implementations), including common components that don't modify the IR, without that component forming what I would generally consider an analysis. In this case, we're really talking about the encapsulation of the inliner's cost heuristic, and the generality of this information is questionable. To put it another way, while I'm fine with someone reusing this logic to build a new inliner, I'd be very skeptical of using it for any other purpose. That's why it does not really feel like an analysis to me. This is not a strong feeling, however, and I'm also fine with it staying in Analysis. 

> > For the same reason, we need to ask first if InlineCost is intended
> > to be a shared utility?
> 
> Sure, but in fact at least some parts of it are shared.

> There is also some reason to prefer surfacing nice high-level public
> APIs when reasonable to do so. For example, if someone wanted to
> write a custom inlining pass that re-used the cost analysis, having
> it separated seems useful. So unless there is a significant problem
> solved by sinking this to be a private API only used by the inliner,
> I would keep it as-is.
I did not take moving to Transforms/Utils to mean making the API private. The things in Transforms/Utils generally have public APIs. 

-Hal 

> > 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/e258e0eb/attachment.html>

More information about the llvm-dev mailing list