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

[Cong Hou]

On Mon, Jul 13, 2015 at 10:43 AM, Duncan P. N. Exon Smith
<dexonsmith at apple.com> wrote:
>
>> On 2015-Jul-10, at 14:02, Cong Hou <congh at google.com> wrote:
>>
>> On Fri, Jul 10, 2015 at 12:06 PM, Duncan P. N. Exon Smith
>> <dexonsmith at apple.com> wrote:
>>>
>>>> On 2015-Jul-10, at 11:26, Cong Hou <congh at google.com> wrote:
>>>>
>>>> congh created this revision.
>>>> congh added reviewers: dexonsmith, davidxl.
>>>> congh added subscribers: llvm-commits, davidxl.
>>>>
>>>> This patch make it possible to directly change the block frequency through BFI instead of updating block frequencies for the whole function (which is relatively expensive). This is useful when there are small local frequency changes and the up-to-date BFI is needed soon.
>>>>
>>>> http://reviews.llvm.org/D11104
>>>>
>>>> Files:
>>>> include/llvm/Analysis/BlockFrequencyInfo.h
>>>> include/llvm/Analysis/BlockFrequencyInfoImpl.h
>>>> lib/Analysis/BlockFrequencyInfo.cpp
>>>> lib/Analysis/BlockFrequencyInfoImpl.cpp
>>>>
>>>>
>>>
>>>
>>>
>>>> Index: include/llvm/Analysis/BlockFrequencyInfoImpl.h
>>>> ===================================================================
>>>> --- include/llvm/Analysis/BlockFrequencyInfoImpl.h
>>>> +++ include/llvm/Analysis/BlockFrequencyInfoImpl.h
>>>> @@ -456,6 +456,8 @@
>>>>
>>>>  BlockFrequency getBlockFreq(const BlockNode &Node) const;
>>>>
>>>> +  bool setBlockFreq(const BlockNode &Node, uint64_t Freq);
>>>> +
>>>>  raw_ostream &printBlockFreq(raw_ostream &OS, const BlockNode &Node) const;
>>>>  raw_ostream &printBlockFreq(raw_ostream &OS,
>>>>                              const BlockFrequency &Freq) const;
>>>> @@ -886,6 +888,9 @@
>>>>  BlockFrequency getBlockFreq(const BlockT *BB) const {
>>>>    return BlockFrequencyInfoImplBase::getBlockFreq(getNode(BB));
>>>>  }
>>>> +  bool setBlockFreq(const BlockT *BB, uint64_t Freq) {
>>>> +    return BlockFrequencyInfoImplBase::setBlockFreq(getNode(BB), Freq);
>>>
>>> You'll be setting block frequencies for new basic blocks as well, right?
>>> What does `getNode()` return for unknown basic blocks?
>>
>> Good point! I have updated the patch to add new entries for newly
>> added basic blocks. PTAL.
>>
>>
>
> Looks better, thanks.  A few more comments below.
>
> Can you find a way to test this?  If not, it should really be
> committed along with its first use.  Otherwise it's likely to
> bitrot.

OK. I will merge this patch with the jump threading patch. Thank you!

>
>> Index: lib/Analysis/BlockFrequencyInfoImpl.cpp
>> ===================================================================
>> --- lib/Analysis/BlockFrequencyInfoImpl.cpp
>> +++ lib/Analysis/BlockFrequencyInfoImpl.cpp
>> @@ -527,6 +527,14 @@
>>    return Freqs[Node.Index].Scaled;
>>  }
>>
>> +bool BlockFrequencyInfoImplBase::setBlockFreq(const BlockNode &Node,
>> +                                              uint64_t Freq) {
>> +  if (!Node.isValid())
>> +    return false;
>
> I think this should just assert:
>
>     assert(Node.isValid() && "Expected valid node");
>     assert(Node.Index < Freqs.size() && "Expected legal index");

OK. Thanks!

>
>> +  Freqs[Node.Index].Integer = Freq;
>> +  return true;
>
> (No need for a `bool` return anymore.)
>
>> +}
>> +
>>  std::string
>>  BlockFrequencyInfoImplBase::getBlockName(const BlockNode &Node) const {
>>    return std::string();
>> Index: lib/Analysis/BlockFrequencyInfo.cpp
>> ===================================================================
>> --- lib/Analysis/BlockFrequencyInfo.cpp
>> +++ lib/Analysis/BlockFrequencyInfo.cpp
>> @@ -150,6 +150,11 @@
>>    return BFI ? BFI->getBlockFreq(BB) : 0;
>>  }
>>
>> +bool BlockFrequencyInfo::setBlockFreq(const BasicBlock *BB,
>> +                                      uint64_t Freq) {
>> +  return BFI ? BFI->setBlockFreq(BB, Freq) : false;
>
> This looks like it should just be an assertion:
>
>     assert(BFI && "Expected analysis to be available");
>     BFI->setBlockFreq(BB, Freq);

Done.

>
>> +}
>> +
>>  /// 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.


Cong

> ?
>
>> +  }
>> +}
>> +
>>  template <class BT> void BlockFrequencyInfoImpl<BT>::initializeRPOT() {
>>    const BlockT *Entry = F->begin();
>>    RPOT.reserve(F->size());
>