<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Jul 12, 2016, at 4:08 PM, Xinliang David Li <<a href="mailto:davidxl@google.com" class="">davidxl@google.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><br class="Apple-interchange-newline"><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><div class="gmail_quote" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">On Tue, Jul 12, 2016 at 3:56 PM, Adam Nemet<span class="Apple-converted-space"> </span><span dir="ltr" class=""><<a href="mailto:anemet@apple.com" target="_blank" class="">anemet@apple.com</a>></span><span class="Apple-converted-space"> </span>wrote:<br class=""><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;">anemet added inline comments.<br class=""><span class=""><br class="">================<br class="">Comment at: include/llvm/Analysis/LazyBlockFrequencyInfo.h:22<br class="">@@ +21,3 @@<br class="">+//<br class="">+// 2. Similarly, getAnalysisUsage should call:<br class="">+//<br class="">----------------<br class=""></span><span class="">davidxl wrote:<br class="">> What is the use model for the new pass manager? Anything worth documenting?<br class=""></span>I am not sure we will need this class with the new PM. Correct me if I am wrong but I thought as long as the client does not call something like getResult<BlockFrequencyAnalysis>(F), the analysis won't be populated. This is not the case for the old PM.<br class=""><br class="">I was going to play with this after this patch and post follow-on patches if anything is required for the new PM.<br class=""><br class="">Since the vectorizer is converted now, I should be able to try it there.<br class=""></blockquote><div class=""><br class=""></div><div class=""><br class=""></div><div class="">ok. Perhaps add a comment there so that people don't start porting it by accident.</div></div></div></blockquote><div><br class=""></div><div>Will do.</div><br class=""><blockquote type="cite" class=""><div class=""><div class="gmail_quote" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><div class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><span class=""><br class="">================<br class="">Comment at: include/llvm/Analysis/LazyBlockFrequencyInfo.h:91<br class="">@@ +90,3 @@<br class="">+/// the file comment for the rules of how to use this properly.<br class="">+class LazyBlockFrequencyInfoPass : public FunctionPass {<br class="">+ LazyBlockFrequencyInfo LBFI;<br class="">----------------<br class=""></span><span class="">davidxl wrote:<br class="">> Can the wrapper class of BFI be 'inlined' into this class so that we don't need LBFI ?<br class=""></span>It could certainly live nested in the pass, I would rather not remove it though; it nicely captures the single functionality it provides and the abstraction overhead should be removed by the compiler.<br class=""></blockquote><div class=""><br class=""></div><div class=""><br class=""></div><div class="">If new PM port is needed, I see the point of having LBFI to be shared. If not -- i.e., it is only used by the WrapperPass, I don't see the need for it -- the patch will also be smaller. This is not an issue about overhead (which has none). As I mentioned before, the issue is the bloated number of classes related to BFI -- so removing this unnecessary abstraction has value for maintenance reasons.</div></div></div></blockquote><div><br class=""></div><div>I don’t see how this could increase maintenance. The class is an implementation detail and as I said I would be happy to make it private in the pass class if that communicates it better for you.</div><div><br class=""></div><div>The class is a single piece of abstraction. It adapts BFI to a lazily computable BFI.</div><div><br class=""></div><div>The pass is a different abstraction to present this as an analysis pass. I think it’s better coding practice to not mix the different abstractions into the same class, i.e. keep the pass boilerplate separate from the underlying result type of the analysis.</div><div><br class=""></div><div>Adam </div><div><br class=""></div><blockquote type="cite" class=""><div class=""><div class="gmail_quote" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><div class=""> </div><div class=""><br class=""></div><div class="">David</div><div class=""><br class=""></div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><br class=""><br class=""><a href="http://reviews.llvm.org/D22141" rel="noreferrer" target="_blank" class="">http://reviews.llvm.org/D22141</a></blockquote></div></div></blockquote></div><br class=""></body></html>