<div style="line-height:1.7;color:#000000;font-size:14px;font-family:arial"><span style="color: rgb(0, 0, 0); font-family: arial; font-size: 14px; line-height: 1.7; white-space: pre-wrap;">At 2013-07-22 12:16:53,"Tobias Grosser" <<a href="mailto:tobias@grosser.es">tobias@grosser.es</a>> wrote:</span><br><pre style="color: rgb(0, 0, 0); font-family: arial; font-size: 14px; line-height: 1.7;">>On 07/21/2013 07:33 PM, Star Tan wrote:
>> At 2013-07-22 01:40:31,"Tobias Grosser" <<a href="mailto:tobias@grosser.es">tobias@grosser.es</a>> wrote:
>>
>>> On 07/21/2013 09:49 AM, Star Tan wrote:
>>>> Hi all,
>>>>
>>>>
>>>> I have attached a patch file to reduce the polly-detect overhead.
>>>
>>> Great.
>>>
>>>> My idea is to avoid calling TypeFinder in Non-DEBUG mode, so
>>>> TypeFinder is only called in DEBUG mode with the DEBUG macro. This
>>>> patch file did this work with following modifications:
>>>>
>>>>
>>>> First, it keeps most of string information by replacing "<<" with "+" operation. For example, code like this:
>>>> INVALID(CFG, "Non branch instruction terminates BB: " + BB.getName());
>>>> would be converted into:
>>>> LastFailure = "Non branch instruction terminates BB: " + BB.getName().str();
>>>>
>>>>
>>>> Second, it simplifies some complex operations like:
>>>> INVALID(AffFunc,
>>>> "Non affine branch in BB '" << BB.getName() << "' with LHS: "
>>>> << *LHS << " and RHS: " << *RHS);
>>>> into:
>>>> LastFailure = "Non affine branch in BB: " + BB.getName().str();
>>>
>>>
>>>> In such cases, some information for "LHS" and "RHS" are missed.
>>>
>>> Yes. And unfortunately, we may also loose the 'name' of unnamed basic
>>> blocks. Basic blocks without a name get formatted as %111 with '111'
>>> being their sequence number. Your above change will not be able to
>>> derive this number.
>> Yes, but it is much cheaper by using BB.getName().str().
>> Currently, I cannot find a better way to derive unnamed basic block without calling TypeFinder.
>
>Yes, that's the reason why it was used in the first place.
>
>>>> However, it only has little affects on the variable "LastFailure",
>>>> while keeping all information for DEBUG information.
>>>
>>> Why is that? It seems the DEBUG output is the very same that gets
>>> written to "LastFailure".
>> No, they are different. For example, the code like this:
>> INVALID(AffFunc,
>> "Non affine branch in BB '" << BB.getName() << "' with LHS: "
>> << *LHS << " and RHS: " << *RHS);
>> would be converted to:
>> LastFailure = "Non affine branch in BB: " + BB.getName().str();
>> INVALID(AffFunc,
>> LastFailure << "' with LHS: " << *LHS << " and RHS: " << *RHS);
>> You see, information about LHS and RHS would be kept in INVALID DEBUG information.
>> To keep the information about unnamed basic blocks, I think we can rewrite it as:
>> FailString = "Non affine branch in BB: ";
>> INVALID(AffFunc,
>> FailString << BB.getName() << "' with LHS: "
>> << *LHS << " and RHS: " << *RHS);
>> LastFailure = FailString + BB.getName().str();
>>>
>>>> Since the
>>>> variable "LastFailure" is only used in ScopGraphPrinter, which should
>>>> only show critical information in graph, I hope such modification is
>>>> acceptable.
>>>
>>> Why should we only show critical information? In the GraphPrinter we do
>>> not worry about compile time so much, such that we can easily show
>>> helpful information. We just need to make sure that we do not slow down
>>> the compile-time in the generic case.
>> To my knowledge, all of those expensive operations are only used for the variable "LastFailure", which is only used in GraphPrinter. Do you mean the Graph Printer does not execute in the generic case? If that is true, I think we should control those operations for "LastFailure" as follows:
>> if (In GraphPrinter mode) {
>> LastFailure = ...
>> }
>> In this way, we can completely remove those operations for "LastFailure" in the generic case except in GraphPrinter mode.
>
>Yes.
>
>
>> What do you mean with "Normal Execution"? Does the GraphPrinter executes in "normal case"?
>> If GraphPrinter is not executes in "normal case", we should move all operations for "LastFailure" under the condition of "GraphPrinter" mode.
>
>It is not executed in normal mode. So yes, we should move all this under
>the condition of 'GraphPrintingMode'.
>
>>> I am not yet fully sure how the reportInvalid* functions should look
>>> like. However, the part of your patch that is easy to commit without
>>> further discussion is to move the 'return false' out of the INVALID
>>> macro. Meaning, translating the above code to:
>>>
>>> if (checkSomething()) {
>>> INVALID(AffFunc, "Test" << SCEV <<);
>>> return false;
>>> }
>>>
>>> I propose to submit such a patch first and then focus on the remaining
>>> problems.
>> Yes, I agree with you. I have attached a patch file to move "return false" out of INVALID macro.
>
>Great. Committed in r186805.
>
>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.</pre><pre style="color: rgb(0, 0, 0); font-family: arial; font-size: 14px; line-height: 1.7;">For example, currently users can add a new counter by simply adding the following line:</pre><pre style="color: rgb(0, 0, 0); font-family: arial; font-size: 14px; line-height: 1.7;">BADSCOP_STAT(SimpleLoop, "Loop not in -loop-simplify form");</pre><pre style="color: rgb(0, 0, 0); font-family: arial; font-size: 14px; line-height: 1.7;">But if we translate INVALID macro into function calls, then users has to add a function for this counter:</pre><pre style="color: rgb(0, 0, 0); font-family: arial; font-size: 14px; line-height: 1.7;">void INVLIAD_SimpleLoop (...).</pre><pre style="color: rgb(0, 0, 0); font-family: arial; font-size: 14px; line-height: 1.7;">This is because we cannot use the following macro combination in function calls:</pre><pre> if (!Context.Verifying) !
\
++Bad##NAME##ForScop; </pre><pre>So, I do not think it is necessary to translate the INVALID macro into function calls.</pre><pre>Do you still think we should translate INVALID macro into a serial of functions like "<span style="line-height: 1.7;">invalid_CFG, invalid_IndVar, invalid_IndEdge, ... ? In that case, I could provide a small patch file for this purpose -:)</span></pre><pre><br></pre><pre style="color: rgb(0, 0, 0); font-family: arial; font-size: 14px; line-height: 1.7;">> 2) Add checks at the beginning of those function calls and
> continue only if LogErrors is set</pre><pre style="color: rgb(0, 0, 0); font-family: arial; font-size: 14px; line-height: 1.7;">Those invalid log strings are used for two separate cases:</pre><pre style="color: rgb(0, 0, 0); font-family: arial; font-size: 14px; line-height: 1.7;">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:</pre><pre style="color: rgb(0, 0, 0); font-family: arial; font-size: 14px; line-height: 1.7;">if (::llvm::DebugFlag && ::llvm::isCurrentDebugType(TYPE)) </pre><pre style="color: rgb(0, 0, 0); font-family: arial; font-size: 14px; line-height: 1.7;">That means string operations controlled by DEBUG will not execute in normal case, so we should not worry about it.</pre><p!
re><span style="line-height: 1.7;">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:</span></pre><pre>#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)</pre><pre><span style="line-height: 1.7;">As you have suggested, we can construct the condition </span><span style="line-height: 1.7;">GraphViewMode with</span><span style="line-height: 1.7;"> "-polly-show", "-polly-show-only", "</span><span style="line-height: 1.7;">polly-dot" and "polly-dot-only"</span><span style="line-height: 1.7;">. However, I see </span><span style="line-height: 1.7;">all </span><span style="line-height: 1.7;">these options are defined as "static" variables in </span><span style="line-height: 1.7;">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?</span></pre><pre><span style="line-height: 1.7;">Thanks,</span></pre><pre><span style="line-height: 1.7;">Star Tan</span></pre><pre style="color: rgb(0, 0, 0); font-family: arial; font-size: 14px; line-height: 1.7;"><br></pre></div>