[llvm] r268452 - PM: Port LoopRotation to the new loop pass manager

[Hal Finkel via llvm-commits]

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

> From: "Sean Silva" <chisophugis at gmail.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: "llvm-commits" <llvm-commits at lists.llvm.org>, "Justin Bogner"
> <mail at justinbogner.com>
> Sent: Tuesday, May 10, 2016 6:07:49 PM
> Subject: Re: [llvm] r268452 - PM: Port LoopRotation to the new loop
> pass manager

> On Tue, May 10, 2016 at 2:59 PM, Hal Finkel < hfinkel at anl.gov >
> wrote:

> > > From: "Sean Silva via llvm-commits" < llvm-commits at lists.llvm.org
> > > >
> > 
> 
> > > To: "Justin Bogner" < mail at justinbogner.com >
> > 
> 
> > > Cc: "llvm-commits" < llvm-commits at lists.llvm.org >
> > 
> 
> > > Sent: Tuesday, May 10, 2016 4:55:08 PM
> > 
> 
> > > Subject: Re: [llvm] r268452 - PM: Port LoopRotation to the new
> > > loop
> > > pass manager
> > 
> 

> > > On Tue, May 10, 2016 at 12:46 PM, Justin Bogner <
> > > mail at justinbogner.com > wrote:
> > 
> 

> > > > Sean Silva < chisophugis at gmail.com > writes:
> > > 
> > 
> 
> > > > > On Tue, May 10, 2016 at 11:27 AM, Justin Bogner <
> > > > > mail at justinbogner.com >
> > > 
> > 
> 
> > > > > wrote:
> > > 
> > 
> 
> > > > >
> > > 
> > 
> 
> > > > >> Sean Silva < chisophugis at gmail.com > writes:
> > > 
> > 
> 
> > > > >> > On Tue, May 3, 2016 at 3:02 PM, Justin Bogner via
> > > > >> > llvm-commits
> > > > >> > <
> > > 
> > 
> 
> > > > >> > llvm-commits at lists.llvm.org > wrote:
> > > 
> > 
> 
> > > > >> >
> > > 
> > 
> 
> > > > >> >> Author: bogner
> > > 
> > 
> 
> > > > >> >> Date: Tue May 3 17:02:31 2016
> > > 
> > 
> 
> > > > >> >> New Revision: 268452
> > > 
> > 
> 
> > > > >> >>
> > > 
> > 
> 
> > > > >> >> URL:
> > > > >> >> http://llvm.org/viewvc/llvm-project?rev=268452&view=rev
> > > 
> > 
> 
> > > > >> >> Log:
> > > 
> > 
> 
> > > > >> >> PM: Port LoopRotation to the new loop pass manager
> > > 
> > 
> 
> > > > >> ...
> > > 
> > 
> 
> > > > >> >> --- llvm/trunk/test/Transforms/LoopRotate/basic.ll
> > > > >> >> (original)
> > > 
> > 
> 
> > > > >> >> +++ llvm/trunk/test/Transforms/LoopRotate/basic.ll Tue
> > > > >> >> May
> > > > >> >> 3
> > > > >> >> 17:02:31
> > > 
> > 
> 
> > > > >> 2016
> > > 
> > 
> 
> > > > >> >> @@ -1,4 +1,6 @@
> > > 
> > 
> 
> > > > >> >> ; RUN: opt -S -loop-rotate < %s | FileCheck %s
> > > 
> > 
> 
> > > > >> >> +; RUN: opt -S
> > > 
> > 
> 
> > > > >> >>
> > > 
> > 
> 
> > > > >> -passes='require<loops>,require<targetir>,require<assumptions>,loop(rotate)'
> > > 
> > 
> 
> > > > >> >> < %s | FileCheck %s
> > > 
> > 
> 
> > > > >> >>
> > > 
> > 
> 
> > > > >> >
> > > 
> > 
> 
> > > > >> > Sorry if this is a stupid question, but why do we need to
> > > > >> > explicitly
> > > 
> > 
> 
> > > > >> > "require" the passes when the loop-rotate already declares
> > > > >> > the
> > > 
> > 
> 
> > > > >> dependency?
> > > 
> > 
> 
> > > > >> > (I feel like there's some part of the bigger picture that
> > > > >> > I'm
> > > > >> > missing
> > > 
> > 
> 
> > > > >> here)
> > > 
> > 
> 
> > > > >>
> > > 
> > 
> 
> > > > >> To be clear, there are no dependencies in the new pass
> > > > >> manager
> > > > >> -
> > > > >> passes
> > > 
> > 
> 
> > > > >> request analysis results from an analysis manager and
> > > > >> they're
> > > > >> calculated
> > > 
> > 
> 
> > > > >> or returned from the cache as appropriate. Naturally, the
> > > > >> next
> > > > >> question
> > > 
> > 
> 
> > > > >> one would ask is "okay then, why aren't these calculated on
> > > > >> demand
> > > > >> when
> > > 
> > 
> 
> > > > >> loop-rotate asks for them?".
> > > 
> > 
> 
> > > > >>
> > > 
> > 
> 
> > > > >
> > > 
> > 
> 
> > > > > Ah, yeah. The whole "schedule vs. cache" is one of the main
> > > > > differences of
> > > 
> > 
> 
> > > > > the new PM.
> > > 
> > 
> 
> > > > >
> > > 
> > 
> 
> > > > >
> > > 
> > 
> 
> > > > >>
> > > 
> > 
> 
> > > > >> The mechanical answer is that loop-rotate uses the
> > > > >> `getCachedResult`
> > > 
> > 
> 
> > > > >> API, not the `getResult` one - the difference being that
> > > 
> > 
> 
> > > > >> `getCachedResult` only returns results if they're already
> > > > >> calculated,
> > > 
> > 
> 
> > > > >> and null otherwise. Hence, without the require<> in the
> > > > >> test,
> > > > >> there are
> > > 
> > 
> 
> > > > >> no results.
> > > 
> > 
> 
> > > > >
> > > 
> > 
> 
> > > > >
> > > 
> > 
> 
> > > > > Ah, makes perfect sense!
> > > 
> > 
> 
> > > > >
> > > 
> > 
> 
> > > > >
> > > 
> > 
> 
> > > > >> This of course leads to "why does loop-rotate use
> > > 
> > 
> 
> > > > >> getCachedResult?"
> > > 
> > 
> 
> > > > >>
> > > 
> > 
> 
> > > > >> It has to. These analyses are function analyses, and a loop
> > > > >> pass
> > > > >> isn't
> > > 
> > 
> 
> > > > >> allowed to cause a function analysis to run, much like a
> > > > >> function
> > > > >> pass
> > > 
> > 
> 
> > > > >> isn't allowed to cause a module analysis to be run. They
> > > > >> have
> > > > >> to
> > > > >> stick
> > > 
> > 
> 
> > > > >> to their own level. This enforces correct layering and acts
> > > > >> as
> > > > >> a
> > > 
> > 
> 
> > > > >> safeguard against accidentally doing extra work.
> > > 
> > 
> 
> > > > >>
> > > 
> > 
> 
> > > > >> That said, the "require<loops>" is redundant, since a
> > > > >> LoopPassManager
> > > 
> > 
> 
> > > > >> can't operate without having calculated that. There may also
> > > > >> be
> > > > >> an
> > > 
> > 
> 
> > > > >> argument to be made that the LPM should implicitly calculate
> > > > >> some
> > > > >> other
> > > 
> > 
> 
> > > > >> analyses, but we'd have to be careful not to do too much.
> > > 
> > 
> 
> > > > >>
> > > 
> > 
> 
> > > > >
> > > 
> > 
> 
> > > > > Makes sense. But then `+ assert((LI && TTI && AC) &&
> > > > > "Analyses
> > > > > for
> > > > > loop
> > > 
> > 
> 
> > > > > rotation not available");` is not really appropriate, right?
> > > > > Can't
> > > > > we hit
> > > 
> > 
> 
> > > > > that assert just by passing the right arguments to opt? (by
> > > > > dropping the
> > > 
> > 
> 
> > > > > `require<*>` analyses from the passes= specification?)
> > > 
> > 
> 
> > > > >
> > > 
> > 
> 
> > > > > (sorry, I'm away from the office and don't have an easy way
> > > > > to
> > > > > test
> > > > > locally)
> > > 
> > 
> 

> > > > Yes it does. The other option is to just bail on the transform
> > > > in
> > > > that
> > > 
> > 
> 
> > > > case, which would be a better option for things like fuzzers
> > > > and
> > > 
> > 
> 
> > > > bisection scripts, but makes it harder to ensure we're doing
> > > > everything
> > > 
> > 
> 
> > > > correctly while we're bringing this up. For now I think the
> > > > assert
> > > > is
> > > 
> > 
> 
> > > > best, but we may want to revisit this at some point.
> > > 
> > 
> 

> > > I'm uncomfortable with the assert because it won't be there in
> > > Release and I don't want to be running into UB in my release
> > > build
> > > from a mistyped command line. I think report_fatal_error is more
> > > appropriate.
> > 
> 
> > +1
> 

> > Also, we might want to consider moving the error-reporting logic
> > into
> > the pass manager itself. I could see having a 'Required' parameter
> > to getCachedResult such that it would be a fatal error should no
> > cached result be available.
> 

> Using a function called "getCachedResult" for this purpose seems a
> bit off in the first place. I think it would make sense to call it
> `getRequiredResult` or something like that so that it expresses the
> semantic intent ("as a loop pass, I can't force this to be computed,
> but I require it so it better be there").
Agreed. 

-Hal 

> -- Sean Silva

> > -Hal
> 

> > > _______________________________________________
> > 
> 
> > > llvm-commits mailing list
> > 
> 
> > > llvm-commits at lists.llvm.org
> > 
> 
> > > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> > 
> 

> > --
> 

> > Hal Finkel
> 
> > Assistant Computational Scientist
> 
> > Leadership Computing Facility
> 
> > Argonne National Laboratory
> 

-- 

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-commits/attachments/20160510/d66f3b34/attachment.html>