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

[Cong Hou]

On Tue, Jul 14, 2015 at 4:14 PM, Duncan P. N. Exon Smith
<dexonsmith at apple.com> wrote:
>
>> On 2015-Jul-14, at 15:45, Cong Hou <congh at google.com> wrote:
>>
>> Thank you very much for your review! I have updated the patch
>> according to your comments.
>>
>> Cong
>
> The renaming/etc. of `doFunction()` should be a separate commit.
>
> Assuming you split that up correctly, this LGTM.  Thanks!

OK. I have split it out from this patch. Thank you very much for your review!


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;
>>>> };
>>>>
>>>> }
>