<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">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><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">
<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 class="im HOEnZb"><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><div class="HOEnZb"><div class="h5">_______________________________________________<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>
</div></div></blockquote></div><br></div></div>