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

[Justin Bogner via llvm-commits]

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.