<div dir="ltr"><br><div class="gmail_extra"><br><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 class=""><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>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><br></div><div>Can you be specific on what is 'function level result split out this way'?</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>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>I have hard time parsing this request too :) Which result and analysis manager?</div><div><br></div><div>thanks,</div><div><br></div><div>David</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><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><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><br></div></div>