[LLVMdev] Analysis of polly-detect overhead in oggenc

[Tobias Grosser]

On 07/22/2013 01:46 AM, Star Tan wrote:
> At 2013-07-22 12:16:53,"Tobias Grosser" <tobias at grosser.es> wrote:
>> I propose two more patches:
>>
>> 	1) Transform the INVALID macro into function calls, that format
>>             the text and that set LastFailure.
> Translating the INVALID macro into function calls would complicate the operations for different counters.
> For example, currently users can add a new counter by simply adding the following line:
> BADSCOP_STAT(SimpleLoop, "Loop not in -loop-simplify form");
> But if we translate INVALID macro into function calls, then users has to add a function for this counter:
> void INVLIAD_SimpleLoop (...).                                                    \

        ^^ No uppercase in function names.

> This is because we cannot use the following macro combination in function calls:
>      if (!Context.Verifying)                      \
>        ++Bad##NAME##ForScop;
> So, I do not think it is necessary to translate the INVALID macro into function calls.
> Do you still think we should translate INVALID macro into a serial of functions like "invalid_CFG, invalid_IndVar, invalid_IndEdge, ...  ? In that case, I could provide a small patch file for this purpose -:)

I think it would still be nice to get rid of this macro. We could 
probably have a default function that takes an enum to report different
errors in the reportInvalid(enum errorKind) style. And then several 
others that would allow more complex formatting (e.g. 
reportInvalidAlias(AliasSet)). Especially the code after 'isMustAlias()' 
would be nice to move out of the main scop detection.

However, this issue is not directly related to the speedup work, so
you are welcome to skip it for now.

(Btw. thanks for not blindly following my suggestions!)

>>          2) Add checks at the beginning of those function calls and
>>             continue only if LogErrors is set
> Those invalid log strings are used for two separate cases:
> 1) The first case is for detailed debugging, which is controlled by the macro DEBUG(dbgs() << MESSAGE). In such a case, string operations will automatically skipped in normal execution mode with the following if-statement:
> if (::llvm::DebugFlag && ::llvm::isCurrentDebugType(TYPE))
> That means string operations controlled by DEBUG will not execute in normal case, so we should not worry about it.
> 2) The other case is for the variable "LastFailure", which is used only in GraphPrinter. Currently string operations for "LastFailure" always execute in normal cases.  My idea is to put such string operations under the condition of "GraphPrinter" mode. For example, I would like to translate the "INVALID" macro into:
> #define INVALID(NAME, MESSAGE)                                                 \
>    do {                                                                         \
>      if (GraphViewMode) {                                                       \
>        std::string Buf;                                                         \
>        raw_string_ostream fmt(Buf);                                             \
>        fmt << MESSAGE;                                                          \
>        fmt.flush();                                                             \
>        LastFailure = Buf;                                                       \
>      }                                                                          \
>      DEBUG(dbgs() << MESSAGE);                                                  \
>      DEBUG(dbgs() << "\n");                                                     \
>      assert(!Context.Verifying &&#NAME);                                        \
>      if (!Context.Verifying)                                                    \
>        ++Bad##NAME##ForScop;                                                    \
>    } while (0)

Looks good.

> As you have suggested, we can construct the condition GraphViewMode with "-polly-show", "-polly-show-only", "polly-dot" and "polly-dot-only". However, I see all  these options  are defined as "static" variables in lib/RegisterPasses.cpp.  Do you think I should translate these local variables into global variables or should I define another option like "-polly-dot-scop" in ScopDetection.cpp?

You can define a new option -polly-detect-collect-errors that enables 
the error tracking. Adding cl::location to this option allows you to
store the option value externally. You can use this to automatically
set this option, in case in lib/RegisterPasses.cpp in case -polly-show, 
-polly-show-only, ... have been set.

Cheers,
Tobias