[PATCH] D11196: Create a wrapper pass for BlockFrequencyInfo so that block frequency analysis can be done without requiring a pass.
Duncan P. N. Exon Smith
dexonsmith at apple.com
Tue Jul 14 16:14:37 PDT 2015
> 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!
>
> 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;
>>> };
>>>
>>> }
More information about the llvm-commits
mailing list