[PATCH] D11196: Create a wrapper pass for BlockFrequencyInfo so that block frequency analysis can be done without requiring a pass.

[Cong Hou]

Thank you very much for your review! I have updated the patch
according to your comments.

Cong

On Tue, Jul 14, 2015 at 2:49 PM, Duncan P. N. Exon Smith
<dexonsmith at apple.com> wrote:
>
>> On 2015-Jul-14, at 13:47, Cong Hou <congh at google.com> wrote:
>>
>> congh created this revision.
>> congh added a reviewer: dexonsmith.
>> congh added subscribers: llvm-commits, davidxl.
>>
>> There are already several wrapper passes in LLVM such as LoopInfoWrapperPass and DominatorTreeWrapperPass. The benefit of a wrapper pass is to decouple the analysis from the function pass. This patch creates a wrapper pass for BlockFrequencyInfo. This is useful when we want to do block frequency analysis conditionally (e.g. only in PGO mode) but don't want to add one more pass dependence.
>>
>> http://reviews.llvm.org/D11196
>>
>> Files:
>>  include/llvm/Analysis/BlockFrequencyInfo.h
>>  include/llvm/InitializePasses.h
>>  lib/Analysis/Analysis.cpp
>>  lib/Analysis/BlockFrequencyInfo.cpp
>>  lib/Transforms/Vectorize/LoopVectorize.cpp
>>
>> <D11196.29708.patch>
>
>> Index: lib/Transforms/Vectorize/LoopVectorize.cpp
>> ===================================================================
>> --- lib/Transforms/Vectorize/LoopVectorize.cpp
>> +++ lib/Transforms/Vectorize/LoopVectorize.cpp
>> @@ -1454,7 +1454,7 @@
>>      LI = &getAnalysis<LoopInfoWrapperPass>().getLoopInfo();
>>      TTI = &getAnalysis<TargetTransformInfoWrapperPass>().getTTI(F);
>>      DT = &getAnalysis<DominatorTreeWrapperPass>().getDomTree();
>> -    BFI = &getAnalysis<BlockFrequencyInfo>();
>> +    BFI = &getAnalysis<BlockFrequencyInfoWrapperPass>().getBlockFrequencyInfo();
>
> 80-column?
>
> Looks like there's precedent for `getBFI()` (looking a few lines up, I
> see `getTTI()`).  If you rename the accessors that'll fix the whitespace
> problem here too.

Done. This is now renamed to getBFI().

>
>>      auto *TLIP = getAnalysisIfAvailable<TargetLibraryInfoWrapperPass>();
>>      TLI = TLIP ? &TLIP->getTLI() : nullptr;
>>      AA = &getAnalysis<AliasAnalysis>();
>> Index: lib/Analysis/BlockFrequencyInfo.cpp
>> ===================================================================
>> --- lib/Analysis/BlockFrequencyInfo.cpp
>> +++ lib/Analysis/BlockFrequencyInfo.cpp
>> @@ -105,45 +105,15 @@
>>  } // end namespace llvm
>>  #endif
>>
>> -INITIALIZE_PASS_BEGIN(BlockFrequencyInfo, "block-freq",
>> -                      "Block Frequency Analysis", true, true)
>> -INITIALIZE_PASS_DEPENDENCY(BranchProbabilityInfo)
>> -INITIALIZE_PASS_DEPENDENCY(LoopInfoWrapperPass)
>> -INITIALIZE_PASS_END(BlockFrequencyInfo, "block-freq",
>> -                    "Block Frequency Analysis", true, true)
>> -
>> -char BlockFrequencyInfo::ID = 0;
>> -
>> -
>> -BlockFrequencyInfo::BlockFrequencyInfo() : FunctionPass(ID) {
>> -  initializeBlockFrequencyInfoPass(*PassRegistry::getPassRegistry());
>> -}
>> -
>> -BlockFrequencyInfo::~BlockFrequencyInfo() {}
>> -
>> -void BlockFrequencyInfo::getAnalysisUsage(AnalysisUsage &AU) const {
>> -  AU.addRequired<BranchProbabilityInfo>();
>> -  AU.addRequired<LoopInfoWrapperPass>();
>> -  AU.setPreservesAll();
>> -}
>> -
>> -bool BlockFrequencyInfo::runOnFunction(Function &F) {
>> -  BranchProbabilityInfo &BPI = getAnalysis<BranchProbabilityInfo>();
>> -  LoopInfo &LI = getAnalysis<LoopInfoWrapperPass>().getLoopInfo();
>> +void BlockFrequencyInfo::recalculate(Function *F, BranchProbabilityInfo *BPI,
>> +                                     LoopInfo *LI) {
>
> Can you take these by reference?  They're not really nullable, and I
> think the references make the clear.
>
> If you can get away with it (not sure), might as well change them to
> `const` as well.  BFI shouldn't be modifying anything.

This is both done on this function and doFunction (which is now
renamed to calculate).

>
>>    if (!BFI)
>>      BFI.reset(new ImplType);
>> -  BFI->doFunction(&F, &BPI, &LI);
>> +  BFI->doFunction(F, BPI, LI);
>
> (Feel free to do the same to `doFunction()` -- change it to a reference
> -- in a separate commit if you get a chance.  Also free feel to rename
> it to something like `calculate` or whatever strikes your fancy; the
> `doFunction()` predates me and I don't think much of it.  No need for
> pre-commit review there if you decide to improve those.)

Done.

>
>>  #ifndef NDEBUG
>>    if (ViewBlockFreqPropagationDAG != GVDT_None)
>>      view();
>>  #endif
>> -  return false;
>> -}
>> -
>> -void BlockFrequencyInfo::releaseMemory() { BFI.reset(); }
>> -
>> -void BlockFrequencyInfo::print(raw_ostream &O, const Module *) const {
>> -  if (BFI) BFI->print(O);
>>  }
>>
>>  BlockFrequency BlockFrequencyInfo::getBlockFreq(const BasicBlock *BB) const {
>> @@ -180,3 +150,48 @@
>>  uint64_t BlockFrequencyInfo::getEntryFreq() const {
>>    return BFI ? BFI->getEntryFreq() : 0;
>>  }
>> +
>> +void BlockFrequencyInfo::releaseMemory() { BFI.reset(); }
>> +
>> +void BlockFrequencyInfo::print(raw_ostream &O) const {
>
> I know this is copy/paste, but I slightly prefer `OS` (and have seen
> that more often).

Done.

>
>> +  if (BFI)
>> +    BFI->print(O);
>> +}
>> +
>> +
>> +INITIALIZE_PASS_BEGIN(BlockFrequencyInfoWrapperPass, "block-freq",
>> +                      "Block Frequency Analysis", true, true)
>> +INITIALIZE_PASS_DEPENDENCY(BranchProbabilityInfo)
>> +INITIALIZE_PASS_DEPENDENCY(LoopInfoWrapperPass)
>> +INITIALIZE_PASS_END(BlockFrequencyInfoWrapperPass, "block-freq",
>> +                    "Block Frequency Analysis", true, true)
>> +
>> +char BlockFrequencyInfoWrapperPass::ID = 0;
>> +
>> +
>> +BlockFrequencyInfoWrapperPass::BlockFrequencyInfoWrapperPass()
>> +    : FunctionPass(ID) {
>> +  initializeBlockFrequencyInfoWrapperPassPass(*PassRegistry::getPassRegistry());
>
> Is this duplication, "PassPass", really the way to go?
>
> Should the class just be called `BlockFrequencyInfoWrapper` so that we
> just end up with `initializeBlockFrequencyInfoWrapperPass()`?  If all
> the prior art duplicates "Pass" I guess it's okay, but it reads pretty
> badly to me.

There are already several wrapper passes named in this way:
TargetLibraryInfoWrapperPass, LoopInfoWrapperPass,
CallGraphWrapperPass, and DominatorTreeWrapperPass. I think probably
it is ok to follow this naming tradition.

>
>> +}
>> +
>> +BlockFrequencyInfoWrapperPass::~BlockFrequencyInfoWrapperPass() {}
>> +
>> +void BlockFrequencyInfoWrapperPass::print(raw_ostream &O,
>> +                                          const Module *) const {
>
> I slightly prefer `OS` here too.
>
>> +  BFI.print(O);
>> +}
>> +
>> +void BlockFrequencyInfoWrapperPass::getAnalysisUsage(AnalysisUsage &AU) const {
>> +  AU.addRequired<BranchProbabilityInfo>();
>> +  AU.addRequired<LoopInfoWrapperPass>();
>> +  AU.setPreservesAll();
>> +}
>> +
>> +void BlockFrequencyInfoWrapperPass::releaseMemory() { BFI.releaseMemory(); }
>> +
>> +bool BlockFrequencyInfoWrapperPass::runOnFunction(Function &F) {
>> +  BranchProbabilityInfo &BPI = getAnalysis<BranchProbabilityInfo>();
>> +  LoopInfo &LI = getAnalysis<LoopInfoWrapperPass>().getLoopInfo();
>> +  BFI.recalculate(&F, &BPI, &LI);
>> +  return false;
>> +}
>> Index: include/llvm/Analysis/BlockFrequencyInfo.h
>> ===================================================================
>> --- include/llvm/Analysis/BlockFrequencyInfo.h
>> +++ include/llvm/Analysis/BlockFrequencyInfo.h
>> @@ -21,26 +21,19 @@
>>  namespace llvm {
>>
>>  class BranchProbabilityInfo;
>> +class LoopInfo;
>>  template <class BlockT> class BlockFrequencyInfoImpl;
>>
>>  /// BlockFrequencyInfo pass uses BlockFrequencyInfoImpl implementation to
>>  /// estimate IR basic block frequencies.
>> -class BlockFrequencyInfo : public FunctionPass {
>> +class BlockFrequencyInfo {
>>    typedef BlockFrequencyInfoImpl<BasicBlock> ImplType;
>>    std::unique_ptr<ImplType> BFI;
>>
>>  public:
>> -  static char ID;
>> -
>> -  BlockFrequencyInfo();
>> -
>> -  ~BlockFrequencyInfo() override;
>> +  BlockFrequencyInfo() = default;
>> +  ~BlockFrequencyInfo() = default;
>
> Can you just delete these?

Done.

>
>>
>> -  void getAnalysisUsage(AnalysisUsage &AU) const override;
>> -
>> -  bool runOnFunction(Function &F) override;
>> -  void releaseMemory() override;
>> -  void print(raw_ostream &O, const Module *M) const override;
>>    const Function *getFunction() const;
>>    void view() const;
>>
>> @@ -51,6 +44,9 @@
>>    /// floating points.
>>    BlockFrequency getBlockFreq(const BasicBlock *BB) const;
>>
>> +  /// recalculate - compute block frequency info for the given function.
>> +  void recalculate(Function *F, BranchProbabilityInfo *BPI, LoopInfo *LI);
>> +
>>    // Print the block frequency Freq to OS using the current functions entry
>>    // frequency to convert freq into a relative decimal form.
>>    raw_ostream &printBlockFreq(raw_ostream &OS, const BlockFrequency Freq) const;
>> @@ -60,7 +56,28 @@
>>    raw_ostream &printBlockFreq(raw_ostream &OS, const BasicBlock *BB) const;
>>
>>    uint64_t getEntryFreq() const;
>> +  void releaseMemory();
>> +  void print(raw_ostream &O) const;
>> +};
>> +
>> +/// \brief Legacy analysis pass which computes \c BlockFrequencyInfo.
>> +class BlockFrequencyInfoWrapperPass : public FunctionPass {
>> +  BlockFrequencyInfo BFI;
>>
>> +public:
>> +  static char ID;
>> +
>> +  BlockFrequencyInfoWrapperPass();
>> +  ~BlockFrequencyInfoWrapperPass() override;
>> +
>> +  BlockFrequencyInfo &getBlockFrequencyInfo() { return BFI; }
>> +  const BlockFrequencyInfo &getBlockFrequencyInfo() const { return BFI; }
>
> (These are what I think I'd rename to `getBFI()`.)

Done.

>
>> +
>> +  void getAnalysisUsage(AnalysisUsage &AU) const override;
>> +
>> +  bool runOnFunction(Function &F) override;
>> +  void releaseMemory() override;
>> +  void print(raw_ostream &O, const Module *M) const override;
>>  };
>>
>>  }
>>
>