[PATCH] D22141: [BFI] Add new LazyBFI analysis pass

[Adam Nemet via llvm-commits]

> On Jul 12, 2016, at 4:08 PM, Xinliang David Li <davidxl at google.com> wrote:
> 
> 
> 
> On Tue, Jul 12, 2016 at 3:56 PM, Adam Nemet <anemet at apple.com <mailto:anemet at apple.com>> wrote:
> anemet added inline comments.
> 
> ================
> Comment at: include/llvm/Analysis/LazyBlockFrequencyInfo.h:22
> @@ +21,3 @@
> +//
> +// 2. Similarly, getAnalysisUsage should call:
> +//
> ----------------
> davidxl wrote:
> > What is the use model for the new pass manager? Anything worth documenting?
> 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.
> 
> I was going to play with this after this patch and post follow-on patches if anything is required for the new PM.
> 
> Since the vectorizer is converted now, I should be able to try it there.
> 
> 
> ok.  Perhaps add a comment there so that people don't start porting it by accident.

Will do.

>  
> 
> ================
> Comment at: include/llvm/Analysis/LazyBlockFrequencyInfo.h:91
> @@ +90,3 @@
> +/// the file comment for the rules of how to use this properly.
> +class LazyBlockFrequencyInfoPass : public FunctionPass {
> +  LazyBlockFrequencyInfo LBFI;
> ----------------
> davidxl wrote:
> > Can the wrapper class of BFI be 'inlined' into this class so that we don't need LBFI ?
> 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.
> 
> 
> 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.

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.

The class is a single piece of abstraction. It adapts BFI to a lazily computable BFI.

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.

Adam 

>  
> 
> David
> 
> 
> 
> http://reviews.llvm.org/D22141 <http://reviews.llvm.org/D22141>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160712/e39a6a86/attachment.html>