Add 'cold' attribute to functions

[Duncan Sands]

Hi Diego,

>>> +/// \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.
>
> It applies to all the edges going out of a cold block.  If a successor is also a
> cold block, then edges coming out of the successor will also be marked cold.  In
> fact, the successor will be marked cold before the current block because the
> caller to this function works bottom up in the CFG.

this doesn't sound right: a successor of a cold block can be hot.

>
> I removed the cold block explanation.
>
>>
>>> +static const uint32_t CC_NONTAKEN_WEIGHT = 64;
>>> +
>>>  static const uint32_t PH_TAKEN_WEIGHT = 20;
>>>  static const uint32_t PH_NONTAKEN_WEIGHT = 12;
>>>

>>> +/// 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.
>
> I added an assert.  Since we are doing a bottom-up traversal, the block has
> never been seen when we start.

I actually think you should do this computation later, see below.

>>
>>> +
>>> +  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?
>
> Well, that's what we are doing in this function.  If BB is cold, then all the
> edges coming out of it must be marked cold.  I'm not sure I'm following you here.

There are two things done here:
   (1) if all successors are cold, then BB is also cold
   (2) placing weights on the edges coming out of BB
Suppose that you already determined that BB was cold (by the expensive scan
above).  Then you don't have to do (1) since you already know that BB is cold.
You could still do (2), giving some blocks coming out of BB the "hot" weight,
and others the "cold" weight, according to whether the successor is hot or cold.
But why bother giving them different weights?  Since BB is cold, it never gets
executed much anyway; carefully worrying about which way execution is likely to
go when it comes out of the cold BB block is kind of pointless since execution
hardly gets there anyway.  That said, it's not a big deal, it was just a remark,
feel free to ignore it!

I did have another thought: since scanning all instructions in a basic block can
be expensive, how about reversing the order in which you check the two ways that
a block can be determined to be cold.  I.e. instead of doing it in this order
   - scan the basic block for a cold call; if one is seen then BB is cold
   - scan all successors; if all are cold then BB is cold
do it in this order:
   - scan all successors; if all are cold then BB is cold
   - otherwise, scan the basic block for a cold call; if one is seen then BB is cold

So if all the successors are cold (which is a quick check) then you can just
skip the scanning of all the instructions.

>
>
>>
>>> +  SmallVector<unsigned, 4> ColdEdges;
>>> +  SmallVector<unsigned, 4> NormalEdges;
>>
>> You could reserve TI->getNumSuccessors() space in these vectors.  Just a
>> thought.
>
>
> I can't, really.  The number of successors is not a constexpr.

ColdEdges.reserve(TI->getNumSuccessors());

Ciao, Duncan.

>
>
> I've attached the revised patch.  Thanks for the review.
>
>
> Diego.