[llvm-commits] [llvm] r80666 - in /llvm/trunk: include/llvm/Analysis/Passes.h include/llvm/LinkAllPasses.h lib/Analysis/ProfileVerifierPass.cpp

[Daniel Dunbar]

On Tue, Sep 1, 2009 at 1:48 AM, Andreas Neustifter<astifter at gmx.at> wrote:
> Author: astifter
> Date: Tue Sep  1 03:48:42 2009
> New Revision: 80666
>
> URL: http://llvm.org/viewvc/llvm-project?rev=80666&view=rev
> Log:
> Preparation for Optimal Edge Profiling:
> This adds a pass to verify the current profile against the flow conditions.
> This is very helpful when later on trying to perserve the profiling information
> during all passes.

Nice!

> +static bool DisableAssertions = false;
> +static cl::opt<bool,true>
> +ProfileVerifierDisableAssertions("profile-verifier-noassert",
> +    cl::location(DisableAssertions), cl::desc("Disable assertions"));
> +bool PrintedDebugTree = false;

Can the option be an argument to createProfileVerifierPass instead?
Also the global variable PrintedDebugTree shouldn't be here, can it
just be a member variable?

> +#ifndef NDEBUG
> +void ProfileVerifierPass::printDebugInfo(const BasicBlock *BB) {
...
> +void ProfileVerifierPass::debugEntry (const BasicBlock* BB, double w,
> +                                      double inw,  int inc, double outw, int
> +                                      outc, double d) {
> +  DEBUG(errs()<<"TROUBLE: Block "<<BB->getNameStr()<<" in "<<BB->getParent()->getNameStr()
> +      <<",BBWeight="<<w<<",inWeight="<<inw<<",inCount="<<inc<<",outWeight="
> +      <<outw<<",outCount"<<outc<<"\n");
...
> +#endif

I would recommend avoiding hiding the definitions of functions with
NDEBUG, except in the particular case where the code adds greatly to
executable size (we try to avoid using the preprocessor as much as
possible). Also, not that the DEBUG macro is unnecessary inside these
functions (unless you just wanted the --debug-only behavior). I would
recommend just having the print/debug routine print/debug, and have
the caller decide what to print/debug.

> +#define CHECK(C,M) \
> +if (C) { \
> +  if (DisableAssertions) { errs()<<(M)<<"\n"; } else { assert((!(C)) && (M)); } \
> +}
> +
> +#define CHECKDEBUG(C,M,D) \
> +if (C) { \
> +  DEBUG(debugEntry(BB, BBWeight, inWeight,  inCount, \
> +                                 outWeight, outCount, (D))); \
> +  if (DisableAssertions) { errs()<<(M)<<"\n"; } else { assert((!(C)) && (M)); } \
> +}

Can you style this code a bit more like the LLVM Verifier
(lib/VMCore/Verifier.cpp). That is, use a CheckFailed sort of function
to hold more of the logic, and only use macros (like Assert1, ...) to
essentially wrap the assert.

Thanks,
 - Daniel