[LLVMdev] Analysis of polly-detect overhead in oggenc

[Tobias Grosser]

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.

> 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".

> 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.

> Results show that it has almost the same performance improvements as
> my previous hack patch file, i.e., reducing the compile-time of
> Polly-detect pass from 90s (>80%) to 0.5s (2.5%) when compiling oggenc
> 16X.

Great.

> Postscript to  Tobias:
>   I have also implemented your initial proposal, which uses some class
>   hierarchy to show different Failure information. Unfortunately, I
>   found it would significantly complicate the source code. It not only
>   introduces a lot of dynamic object "new" and "copy" operations, but
>   also makes source code hard to understand. I argue that very
>   detailed information for the variable "LastFailure" is not essential
>   because it should only show critical information in the graph. If
>   users want to know detailed failure information, they should refer
>   to DEBUG information. This new patch file keeps most of critical
>   information for "LastFailure" except some detailed information about
>   Instruction and SCEV.

Interesting. This was also something I was afraid of. Passing new/delete
stuff through the scop detection is probably not what we want.

> Do you have any suggestion?

I do.

Your patch goes in the right direction and it helped to get a better
idea of what should be done. I think the first point that we learned is
that passing class pointers around is probably too distrubing. I also
believe having the formatting of the error messages in the normal scop
detection is not great, as it both adds unrelated stuff to the code, but
also makes it harder to conditionally disable the error reporting. On
the other side, I believe it is good to make the 'return false'
explicit.

Hence, I propose to transform the code in something like the following:

Instead of

  if (checkSomething())
    INVALID(AffFunc, "Test" << SCEV <<);

we should get something like:

  if (checkSomething()) {
    reportInvalidAffFunc(SCEV);
    return false;
  }

The reportInvalidAffFunc is then either a NO-OP (during normal
execution) or it reports the error (in case we need it).

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.

Cheers,
Tobias