Add 'cold' attribute to functions

[Duncan Sands]

Hi Diego,

On 15/05/13 17:31, Diego Novillo wrote:
> On 2013-04-10 17:33 , Diego Novillo wrote:
>> This patch adds a new function attribute 'cold' to functions.
>
> Thanks for all the feedback.  In fixing up the patch I noticed that there is a
> 'bindings/' directory where attributes seem to play a role. I'm not sure what
> this is.  Not all the existing attributes seem to exist in these bindings.
>
> Should I add the following changes as well?  There don't seem to be tests for
> these files (nothing I could find in a quick search, anyway).
>
> Main patch is attached.  OK to commit?


> diff --git a/docs/LangRef.rst b/docs/LangRef.rst
> index 4d006f1..1c0864d 100644
> --- a/docs/LangRef.rst
> +++ b/docs/LangRef.rst
> @@ -807,6 +807,11 @@ example:
>      This attribute indicates that the inliner should attempt to inline
>      this function into callers whenever possible, ignoring any active
>      inlining size threshold for this caller.
> +``cold``
> +    This attribute indicates that this function is not called
> +    frequently.

how about
   not called frequently -> rarely called
or something else which indicates that 'cold' is only appropriate when the
function is hardly ever called, not when it's just not called *that* much.

   When computing edge weights, basic blocks
> +    post-dominated by a cold function are also considered to be cold;

post-dominated by a cold function -> post-dominated by a cold function call

This covers both calls to a cold function, and calls to a function where it is
the call that is marked cold but not the function.

> +    and, thus, given low weight.
>  ``nonlazybind``
>      This attribute suppresses lazy symbol binding for the function. This
>      may make calls to the function faster, at the cost of extra program
> diff --git a/include/llvm-c/Core.h b/include/llvm-c/Core.h
> index 6b62f33..085e533 100644
> --- a/include/llvm-c/Core.h
> +++ b/include/llvm-c/Core.h
> @@ -166,6 +166,7 @@ typedef enum {
>         and the path forward agreed upon.
>      LLVMAddressSafety = 1ULL << 32,
>      LLVMStackProtectStrongAttribute = 1ULL<<33
> +    LLVMCold = 1ULL << 40

Why the leap from 33 to 40?  These numbers don't need to be the same as those
used in the bitcode (though I guess it doesn't hurt).

>      */
>  } LLVMAttribute;
>
> diff --git a/include/llvm/Analysis/BranchProbabilityInfo.h b/include/llvm/Analysis/BranchProbabilityInfo.h
> index 6c23f7c..8edbcaa 100644
> --- a/include/llvm/Analysis/BranchProbabilityInfo.h
> +++ b/include/llvm/Analysis/BranchProbabilityInfo.h
> @@ -131,11 +131,15 @@ private:
>    /// \brief Track the set of blocks directly succeeded by a returning block.
>    SmallPtrSet<BasicBlock *, 16> PostDominatedByUnreachable;
>
> +  /// \brief Track the set of blocks eventually succeeded by a cold call.
eventually succeeded by a cold call -> that always lead to a cold call

> +  SmallPtrSet<BasicBlock *, 16> PostDominatedByColdCall;
> +
>    /// \brief Get sum of the block successors' weights.
>    uint32_t getSumForBlock(const BasicBlock *BB) const;
>
>    bool calcUnreachableHeuristics(BasicBlock *BB);
>    bool calcMetadataWeights(BasicBlock *BB);
> +  bool calcColdCallHeuristics(BasicBlock *BB);
>    bool calcPointerHeuristics(BasicBlock *BB);
>    bool calcLoopBranchHeuristics(BasicBlock *BB);
>    bool calcZeroHeuristics(BasicBlock *BB);
> diff --git a/lib/Analysis/BranchProbabilityInfo.cpp b/lib/Analysis/BranchProbabilityInfo.cpp
> index 6c58856..769c47e 100644
> --- a/lib/Analysis/BranchProbabilityInfo.cpp
> +++ b/lib/Analysis/BranchProbabilityInfo.cpp
> @@ -69,6 +69,22 @@ static const uint32_t UR_TAKEN_WEIGHT = 1;
>  /// easily subsume it.
>  static const uint32_t UR_NONTAKEN_WEIGHT = 1024*1024 - 1;
>
> +/// \brief Weight for a branch taken going into a cold block.
> +///
> +/// This is the weight for a branch taken toward a block marked
> +/// cold.  A block is marked cold if it's postdominated by a
> +/// block containing a call to a cold function.  Cold functions
> +/// are those marked with attribute 'cold'.
> +static const uint32_t CC_TAKEN_WEIGHT = 4;
> +
> +/// \brief Weight for a branch not-taken into a cold block.
> +///
> +/// This is the weight for a branch not taken toward a block marked
> +/// cold.  A block is marked cold if it's postdominated by a
> +/// block containing a call to a cold function.  Cold functions
> +/// are those marked with attribute 'cold'.

I don't see the point in duplicating the explanation of what a cold block is.
Also, does this weight only apply when one of the successors is a cold block?
Is so, I think you should mention that.

> +static const uint32_t CC_NONTAKEN_WEIGHT = 64;
> +
>  static const uint32_t PH_TAKEN_WEIGHT = 20;
>  static const uint32_t PH_NONTAKEN_WEIGHT = 12;
>
> @@ -193,6 +209,63 @@ bool BranchProbabilityInfo::calcMetadataWeights(BasicBlock *BB) {
>    return true;
>  }
>
> +/// \brief Calculate edge weights for edges leading to cold blocks.
> +///
> +/// A cold block is one post-dominated by  a block with a call to a
> +/// cold function.  Those edges are unlikely to be taken, so we give
> +/// them relatively low weight.
> +///
> +/// Return true if we could compute the weights for cold eges.  Return false,

eges -> edges

> +/// otherwise.
> +bool BranchProbabilityInfo::calcColdCallHeuristics(BasicBlock *BB) {
> +  // If the block itself contains a cold function, add it to the
> +  // set of blocks postdominated by a cold call.
> +  for (BasicBlock::iterator I = BB->begin(), E = BB->end(); I != E; ++I)
> +    if (CallInst *CI = dyn_cast<CallInst>(I))
> +      if (CI->hasFnAttr(Attribute::Cold))
> +        PostDominatedByColdCall.insert(BB);

How about first checking whether the basic block in in the set already?  Since
if it is then you can avoid a potentially expensive scan over the basic block.
If this is impossible, how about adding an assert to check that it never
happens.

> +
> +  TerminatorInst *TI = BB->getTerminator();
> +  if (TI->getNumSuccessors() == 0)
> +    return false;

If BB itself is cold, is there any point spending time computing weights and so
on for the edges going out of it?  How about just bailing out before this point
instead?

> +  SmallVector<unsigned, 4> ColdEdges;
> +  SmallVector<unsigned, 4> NormalEdges;

You could reserve TI->getNumSuccessors() space in these vectors.  Just a
thought.

> +
> +  for (succ_iterator I = succ_begin(BB), E = succ_end(BB); I != E; ++I)
> +    if (PostDominatedByColdCall.count(*I))
> +      ColdEdges.push_back(I.getSuccessorIndex());
> +    else
> +      NormalEdges.push_back(I.getSuccessorIndex());
> +
> +  // If all successors are in the set of blocks post-dominated by cold calls,
> +  // this block is in the set post-dominated by cold calls.
> +  if (ColdEdges.size() == TI->getNumSuccessors())
> +    PostDominatedByColdCall.insert(BB);
> +
> +  // Skip probabilities if this block has a single successor.
> +  if (TI->getNumSuccessors() == 1 || ColdEdges.empty())
> +    return false;
> +
> +  uint32_t ColdWeight =
> +      std::max(CC_TAKEN_WEIGHT / (unsigned) ColdEdges.size(), MIN_WEIGHT);
> +  for (SmallVector<unsigned, 4>::iterator I = ColdEdges.begin(),
> +                                          E = ColdEdges.end();
> +       I != E; ++I)
> +    setEdgeWeight(BB, *I, ColdWeight);
> +
> +  if (NormalEdges.empty())
> +    return true;
> +  uint32_t NormalWeight = std::max(
> +      CC_NONTAKEN_WEIGHT / (unsigned) NormalEdges.size(), NORMAL_WEIGHT);
> +  for (SmallVector<unsigned, 4>::iterator I = NormalEdges.begin(),
> +                                          E = NormalEdges.end();
> +       I != E; ++I)
> +    setEdgeWeight(BB, *I, NormalWeight);
> +
> +  return true;
> +}
> +
>  // Calculate Edge Weights using "Pointer Heuristics". Predict a comparsion
>  // between two pointer or pointer and NULL will fail.
>  bool BranchProbabilityInfo::calcPointerHeuristics(BasicBlock *BB) {

Ciao, Duncan.