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

[Xinliang David Li via llvm-commits]

On Tue, Jul 12, 2016 at 3:56 PM, Adam Nemet <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.


>
> ================
> 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.


David


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