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
Hi, here's a patch for a minor issue I stumbled upon today, which may prevent some profiling info files from being loaded (on win32 -- I doubt that other systems are affected). Martin -------------- next part -------------- A non-text attachment was scrubbed... Name: llvm-r79697-profileinfoloader.patch Type: application/octet-stream Size: 616 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090822/7aba7e72/attachment.obj>
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 I will try to get this in, thanks! M. Tofall wrote:> Hi, > > here's a patch for a minor issue I stumbled upon today, which may > prevent some profiling info files from being loaded (on win32 -- I > doubt that other systems are affected). > > Martin > > > ------------------------------------------------------------------------ > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev- -- =========================================================================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 iEYEARECAAYFAkqSZI4ACgkQPiYq0rq7s/CBzwCfbRGET+E4wBnvFIPBNaWnfchv vSQAn2g/0XPgWWtIUeyICq6+aOJ4NmaN =1z9U -----END PGP SIGNATURE-----
On Tue, Aug 18, 2009 at 4:07 AM, Andreas Neustifter<e0325716 at student.tuwien.ac.at> wrote:>>> + // 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.Ok. It may still be nice to have this in a separate interface (one that would only be used by ProfileInfo implementations, not by ProfileInfo clients), just so that ProfileInfo itself has a small and clear interface. Not important for now though...>>> + // the sum of the edge frequencies from the incoming edges. >> >> the -> The > > Even if its part of an sentence? (See lib/Analysis/ProfileInfo.cpp, line 49.)Ah, missed that. I personally like '// Some trailing off thought ...' <more code> '// ... continue the comment'. - Daniel