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

[Xinliang David Li via llvm-commits]

On Tue, Jul 12, 2016 at 5:29 PM, Adam Nemet <anemet at apple.com> wrote:

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

Making it private sounds good.

thanks,

David


>
> 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
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160712/7263c49a/attachment-0001.html>