This is an archive of the discontinued LLVM Phabricator instance.

[MBFIWrapper] Add a new function getBlockProfileCount
ClosedPublic

Authored by Carrot on Sep 16 2020, 3:58 PM.

Details

Summary

MBFIWrapper keeps track of block frequencies of newly created blocks and modified blocks, modified block frequencies should also impact block profile count. This class doesn't provide interface getBlockProfileCount, users can only use the underlying MBFI to query profile count, the underlying MBFI doesn't know the modifications made in MBFIWrapper, so it either provides stale profile count for modified block or simply crashes on new blocks.

So this patch add function getBlockProfileCount to class MBFIWrapper to handle new blocks or modified blocks.

Diff Detail

Unit TestsFailed

Event Timeline

Carrot created this revision.Sep 16 2020, 3:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 16 2020, 3:58 PM
Carrot requested review of this revision.Sep 16 2020, 3:58 PM

Is it possible to add a test case?

It can be triggered by a test case in my next basic block duplication patch, because newly created basic block can cause MBFI.getBlockProfileCount crash.

It is very difficult to write a test case in current code base.

davidxl added inline comments.Sep 21 2020, 2:56 PM
llvm/lib/CodeGen/MBFIWrapper.cpp
37

Can you add some comments in the file here ?

Carrot updated this revision to Diff 293297.Sep 21 2020, 5:09 PM
Carrot marked an inline comment as done.
davidxl added inline comments.Sep 21 2020, 7:12 PM
llvm/lib/CodeGen/MBFIWrapper.cpp
42

If the code reaches here, will the call simply return 0 (according to its implementation)?

Carrot added inline comments.Sep 22 2020, 12:00 PM
llvm/lib/CodeGen/MBFIWrapper.cpp
42

See the function getBlockFreq and setBlockFreq, when code reaches here, it means MBB frequency is not changed, so its profile count is not impacted, so we can simply query the underlying MBFI for profile count.

This revision is now accepted and ready to land.Sep 22 2020, 12:36 PM
This revision was automatically updated to reflect the committed changes.