-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Daniel Dunbar wrote:> Thanks, applied as r78247. Next? :)Oh my, first he is lagging and now it's all a hurry :-) Okay, here comes the next one. I'm taking a short vacation next week so I will try to prepare 1 or 2 of the next patches so that you can work on them as you have time. Greetings, Andi - -- =========================================================================This email is signed, for more information see http://web.student.tuwien.ac.at/~e0325716/gpg.html -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iEYEARECAAYFAkp9gHUACgkQPiYq0rq7s/AILQCfcuSVw+jSRyX2E2olE4cgbZE7 d0MAnRNj8LtvUJ+/3BpeLj023+5srbVm =5NqX -----END PGP SIGNATURE----- -------------- next part -------------- A non-text attachment was scrubbed... Name: llvm-r78463.zero-entry-edge.double.cleanup.patch Type: text/x-patch Size: 17547 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090808/89977944/attachment.bin>
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Hi! Andreas Neustifter wrote:> Daniel Dunbar wrote: >> Thanks, applied as r78247. Next? :) > > Oh my, first he is lagging and now it's all a hurry :-) > > Okay, here comes the next one. > > I'm taking a short vacation next week so I will try to prepare 1 or 2 of > the next patches so that you can work on them as you have time.As promised, here is the next one. Greetings, Andi - -- =========================================================================This email is signed, for more information see http://web.student.tuwien.ac.at/~e0325716/gpg.html -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iEYEARECAAYFAkp9h5sACgkQPiYq0rq7s/BmcgCfRlOfTNMnwpsf70SpQPaqMSh3 RdMAniuAD3HNnaydQbfgqnfHycMdth45 =uDtb -----END PGP SIGNATURE----- -------------- next part -------------- A non-text attachment was scrubbed... Name: llvm-r78463.profile-estimator.patch Type: text/x-patch Size: 10920 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090808/709ab0fb/attachment.bin>
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Hi! Andreas Neustifter wrote:> > Daniel Dunbar wrote: >> >> Thanks, applied as r78247. Next? :) > > > > Oh my, first he is lagging and now it's all a hurry :-) > > > > Okay, here comes the next one. > > > > I'm taking a short vacation next week so I will try to prepare 1 or 2 of > > the next patches so that you can work on them as you have time.And another one. Greetings, Andi - -- =========================================================================This email is signed, for more information see http://web.student.tuwien.ac.at/~e0325716/gpg.html -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iEYEARECAAYFAkp9lTUACgkQPiYq0rq7s/A5ZQCglyP2HuapIVTRjakypNmi9Ytd vlUAnRBGijUWx/Py/VKlhxJvLGwHtfS0 =wv5q -----END PGP SIGNATURE----- -------------- next part -------------- A non-text attachment was scrubbed... Name: llvm-r78470.small.cleanup.patch Type: text/x-patch Size: 4999 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090808/675d897d/attachment.bin>
Applied as r78477. I do have a few comments on the patch, below, but I'll wait to see where things are going before acting on them. :) --> Index: include/llvm/Analysis/ProfileInfo.h > ================================================================== > + // MissingValue - The value that is returned for execution counts in case > + // no value is available. > + static const int MissingValue = -1;This should be a double?> + // getFunction() - Returns the Function for an Edge, checking for validity. > + static const Function* getFunction(Edge e) { > + assert(e.second && "Invalid ProfileInfo::Edge"); > + return e.second->getParent(); > + } > + > + // getEdge() - Creates an Edge from two BasicBlocks. > + static Edge getEdge(const BasicBlock* Src, const BasicBlock* Dest) { > + return std::make_pair(Src,Dest); > + }I think these should be private; eventually they are just an implementation detail of a particular ProfileInfo provider.> - // We don't want to create an std::set unless we are dealing with a block that > - // has a LARGE number of in-edges. Handle the common case of having only a > - // few in-edges with special code.Thank you for killing this premature optimization. :)> + // the sum of the edge frequencies from the incoming edges.the -> The> std::set<const BasicBlock*> ProcessedPreds; > - ProcessedPreds.insert(FirstPred); > - ProcessedPreds.insert(SecondPred); > - ProcessedPreds.insert(ThirdPred); > + double Count = 0; > for (; PI != PE; ++PI) > - if (ProcessedPreds.insert(*PI).second) > - Count += getEdgeWeight(*PI, BB); > + if (ProcessedPreds.insert(*PI).second) { > + double w = getEdgeWeight(getEdge(*PI, BB)); > + if (w == MissingValue) { > + Count = MissingValue; break;Please break the line before 'break'. This code also makes me thing MissingValue should be renamed, MissingValue is something like "NoData", whereas MissingValue could easily be interpreted as NeverExecuted. I think there should probably be a comment in the doc for this method that the count goes to MissingValue if any edge is missing data. -- Thanks, - Daniel On Sat, Aug 8, 2009 at 6:41 AM, Andreas Neustifter<e0325716 at student.tuwien.ac.at> wrote:> -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > Daniel Dunbar wrote: >> Thanks, applied as r78247. Next? :) > > Oh my, first he is lagging and now it's all a hurry :-) > > Okay, here comes the next one. > > I'm taking a short vacation next week so I will try to prepare 1 or 2 of > the next patches so that you can work on them as you have time. > > Greetings, Andi > > - -- > =========================================================================> This email is signed, for more information see > http://web.student.tuwien.ac.at/~e0325716/gpg.html > -----BEGIN PGP SIGNATURE----- > Version: GnuPG v1.4.9 (GNU/Linux) > Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org > > iEYEARECAAYFAkp9gHUACgkQPiYq0rq7s/AILQCfcuSVw+jSRyX2E2olE4cgbZE7 > d0MAnRNj8LtvUJ+/3BpeLj023+5srbVm > =5NqX > -----END PGP SIGNATURE----- > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu 聽 聽 聽 聽 http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev > >
Applied as r78484, with some minor tweaks. A few nits: --> --- a/include/llvm/Analysis/LoopInfo.h > +++ b/include/llvm/Analysis/LoopInfo.h > @@ -213,6 +213,25 @@ public: > return 0; > } > > + /// getExitEdges - Return all pairs of (_inside_block_,_outside_block_). > + /// (Modelled after getExitingBlocks().)Can you change this comment to be freestanding? You don't need to explain what it was modeled after, just explain what it does.> @@ -0,0 +1,233 @@ > +//===- ProfileEstimatorPass.cpp - LLVM Pass to estimate profile info ------===// > +// > +// The LLVM Compiler Infrastructure > +// > +// This file is distributed under the University of Illinois Open Source > +// License. See LICENSE.TXT for details. > +// > +//===----------------------------------------------------------------------===// > +// > +// This file implements a concrete implementation of profiling information that > +// estimates the profiling information in a very crude and unimaginative way. > +// > +//===----------------------------------------------------------------------===//Cool! One great thing about this is we can now test things like llvm-prof without having to generate sampled profile data. It would be nice to push that along, if only to have to sanity check subsequent patches. Also, there are some obvious FIXMEs that are probably worth putting in the code, for example it would be nice to do some estimates on the relative frequency of invoke destinations.> +static cl::opt<double> > +ProfileInfoExecCount( > + "profile-estimator-loop-weight", cl::init(10), > + cl::value_desc("loop-weight"), > + cl::desc("Number of loop executions used for profile-estimator") > +);This variable should be renamed to match the option.> + // createProfileEstimatorPass - This function returns a Pass that estimates > + // profiling information using the given loop execution count.// -> ///, for doxygen.> +#define EDGE_ERROR(V1,V2) \ > + DEBUG(DOUT<<"-- Edge ("<<(V1)->getName()<<","<<(V2)->getName() \ > + <<") is not calculated, returning\n"); > + > +#define EDGE_WEIGHT(E) \ > + DEBUG(DOUT<<"-- Weight of Edge ("<<((E).first?(E).first->getName():"0") \ > + <<","<<(E).second->getName()<<"):"<<getEdgeWeight(E)<<"\n");Ok. I would just make these inline functions for readability. I did switch the to use errs() instead of DOUT.> +// recurseBasicBlock() - This calculates the ProfileInfo estimation for a > +// single block and then recurses into the successors. > +void ProfileEstimatorPass::recurseBasicBlock(BasicBlock *BB) { > + > + // break recursion if already visitedNit: Please make this a well formed sentence, i.e. capitalize and add a period. Same for a bunch of other comments... :)> + // if block is an loop header, first subtract all weigths from edges that > + // exit this loop, then distribute remaining weight on to the edges exiting > + // this loop. finally the weight of the block is increased, to simulate > + // several executions of this loopweigths -> weights> +bool ProfileEstimatorPass::runOnFunction(Function &F) { > + if (F.isDeclaration()) return false; > + > + LI = &getAnalysis<LoopInfo>(); > + FunctionInformation.erase(&F); > + BlockInformation[&F].clear(); > + EdgeInformation[&F].clear(); > + BBisVisited.clear(); > + > + DEBUG(DOUT<<"Working on function "<<F.getName()<<"\n"); > + > + // since the entry block is the first one and has no predecessors, the edge > + // (0,entry) is inserted with the starting weight of 1 > + BasicBlock *entry = &F.getEntryBlock(); > + BlockInformation[&F][entry] = 1; > + > + Edge edge = getEdge(0,entry); > + EdgeInformation[&F][edge] = 1; EDGE_WEIGHT(edge); > + recurseBasicBlock(entry);I was hoping ProfileInfo would be be an abstract base class by now so this pass could didn't have to convert information into the format ProfileInfo is using to store things. In particular, it would be nice to eventually make this pass not compute anything until someone asks. Of course we can always fix this once things have settled. -- - Daniel On Sat, Aug 8, 2009 at 7:11 AM, Andreas Neustifter<e0325716 at student.tuwien.ac.at> wrote:> -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > Hi! > > Andreas Neustifter wrote: >> Daniel Dunbar wrote: >>> Thanks, applied as r78247. Next? :) >> >> Oh my, first he is lagging and now it's all a hurry :-) >> >> Okay, here comes the next one. >> >> I'm taking a short vacation next week so I will try to prepare 1 or 2 of >> the next patches so that you can work on them as you have time. > > As promised, here is the next one. > > Greetings, Andi > > - -- > =========================================================================> This email is signed, for more information see > http://web.student.tuwien.ac.at/~e0325716/gpg.html > -----BEGIN PGP SIGNATURE----- > Version: GnuPG v1.4.9 (GNU/Linux) > Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org > > iEYEARECAAYFAkp9h5sACgkQPiYq0rq7s/BmcgCfRlOfTNMnwpsf70SpQPaqMSh3 > RdMAniuAD3HNnaydQbfgqnfHycMdth45 > =uDtb > -----END PGP SIGNATURE----- > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev > >
Applied as r78485. - Daniel On Sat, Aug 8, 2009 at 8:09 AM, Andreas Neustifter<e0325716 at student.tuwien.ac.at> wrote:> -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > Hi! > > Andreas Neustifter wrote: >> > Daniel Dunbar wrote: >>> >> Thanks, applied as r78247. Next? :) >> > >> > Oh my, first he is lagging and now it's all a hurry :-) >> > >> > Okay, here comes the next one. >> > >> > I'm taking a short vacation next week so I will try to prepare 1 or 2 of >> > the next patches so that you can work on them as you have time. > > And another one. > > Greetings, Andi > > - -- > =========================================================================> This email is signed, for more information see > http://web.student.tuwien.ac.at/~e0325716/gpg.html > -----BEGIN PGP SIGNATURE----- > Version: GnuPG v1.4.9 (GNU/Linux) > Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org > > iEYEARECAAYFAkp9lTUACgkQPiYq0rq7s/A5ZQCglyP2HuapIVTRjakypNmi9Ytd > vlUAnRBGijUWx/Py/VKlhxJvLGwHtfS0 > =wv5q > -----END PGP SIGNATURE----- > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev > >
Hi! Daniel Dunbar wrote:> Applied as r78477. > > I do have a few comments on the patch, below, but I'll wait to see > where things are going before acting on them. :) > > -- > >> Index: include/llvm/Analysis/ProfileInfo.h >> ==================================================================> >> + // MissingValue - The value that is returned for execution counts in case >> + // no value is available. >> + static const int MissingValue = -1; > > This should be a double?Yes it should be. Since C++ (for some reason I can not fathom) does not allow to use a static const float this was my workaround. Is there a better solution?>> + // getFunction() - Returns the Function for an Edge, checking for validity. >> + static const Function* getFunction(Edge e) { >> + assert(e.second && "Invalid ProfileInfo::Edge"); >> + return e.second->getParent(); >> + } >> + >> + // getEdge() - Creates an Edge from two BasicBlocks. >> + static Edge getEdge(const BasicBlock* Src, const BasicBlock* Dest) { >> + return std::make_pair(Src,Dest); >> + } > > I think these should be private; eventually they are just an > implementation detail of a particular ProfileInfo provider.My plan was to have Edges as integral part of the ProfileInfo since they only store information for a given CFG. An provider of ProfileInformation can then decide at which level information is provided. If possible the ProfileInfo calculates other information from that. (E.g. calculate block counts from edge counts.) This has the advantage that not every ProfileInfo provider has to reimplement the logic for deriving additional information.>> - // We don't want to create an std::set unless we are dealing with a block that >> - // has a LARGE number of in-edges. Handle the common case of having only a >> - // few in-edges with special code. > > Thank you for killing this premature optimization. :)Well, when not caching the results it was good I guess. But with a results cache thats not necessary any more.>> + // the sum of the edge frequencies from the incoming edges. > > the -> TheEven if its part of an sentence? (See lib/Analysis/ProfileInfo.cpp, line 49.)> This code also makes me thing MissingValue should be renamed, > MissingValue is something like "NoData", whereas MissingValue could > easily be interpreted as NeverExecuted. I think there should probably > be a comment in the doc for this method that the count goes to > MissingValue if any edge is missing data.I will think about how to solve this best. Thank you so much! Andi