[PATCH] D11104: Add setBlockFreq interface to BlockFrequencyInfo, BlockFrequencyInfoImplBase, and BlockFrequencyInfoImpl.
Duncan P. N. Exon Smith
dexonsmith at apple.com
Mon Jul 13 14:14:01 PDT 2015
> 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.
More information about the llvm-commits
mailing list