<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, May 10, 2016 at 4:43 PM, Chandler Carruth via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div><div class="h5"><div dir="ltr">On Tue, May 10, 2016 at 5:07 PM Sean Silva via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, May 10, 2016 at 2:59 PM, Hal Finkel <span dir="ltr"><<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div style="font-family:arial,helvetica,sans-serif;font-size:10pt;color:#000000"><br><br><hr><blockquote style="border-left:2px solid rgb(16,16,255);margin-left:5px;padding-left:5px;color:rgb(0,0,0);font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt"><b>From: </b>"Sean Silva via llvm-commits" <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>><br><b>To: </b>"Justin Bogner" <<a href="mailto:mail@justinbogner.com" target="_blank">mail@justinbogner.com</a>><br><b>Cc: </b>"llvm-commits" <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>><br><b>Sent: </b>Tuesday, May 10, 2016 4:55:08 PM<br><b>Subject: </b>Re: [llvm] r268452 - PM: Port LoopRotation to the new loop pass manager<div><div><br><br><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, May 10, 2016 at 12:46 PM, Justin Bogner <span dir="ltr"><<a href="mailto:mail@justinbogner.com" target="_blank">mail@justinbogner.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div>Sean Silva <<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>> writes:<br>
> On Tue, May 10, 2016 at 11:27 AM, Justin Bogner <<a href="mailto:mail@justinbogner.com" target="_blank">mail@justinbogner.com</a>><br>
> wrote:<br>
><br>
>> Sean Silva <<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>> writes:<br>
>> > On Tue, May 3, 2016 at 3:02 PM, Justin Bogner via llvm-commits <<br>
>> > <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br>
>> ><br>
>> >> Author: bogner<br>
>> >> Date: Tue May 3 17:02:31 2016<br>
>> >> New Revision: 268452<br>
>> >><br>
>> >> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=268452&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=268452&view=rev</a><br>
>> >> Log:<br>
>> >> PM: Port LoopRotation to the new loop pass manager<br>
>> ...<br>
>> >> --- llvm/trunk/test/Transforms/LoopRotate/basic.ll (original)<br>
>> >> +++ llvm/trunk/test/Transforms/LoopRotate/basic.ll Tue May 3 17:02:31<br>
>> 2016<br>
>> >> @@ -1,4 +1,6 @@<br>
>> >> ; RUN: opt -S -loop-rotate < %s | FileCheck %s<br>
>> >> +; RUN: opt -S<br>
>> >><br>
>> -passes='require<loops>,require<targetir>,require<assumptions>,loop(rotate)'<br>
>> >> < %s | FileCheck %s<br>
>> >><br>
>> ><br>
>> > Sorry if this is a stupid question, but why do we need to explicitly<br>
>> > "require" the passes when the loop-rotate already declares the<br>
>> dependency?<br>
>> > (I feel like there's some part of the bigger picture that I'm missing<br>
>> here)<br>
>><br>
>> To be clear, there are no dependencies in the new pass manager - passes<br>
>> request analysis results from an analysis manager and they're calculated<br>
>> or returned from the cache as appropriate. Naturally, the next question<br>
>> one would ask is "okay then, why aren't these calculated on demand when<br>
>> loop-rotate asks for them?".<br>
>><br>
><br>
> Ah, yeah. The whole "schedule vs. cache" is one of the main differences of<br>
> the new PM.<br>
><br>
><br>
>><br>
>> The mechanical answer is that loop-rotate uses the `getCachedResult`<br>
>> API, not the `getResult` one - the difference being that<br>
>> `getCachedResult` only returns results if they're already calculated,<br>
>> and null otherwise. Hence, without the require<> in the test, there are<br>
>> no results.<br>
><br>
><br>
> Ah, makes perfect sense!<br>
><br>
><br>
>> This of course leads to "why does loop-rotate use<br>
>> getCachedResult?"<br>
>><br>
>> It has to. These analyses are function analyses, and a loop pass isn't<br>
>> allowed to cause a function analysis to run, much like a function pass<br>
>> isn't allowed to cause a module analysis to be run. They have to stick<br>
>> to their own level. This enforces correct layering and acts as a<br>
>> safeguard against accidentally doing extra work.<br>
>><br>
>> That said, the "require<loops>" is redundant, since a LoopPassManager<br>
>> can't operate without having calculated that. There may also be an<br>
>> argument to be made that the LPM should implicitly calculate some other<br>
>> analyses, but we'd have to be careful not to do too much.<br>
>><br>
><br>
> Makes sense. But then `+ assert((LI && TTI && AC) && "Analyses for loop<br>
> rotation not available");` is not really appropriate, right? Can't we hit<br>
> that assert just by passing the right arguments to opt? (by dropping the<br>
> `require<*>` analyses from the passes= specification?)<br>
><br>
> (sorry, I'm away from the office and don't have an easy way to test locally)<br>
<br>
</div></div>Yes it does. The other option is to just bail on the transform in that<br>
case, which would be a better option for things like fuzzers and<br>
bisection scripts, but makes it harder to ensure we're doing everything<br>
correctly while we're bringing this up. For now I think the assert is<br>
best, but we may want to revisit this at some point.<br>
</blockquote></div><br></div><div class="gmail_extra">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.</div></div></div></div></blockquote>+1<br><br>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.<br></div></div></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>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").</div></div></div></div></blockquote><div><br></div></div></div><div>I think "cached" is still really important -- it signifies that unlike other query paths it won't go and do work all of a sudden. =]</div><div><br></div><div>That said, I agree that the pattern of requiring a cached result from an outer pass manager is very common and we should streamline this.</div><div><br></div><div>The reason I hadn't actually done it is that I'm actually *really* torn about whether it would be better to have passes just skip their logic or fail hard. I almost *every* case I prefer the assert or fatal error... except when it comes down to bisecting a failing pass or similar techniques.</div></div></div></blockquote><div><br></div><div><br></div><div>How about this: getCachedResults can either return null or fail hard depending on an internal option. The client code shares the same boiler plate -- skips/bails when null is returned from getCachedResult. In normal build, you would get assertion/fail, but bitsecting tools can disable the assert.</div><div><br></div><div>David</div><div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div><br></div><div>-Chandler</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div>-- Sean Silva</div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div style="font-family:arial,helvetica,sans-serif;font-size:10pt;color:#000000"><br> -Hal<span><br><blockquote style="border-left:2px solid rgb(16,16,255);margin-left:5px;padding-left:5px;color:rgb(0,0,0);font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt"><div dir="ltr"><div class="gmail_extra"></div><div class="gmail_extra"><br></div></div>
<br>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br><a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br></blockquote><br><br><br></span><span><font color="#888888">-- <br><div><span name="x"></span>Hal Finkel<br>Assistant Computational Scientist<br>Leadership Computing Facility<br>Argonne National Laboratory<span name="x"></span><br></div></font></span></div></div></blockquote></div></div></div>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></span></div></div>
<br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div></div>