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

[Sean Silva via llvm-commits]

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").

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