r198640 - CodeGen: Initial instrumentation based PGO implementation

[Eric Christopher]

Hi Justin,

Is it possible to separate out the "instrument to get counters" from
"annotate based on information" parts of the PGO class?

It would be great to see a top level description of the algorithm you're
using to do region creation etc along with an example. It seems that you're
creating regions from a way different than most previous literature on the
topic so a literature link would be good if you've got one, else a
description is going to be key.

Few comments on the code:


> +  PGO.setCurrentRegionCount(0);
>

Should comment this as it's not obvious from reading the surrounding code.

>
> +  Cnt.beginRegion(Builder);
>    eval.begin(*this);
>    LValue lhs = EmitLValue(expr->getTrueExpr());
>    eval.end(*this);
> +  Cnt.adjustFallThroughCount();
>
>
The adjust stuff is wonky enough that you should comment all uses of it. It
isn't obvious why/when/where you'd want to do it. Same with when you apply
adjustments.

Basically if it's going to be a part of any abnormal edge then perhaps it
makes more sense to handle it as part of abnormal edge calculations? What
about the code needs this sort of handling? Ditto with the parent count
subtractions you see at loops.


> +    PGO.setCurrentRegionCount(0);
>

Go ahead document why you're resetting the region count here.


>    // As long as the condition is true, iterate the loop.
> -  if (EmitBoolCondBranch)
> -    Builder.CreateCondBr(BoolCondVal, LoopBody, LoopExit.getBlock());
> +  if (EmitBoolCondBranch) {
> +    Builder.CreateCondBr(BoolCondVal, LoopBody, LoopExit.getBlock(),
> +                         PGO.createBranchWeights(LoopCount, ExitCount));
> +  }
>
>
Don't think the braces are necessary here.

+    uint64_t Total = CaseCnt.getCount() - CaseCnt.getParentCount();
> +    unsigned NCases = Range.getZExtValue() + 1;
> +    // Divide the weights evenly between the cases, ensuring that the
> total
> +    // weight is preserved. Ie, a weight of 5 over three cases will be
> +    // distributed as weights of 2, 2, and 1.
> +    uint64_t Weight = Total / NCases, Rem = Total % NCases;
> +    for (unsigned I = 0; I != NCases; ++I) {
> +      if (SwitchWeights)
> +        SwitchWeights->push_back(Weight + (Rem ? 1 : 0));
> +      if (Rem)
> +        Rem--;
>

Is this related to propagating some difference between parent counts and
"normal" counts?


> -  // If the body of the case is just a 'break', and if there was no
> fallthrough,
> -  // try to not emit an empty block.
> -  if ((CGM.getCodeGenOpts().OptimizationLevel > 0) &&
> +  // If the body of the case is just a 'break', try to not emit an empty
> block.
> +  // If we're profiling or we're not optimizing, leave the block in for
> better
> +  // debug and coverage analysis.
> +  if (!CGM.getCodeGenOpts().ProfileInstrGenerate &&
> +      CGM.getCodeGenOpts().OptimizationLevel > 0 &&
>        isa<BreakStmt>(S.getSubStmt())) {
>      JumpDest Block = BreakContinueStack.back().BreakBlock;
>
>
Is it really worth that much for code gen/optimization time to not just
always have it? Also, do we really want instrumentation to change the cfg
we emit? Also, it doesn't change in debug mode, just without optimization
so that part of the comment isn't quite accurate :)

There's probably a bit more, but this seems like a good place to start
given the questions at the top.

-eric
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140107/e3411da7/attachment.html>