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

[Sean Silva via llvm-commits]

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160510/dcb36321/attachment.html>