[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 14:49:45 PDT 2015
> 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.
> 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.
> 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.)
> #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).
> + 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.
> +}
> +
> +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?
>
> - 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()`.)
> +
> + 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