<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jun 6, 2016 at 11:59 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">chandlerc added a comment.<br>
<br>
Thanks, this definitely helps.<br>
<span><br>
In <a href="http://reviews.llvm.org/D20560#450661" rel="noreferrer" target="_blank">http://reviews.llvm.org/D20560#450661</a>, @davidxl wrote:<br>
<br>
> New summary of the change:<br>
><br>
> 1. split the legacy LoopAccess Analysis pass out of the header file LoopAccessAnalysis.h<br>
<br></span></blockquote><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>
<br>
</span>Any particular reason to split these apart? It would seem simpler to just do the refactoring in place in the header file...<br>
<span><br></span></blockquote><div><br></div><div>The file LoopAccessAnalysis.h is pretty big so I split out parts that are relevant to pass manager to a new file (where the new pass  will also reside) to reduce dependency.  I don't have strong preference. If you think it is better to keep them together, I can make the change.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>
> 2. introduced a new class LoopAccessFuncInfo that represents the function level analysis results that is cacheble.<br>
<br>
<br>
</span>Makes sense. I'd probably call it LoopAccessInfo to fit with the common pattern, but I'm happy if there are reasons to name this differently.<br>
<span><br></span></blockquote><div><br></div><div>LoopAccessInfo name is already taken for the loop level data structure. I can change it to LoopAccessLoopInfo if you think it is better.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>
> 3. move the interfaces that access the loop level LoopAccessInfo from legacy pass class to the new LoopAccessFuncInfo class<br>
<br>
> 4. change clients to use the new interfaces<br>
<br>
><br>
<br>
>   Does it make sense?<br>
<br>
<br>
</span>Yep. See one question on the code I have initially below...<br>
<br>
<br>
================<br>
Comment at: include/llvm/Analysis/LoopAccessInfoAnalysis.h:36-45<br>
@@ +35,12 @@<br>
+<br>
+// Analysis manager base<br>
+class LoopAccessAMBase {<br>
+public:<br>
+  virtual TargetLibraryInfo *getTLI() = 0;<br>
+  virtual AliasAnalysis *getAAResults() = 0;<br>
+  virtual DominatorTree *getDominatorTree() = 0;<br>
+  virtual LoopInfo *getLoopInfo() = 0;<br>
+  virtual ScalarEvolution *getSCEV() = 0;<br>
+  virtual ~LoopAccessAMBase() {}<br>
+};<br>
+<br>
----------------<br>
Why this pattern?<br>
<br>
Specifically, why deviate from the pattern that other analyses that have been ported use where you just construct the result class with pointers to the various other analysis results they reference?<br>
<br>
Introducing a complete abstract interface seems like a much heavier weight abstraction; unless there is a specific problem solved, I'd rather the lighter weight abstraction.<br></blockquote><div><br></div><div>The problem is that this info depends on 5 different analyses, so I thought encapsulate them into one place make it cleaner </div><div>1) one centralized place for finding the informatin</div><div>2) make the creator interface slightly cleaner.</div><div><br></div><div>I don't have strong preference one way or the other.</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>
<br>
<a href="http://reviews.llvm.org/D20560" rel="noreferrer" target="_blank">http://reviews.llvm.org/D20560</a><br>
<br>
<br>
<br>
</blockquote></div><br></div></div>