[llvm-commits] [llvm] r162799 - in /llvm/trunk: include/llvm/Analysis/Passes.h include/llvm/Analysis/ProfileDataLoader.h include/llvm/InitializePasses.h include/llvm/LinkAllPasses.h lib/Analysis/Analysis.cpp lib/Analysis/CMakeLists.txt lib/Analysis/ProfileDataLoader.cpp lib/Analysis/ProfileDataLoaderPass.cpp lib/Analysis/ProfileInfo.cpp test/Analysis/Profiling/load-branch-weights-ifs.ll test/Analysis/Profiling/load-branch-weights-loops.ll test/Analysis/Profiling/load-branch-weights-switches.ll
Bill Wendling
wendling at apple.com
Wed Aug 29 14:21:34 PDT 2012
Hi Manman & Alastair,
Comments inline.
-bw
On Aug 28, 2012, at 3:21 PM, Manman Ren <mren at apple.com> wrote:
> Author: mren
> Date: Tue Aug 28 17:21:25 2012
> New Revision: 162799
>
> URL: http://llvm.org/viewvc/llvm-project?rev=162799&view=rev
> Log:
> Profile: set branch weight metadata with data generated from profiling.
>
> This patch implements ProfileDataLoader which loads profile data generated by
> -insert-edge-profiling and updates branch weight metadata accordingly.
>
> Patch by Alastair Murray.
>
> Added: llvm/trunk/include/llvm/Analysis/ProfileDataLoader.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Analysis/ProfileDataLoader.h?rev=162799&view=auto
> ==============================================================================
> --- llvm/trunk/include/llvm/Analysis/ProfileDataLoader.h (added)
> +++ llvm/trunk/include/llvm/Analysis/ProfileDataLoader.h Tue Aug 28 17:21:25 2012
> @@ -0,0 +1,148 @@
> +//===- ProfileDataLoader.h - Load & convert profile info ----*- C++ -*-===//
> +//
> +// The LLVM Compiler Infrastructure
> +//
> +// This file is distributed under the University of Illinois Open Source
> +// License. See LICENSE.TXT for details.
> +//
> +//===----------------------------------------------------------------------===//
> +//
> +// The ProfileDataLoader class is used to load profiling data from a dump file.
> +// The ProfileDataT<FType, BType> class is used to store the mapping of this
> +// data to control flow edges.
> +//
> +//===----------------------------------------------------------------------===//
> +
> +#ifndef LLVM_ANALYSIS_PROFILEDATALOADER_H
> +#define LLVM_ANALYSIS_PROFILEDATALOADER_H
> +
> +#include "llvm/Support/Debug.h"
> +#include "llvm/Support/ErrorHandling.h"
> +#include <vector>
> +#include <string>
> +#include <map>
> +
> +namespace llvm {
> +
> +class ModulePass;
> +class Function;
> +class BasicBlock;
> +
> +// Helpers for dumping edges to dbgs().
> +raw_ostream& operator<<(raw_ostream &O, std::pair<const BasicBlock *,
> + const BasicBlock *> E);
> +raw_ostream& operator<<(raw_ostream &O, const BasicBlock *BB);
> +raw_ostream& operator<<(raw_ostream &O, const Function *F);
> +
> +/// \brief The ProfileDataT<FType, BType> class is used to store the mapping of
> +/// profiling data to control flow edges.
> +///
> +/// An edge is defined by its source and sink basic blocks.
> +template<class FType, class BType>
> +class ProfileDataT {
> + public:
> + // The profiling information defines an Edge by its source and sink basic
> + // blocks.
> + typedef std::pair<const BType*, const BType*> Edge;
> +
> + private:
> + typedef std::map<Edge, unsigned> EdgeWeights;
> +
Is a std::map really necessary here? Could you get away with a DenseMap instead?
> + /// \brief Count the number of times a transition between two blocks is
> + /// executed.
> + ///
> + /// As a special case, we also hold an edge from the null BasicBlock to the
> + /// entry block to indicate how many times the function was entered.
> + std::map<const FType*, EdgeWeights> EdgeInformation;
> +
> + public:
> + static char ID; // Class identification, replacement for typeinfo
> + ProfileDataT() {};
> + ~ProfileDataT() {};
> +
> + /// getFunction() - Returns the Function for an Edge.
> + static const FType* getFunction(Edge e) {
We typically place the '*' next to the function name.
> + // e.first may be NULL
> + assert( ((!e.first) || (e.first->getParent() == e.second->getParent()))
Why the spaces here?
> + && "A ProfileData::Edge can not be between two functions");
> + assert(e.second && "A ProfileData::Edge must have a real sink");
> + return e.second->getParent();
> + }
> +
> + /// getEdge() - Creates an Edge between two BasicBlocks.
> + static Edge getEdge(const BType *Src, const BType *Dest) {
> + return std::make_pair(Src, Dest);
I suspect that you can get away with doing "Edge(Src, Dest)" here instead of std::make_pair. It's a bit more readable.
> + }
> +
> + /// getEdgeWeight - Return the number of times that a given edge was
> + /// executed.
> + unsigned getEdgeWeight(Edge e) const {
> + const FType *f = getFunction(e);
> + assert( (EdgeInformation.find(f) != EdgeInformation.end())
> + && "No profiling information for function");
> + EdgeWeights weights = EdgeInformation.find(f)->second;
> +
> + assert( (weights.find(e) != weights.end())
> + && "No profiling information for edge");
> + return weights.find(e)->second;
> + }
> +
> + /// addEdgeWeight - Add 'weight' to the already stored execution count for
> + /// this edge.
> + void addEdgeWeight(Edge e, unsigned weight) {
> + EdgeInformation[getFunction(e)][e] += weight;
> + }
> +};
> +
> +typedef ProfileDataT<Function, BasicBlock> ProfileData;
> +//typedef ProfileDataT<MachineFunction, MachineBasicBlock> MachineProfileData;
> +
> +/// The ProfileDataLoader class is used to load raw profiling data from the
> +/// dump file.
> +class ProfileDataLoader {
> +private:
> + /// The name of the file where the raw profiling data is stored.
> + const std::string &Filename;
> +
> + /// A vector of the command line arguments used when the target program was
> + /// run to generate profiling data. One entry per program run.
> + std::vector<std::string> CommandLines;
> +
Would a SmallVector work here?
> + /// The raw values for how many times each edge was traversed, values from
> + /// multiple program runs are accumulated.
> + std::vector<unsigned> EdgeCounts;
> +
And here?
>
> Added: llvm/trunk/lib/Analysis/ProfileDataLoader.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/ProfileDataLoader.cpp?rev=162799&view=auto
> ==============================================================================
> --- llvm/trunk/lib/Analysis/ProfileDataLoader.cpp (added)
> +++ llvm/trunk/lib/Analysis/ProfileDataLoader.cpp Tue Aug 28 17:21:25 2012
> @@ -0,0 +1,186 @@
> +//===- ProfileDataLoader.cpp - Load profile information from disk ---------===//
> +//
> +// The LLVM Compiler Infrastructure
> +//
> +// This file is distributed under the University of Illinois Open Source
> +// License. See LICENSE.TXT for details.
> +//
> +//===----------------------------------------------------------------------===//
> +//
> +// The ProfileDataLoader class is used to load raw profiling data from the dump
> +// file.
> +//
> +//===----------------------------------------------------------------------===//
> +
> +#include "llvm/Module.h"
> +#include "llvm/InstrTypes.h"
> +#include "llvm/Analysis/ProfileDataLoader.h"
> +#include "llvm/Analysis/ProfileDataTypes.h"
> +#include "llvm/Support/raw_ostream.h"
> +#include <cstdio>
> +#include <cstdlib>
> +using namespace llvm;
> +
> +namespace llvm {
> +
This namespace shouldn't be necessary. You can just add 'llvm::' to the function name.
> +template<>
> +char ProfileDataT<Function,BasicBlock>::ID = 0;
> +
> +raw_ostream& operator<<(raw_ostream &O, const Function *F) {
> + return O << F->getName();
> +}
> +
> +raw_ostream& operator<<(raw_ostream &O, const BasicBlock *BB) {
> + return O << BB->getName();
> +}
> +
> +raw_ostream& operator<<(raw_ostream &O, std::pair<const BasicBlock *, const BasicBlock *> E) {
> + O << "(";
> +
> + if (E.first)
> + O << E.first;
> + else
> + O << "0";
> +
> + O << ",";
> +
> + if (E.second)
> + O << E.second;
> + else
> + O << "0";
> +
> + return O << ")";
> +}
> +
> +} // namespace llvm
> +
> +/// ByteSwap - Byteswap 'Var' if 'Really' is true. Required when the compiler
> +/// host and target have different endianness.
> +static inline unsigned ByteSwap(unsigned Var, bool Really) {
Gross! What do you mean with the 'Really'? Either call this an expect it to do what it says or don't call it at all.
> + if (!Really) return Var;
> + return ((Var & (255U<< 0U)) << 24U) |
> + ((Var & (255U<< 8U)) << 8U) |
> + ((Var & (255U<<16U)) >> 8U) |
> + ((Var & (255U<<24U)) >> 24U);
> +}
> +
> +/// AddCounts - Add 'A' and 'B', accounting for the fact that the value of one
> +/// (or both) may not be defined.
> +static unsigned AddCounts(unsigned A, unsigned B) {
> + // If either value is undefined, use the other.
> + // Undefined + undefined = undefined.
> + if (A == ProfileDataLoader::Uncounted) return B;
> + if (B == ProfileDataLoader::Uncounted) return A;
> +
> + // Saturate to the maximum storable value. This could change taken/nottaken
> + // ratios, but is presumably better than wrapping and thus potentially
> + // inverting ratios.
> + unsigned long long tmp = (unsigned long long)A + (unsigned long long)B;
Please use something else like uint64_t instead of unsigned long long.
> + if (tmp > (unsigned long long)ProfileDataLoader::MaxCount)
> + tmp = ProfileDataLoader::MaxCount;
> + return (unsigned)tmp;
?? Why the truncation here? Why are you doing math in 'unsigned long long' for 'unsigned' values?
> +}
> +
> +/// ReadProfilingData - Load 'NumEntries' items of type 'T' from file 'F'
> +template <typename T>
> +static void ReadProfilingData(const char *ToolName, FILE *F,
> + std::vector<T> &Data, size_t NumEntries) {
> + // Read in the block of data...
> + if (fread(&Data[0], sizeof(T), NumEntries, F) != NumEntries) {
> + errs() << ToolName << ": profiling data truncated!\n";
> + perror(0);
> + exit(1);
> + }
> +}
> +
> +/// ReadProfilingNumEntries - Read how many entries are in this profiling data
> +/// packet.
> +static unsigned ReadProfilingNumEntries(const char *ToolName, FILE *F,
> + bool ShouldByteSwap) {
> + std::vector<unsigned> NumEntries(1);
Use a SmallVector here please.
> + ReadProfilingData<unsigned>(ToolName, F, NumEntries, 1);
> + return ByteSwap(NumEntries[0], ShouldByteSwap);
Predicate calling ByteSwap on ShouldByteSwap instead of passing ShouldByteSwap in as a parameter.
> +}
> +
> +/// ReadProfilingBlock - Read the number of entries in the next profiling data
> +/// packet and then accumulate the entries into 'Data'.
> +static void ReadProfilingBlock(const char *ToolName, FILE *F,
> + bool ShouldByteSwap,
> + std::vector<unsigned> &Data) {
> + // Read the number of entries...
> + unsigned NumEntries = ReadProfilingNumEntries(ToolName, F, ShouldByteSwap);
> +
> + // Read in the data.
> + std::vector<unsigned> TempSpace(NumEntries);
> + ReadProfilingData<unsigned>(ToolName, F, TempSpace, (size_t)NumEntries);
> +
> + // Make sure we have enough space ...
> + if (Data.size() < NumEntries)
> + Data.resize(NumEntries, ProfileDataLoader::Uncounted);
> +
> + // Accumulate the data we just read into the existing data.
> + for (unsigned i = 0; i < NumEntries; ++i) {
> + Data[i] = AddCounts(ByteSwap(TempSpace[i], ShouldByteSwap), Data[i]);
> + }
> +}
> +
> +/// ReadProfilingArgBlock - Read the command line arguments that the progam was
> +/// run with when the current profiling data packet(s) were generated.
> +static void ReadProfilingArgBlock(const char *ToolName, FILE *F,
Please see about using MemoryBuffer instead of FILE*.
> + bool ShouldByteSwap,
> + std::vector<std::string> &CommandLines) {
> + // Read the number of bytes ...
> + unsigned ArgLength = ReadProfilingNumEntries(ToolName, F, ShouldByteSwap);
> +
> + // Read in the arguments (if there are any to read). Round up the length to
> + // the nearest 4-byte multiple.
> + std::vector<char> Args(ArgLength+4);
Try using SmallVector instead of std::vector when you can.
> + if (ArgLength)
> + ReadProfilingData<char>(ToolName, F, Args, (ArgLength+3) & ~3);
> +
> + // Store the arguments.
> + CommandLines.push_back(std::string(&Args[0], &Args[ArgLength]));
> +}
> +
> +const unsigned ProfileDataLoader::Uncounted = ~0U;
> +const unsigned ProfileDataLoader::MaxCount = ~0U - 1U;
> +
> +/// ProfileDataLoader ctor - Read the specified profiling data file, exiting
> +/// the program if the file is invalid or broken.
> +ProfileDataLoader::ProfileDataLoader(const char *ToolName,
> + const std::string &Filename)
> + : Filename(Filename) {
> + FILE *F = fopen(Filename.c_str(), "rb");
Look into using MemoryBuffer for this instead.
> + if (F == 0) {
> + errs() << ToolName << ": Error opening '" << Filename << "': ";
> + perror(0);
> + exit(1);
Please don't call 'exit' from the library. If you need to do something drastic, use something like 'report_fatal_error'. (Same with the 'exit' call above.)
> Added: llvm/trunk/lib/Analysis/ProfileDataLoaderPass.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/ProfileDataLoaderPass.cpp?rev=162799&view=auto
> ==============================================================================
> --- llvm/trunk/lib/Analysis/ProfileDataLoaderPass.cpp (added)
> +++ llvm/trunk/lib/Analysis/ProfileDataLoaderPass.cpp Tue Aug 28 17:21:25 2012
+/// readEdge - Take the value from a profile counter and assign it to an edge.
+void ProfileMetadataLoaderPass::readEdge(unsigned ReadCount,
+ ProfileData &PB, ProfileData::Edge e,
+ std::vector<unsigned> &Counters) {
Counters doesn't appear to be modified here. If so, please use `ArrayRef' instead.
+ if (ReadCount < Counters.size()) {
Early exit here instead of wrapping the whole thing in the 'if-then' statement.
+ unsigned weight = Counters[ReadCount];
+ assert(weight != ProfileDataLoader::Uncounted);
+ PB.addEdgeWeight(e, weight);
+
+ DEBUG(dbgs() << "-- Read Edge Counter for " << e
+ << " (# "<< (ReadCount) << "): "
+ << PB.getEdgeWeight(e) << "\n");
+ }
+}
> +/// setBranchWeightMetadata - Translate the counter values associated with each
> +/// edge into branch weights for each conditional branch (a branch with 2 or
> +/// more desinations).
> +void ProfileMetadataLoaderPass::setBranchWeightMetadata(Module &M,
> + ProfileData &PB) {
> + for (Module::iterator F = M.begin(), E = M.end(); F != E; ++F) {
> + if (F->isDeclaration()) continue;
> + DEBUG(dbgs() << "Setting branch metadata in '" << F->getName() << "'\n");
> +
> + for (Function::iterator BB = F->begin(), E = F->end(); BB != E; ++BB) {
> + TerminatorInst *TI = BB->getTerminator();
> + unsigned NumSuccessors = TI->getNumSuccessors();
> +
> + // If there is only one successor then we can not set a branch
> + // probability as the target is certain.
> + if (NumSuccessors < 2) continue;
> +
> + // Load the weights of all edges leading from this terminator.
> + DEBUG(dbgs() << "-- Terminator with " << NumSuccessors
> + << " successors:\n");
> + std::vector<uint32_t> Weights(NumSuccessors);
Try using a SmallVector here.
> + for (unsigned s = 0 ; s < NumSuccessors ; ++s) {
> + ProfileData::Edge edge = PB.getEdge(BB, TI->getSuccessor(s));
> + Weights[s] = (uint32_t)PB.getEdgeWeight(edge);
> + DEBUG(dbgs() << "---- Edge '" << edge << "' has weight "
> + << Weights[s] << "\n");
> + }
> +
> + // Set branch weight metadata. This will set branch probabilities of
> + // 100%/0% if that is true of the dynamic execution.
> + // BranchProbabilityInfo can account for this when it loads this metadata
> + // (it gives the unexectuted branch a weight of 1 for the purposes of
> + // probability calculations).
> + MDBuilder MDB(TI->getContext());
> + MDNode *Node = MDB.createBranchWeights(Weights);
> + TI->setMetadata(LLVMContext::MD_prof, Node);
> + NumTermsAnnotated++;
> + }
> + }
> +}
> +
> +bool ProfileMetadataLoaderPass::runOnModule(Module &M) {
> + ProfileDataLoader PDL("profile-data-loader", Filename);
> + ProfileData PB;
> +
> + std::vector<unsigned> Counters = PDL.getRawEdgeCounts();
> +
Why are you making a copy of 'PDL.getRawEdgeCounts()' here? Is this array modified? (It didn't look like it at first...) If it's not modified, please change it into an ArrayRef<unsigned>.
> + unsigned ReadCount = matchEdges(M, PB, Counters);
> +
> + if (ReadCount != Counters.size()) {
> + errs() << "WARNING: profile information is inconsistent with "
> + << "the current program!\n";
> + }
> + NumEdgesRead = ReadCount;
> +
> + setBranchWeightMetadata(M, PB);
> +
> + return ReadCount > 0;
> +}
-bw
More information about the llvm-commits
mailing list