<div dir="ltr">Ok. Given the large difference between the legacy and new PM in this design, I will directly jump into the new PM porting without going through the refactoring first. When the patch is ready, we then decide if the patch needs to be split into two parts.<div><br></div><div>thanks,</div><div><br></div><div>David</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jun 8, 2016 at 6:54 PM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@gmail.com" target="_blank">chandlerc@gmail.com</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"><span class=""><div dir="ltr">On Wed, Jun 8, 2016 at 6:49 PM Xinliang David Li 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 Wed, Jun 8, 2016 at 6:34 PM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@gmail.com" target="_blank">chandlerc@gmail.com</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"><span><div dir="ltr">On Wed, Jun 8, 2016 at 6:31 PM Xinliang David Li <<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.com</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">Let me know if you want this change to be reverted. It does look like an incremental improvement so I guess not.</div></blockquote><div><br></div></span><div>Not really sure.</div><div><br></div><div>I think you'll essentially end up reverting some of the changes here because it doest (IMO) make sense to have the function-level result split out this way. I'm not sure how much though.</div></div></div></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>This change is NFC -- it simply wraps the the original Cache (DenseMap) into a wrapper class with nicer interfaces, nothing more (and pretends to be function level analysis result while in fact it is just a cache to the loop level analysis result).</div></div></div></div></blockquote><div><br></div></span><div>Yes, but I don't think this really adds anything but boilerplate *unless* it was necessary for the new pass manager. Since it shouldn't be (because we can instead use the pass managers LoopAnalysisManager directly), I don't think having two classes is helpful here.</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>Can you be specific on what is 'function level result split out this way'?</div></div></div></div></blockquote><div><br></div></span><div>The DenseMap being split into the LoopAccessAnalysisResult object, separate from the LoopAccessAnalysis function analysis pass.</div><div><br></div><div>If in the new pass manager this can be a *loop* analysis pass and the LoopAccessInfo directly provided as the result, I think the legacy pass manager won't really benefit from this separation.</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> </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>Maybe it would make sense to juts fold the result stuff back into the analysis manager but try to keep any thing in the LoopAccessInfo that really belonged there?</div></div></div></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>I have hard time parsing this request too :) Which result and analysis manager?</div></div></div></div></blockquote><div><br></div></span><div>The LoopAccessAnalysisResult added in this patch. The "analysis manager" was a typo on my part, I meant to say "pass" referring to the LoopAccessAnalysis which will eventually become a LoopAccessWrapperPass or some such for the legacy pass manager.</div><div><div class="h5"><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>thanks,</div><div><br></div><div>David</div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><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><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><br></div><div>David</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jun 8, 2016 at 6:29 PM, Xinliang David Li <span dir="ltr"><<a href="mailto:xinliangli@gmail.com" target="_blank">xinliangli@gmail.com</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"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Wed, Jun 8, 2016 at 6:09 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">chandlerc added a comment.<br>
<br>
At a meta level, if I've started reviewing a patch, please wait until I have a chance to see your updates before submitting it. I'm sorry I didn't get to it in the early morning, but its still well under 24h turn around and I don't know that its reasonable to expect lower latency.<br>
<br>
That's not to say that I don't appreciate all of Adam and Davide's comments -- they're help reviewing is much appreciated -- but as it happens I don't actually think this is the right approach.<br></blockquote><div><br></div></span><div>Ah -- you had initial comments saying this makes sense -- thus I assumed it is fine with lgtm.</div><div><br></div><div>What you said below makes sense. I will rework on the patch to use LoopAnalysisManager to see if works.</div><div><br></div><div>thanks,</div><div><br></div><div>David</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>
<br>
See below for more details.<br>
<br>
<br>
================<br>
Comment at: llvm/trunk/include/llvm/Analysis/LoopAccessAnalysis.h:704-706<br>
@@ +703,5 @@<br>
+private:<br>
+ /// \brief LoopAccessInfo is created on demand. This map caches<br>
+ /// the computed results.<br>
+ DenseMap<Loop *, std::unique_ptr<LoopAccessInfo>> LoopAccessInfoMap;<br>
+<br>
----------------<br>
This doesn't seem like the right design. It leads to the `getInfo().getInfo()` awkward pattern that IMO is only marginally less awkward spelled as `getResult().getInfo()`.<br>
<br>
Instead, I think this pretty clearly wants to be a Loop analysis in the new pass manager. I think that the caching and map should be the new pass manager's caching and map, rather than rolling our own in a function analysis manager.<br>
<br>
As a consequence, I don't think you need this result type at all. I think the LoopAccessInfo is already the correct result type. I think you'll need the DenseMap and logic around it in the legacy pass manager's function analysis, and you'll want a different query path to use the LoopAnalysisManager directly in the new pass manager.<br>
<br>
Does that make sense?<br>
<span><br>
<br>
Repository:<br>
rL LLVM<br>
<br>
<a href="http://reviews.llvm.org/D20560" rel="noreferrer" target="_blank">http://reviews.llvm.org/D20560</a><br>
<br>
<br>
<br>
</span></span><span><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>
</div></div></span></blockquote></div><br></div></div>
</blockquote></div><br></div>
</blockquote></div></div></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></div></div></div></div>
</blockquote></div><br></div>