[PATCH] D11104: Add setBlockFreq interface to BlockFrequencyInfo, BlockFrequencyInfoImplBase, and BlockFrequencyInfoImpl.

[Duncan P. N. Exon Smith]

> On 2015-Jul-13, at 11:17, Cong Hou <congh at google.com> wrote:
> 
>>> 
>>> +
>>> /// Pop up a ghostview window with the current block frequency propagation
>>> /// rendered using dot.
>>> void BlockFrequencyInfo::view() const {
>>> Index: include/llvm/Analysis/BlockFrequencyInfoImpl.h
>>> ===================================================================
>>> --- include/llvm/Analysis/BlockFrequencyInfoImpl.h
>>> +++ include/llvm/Analysis/BlockFrequencyInfoImpl.h
>>> @@ -938,6 +941,21 @@
>>>   finalizeMetrics();
>>> }
>>> 
>>> +template <class BT>
>>> +bool BlockFrequencyInfoImpl<BT>::setBlockFreq(const BlockT *BB, uint64_t Freq) {
>>> +  if (Nodes.count(BB))
>>> +    return BlockFrequencyInfoImplBase::setBlockFreq(getNode(BB), Freq);
>>> +  else {
>>> +    // If BB is a newly added block after BFI is done, we need to create a new
>>> +    // BlockNode for it assigned with a new index. The index can be determined
>>> +    // by the size of Freqs.
>>> +    BlockNode NewNode(Freqs.size());
>>> +    Nodes[BB] = NewNode;
>>> +    Freqs.emplace_back();
>>> +    return BlockFrequencyInfoImplBase::setBlockFreq(NewNode, Freq);
>> 
>> It seems weird to `emplace_back()` with nothing and then call another
>> function that fills it.
>> 
>> Why not just:
>> 
>>    Nodes[BB] = Freqs.size();
>>    Freqs.push_back(Freq);
>> 
> 
> There is no constructor for the element type of Freqs (FrequencyData).
> The previous way to initialize a FrequencyData object is to create a
> empty one and assign values to it. We also want to call
> BlockFrequencyInfoImplBase::setBlockFreq which has two assertions.
> 
> 

Ah, I was working from memory and thought this was just integers.
SGTM in that case.