Page MenuHomePhabricator

Extending CFGPrinter and CallPrinter with Heat Colors
ClosedPublic

Authored by rcorcs on Nov 24 2017, 4:35 AM.

Details

Summary

Extends the CFGPrinter and CallPrinter with heat colors based on heuristics or profiling information.
The colors can be enabled for CFGPrinter by using the option -cfg-heat-colors for both -dot-cfg[-only] and -view-cfg[-only].
Similarly, the colors can be enabled for CallPrinter by using the option -callgraph-heat-colors for both -dot-callgraph and -view-callgraph.

Diff Detail

Repository
rL LLVM

Event Timeline

rcorcs created this revision.Nov 24 2017, 4:35 AM
rcorcs updated this revision to Diff 124217.Nov 24 2017, 9:59 AM

Some code refactoring was performed, for example, replacing data structures from the C++ STL to equivalent one in LLVM ADT.

davidxl added inline comments.Nov 27 2017, 9:45 AM
include/llvm/Analysis/CFGPrinter.h
80

getF -> getFunction

158

Can you commit these format changes separately as NFC?

255

total may overflow ..

272

to get edge frequency, what you should do is to: get the edge probability information using BPI, and then multiply it with the source BB's frequency.

For edges, the branch probability data is also useful to be displayed.

279

early return if not?

lib/Analysis/CFGPrinter.cpp
26

IMO, it is better to make it 'true' by default.

82

use function level max freq can be misleading. The function itself can be totally cold according to the profile summary. With profile data, it is better to use profile summary data: There are three ranges : hot, medium (between hot and cold), and cold.

rcorcs updated this revision to Diff 124573.Nov 28 2017, 7:36 AM

Changes regarding @davidxl comments.

About the edge labels:
The default is to show the branch probability of the edge.
If -cfg-raw-weights=true, then it either displays the old label if it can be acquired from the metadata, otherwise it displays the product of branch probability and the source BB's frequency.

For the last comment about computing the max. freq. based on the function level.
I completely agree that a function level max. freq. can be misleading.
I had originally written my CFGHeatPrinter plugin as a ModulePass, which I believe is better suited for this use case.
I would then compute the heat-ratio of each basic block, based on the basic block with the max. freq. on the module level.

rcorcs updated this revision to Diff 125125.Dec 1 2017, 6:22 AM

Updates the CFGPrinters to work on a per-module basis.

Because the heat factor of each basic block is computed relative to others within the scope being analysed
if we compute the heat factor relative to the basic block with the maximum frequency within the whole module,
it should provide heat colors more representative to the true block frequencies than if we compute the same values
on a per-function basis.
Ideally, the heat map would be computed using profiling information and considering the whole program (perhaps after linking all modules).

davidxl added inline comments.Dec 1 2017, 10:54 AM
lib/Analysis/CallPrinter.cpp
57

Consider templatizing this so that it works for LazyCallGraph too.

227

How about add color attribute to call edges too -- especially when full graph is displayed?

lib/Analysis/HeatUtils.cpp
65

This is not correct when FullCallGraph is displayed.

In that case, you just need to get the callSite instruction and get its parent BB's frequency.

rcorcs updated this revision to Diff 125273.Dec 2 2017, 11:57 AM

Fixed function getNumOfCalls for both full and simplified visualization of the call-graph.

Added edge width in both CallPrinter and CFGPrinter based on the edge weight.

I've also performed some small code refactoring.

davidxl added inline comments.Dec 4 2017, 10:52 AM
lib/Analysis/CallPrinter.cpp
61

clang-format

219

clang-format

222

clang-format

lib/Analysis/HeatUtils.cpp
70

clang-format

rcorcs updated this revision to Diff 125433.Dec 4 2017, 3:33 PM

Fix small code-style standards.

Change option name to -callgraph-weights, instead of -callgraph-estimate-weight, similar to -cfg-weights.

rcorcs updated this revision to Diff 125434.Dec 4 2017, 3:41 PM

Fix remaining code-style standards and option descriptions.

sfertile added inline comments.Jan 4 2018, 10:30 AM
lib/Analysis/CFGPrinter.cpp
135

Should delete these print functions rather then comment them out .

davidxl accepted this revision.Jan 8 2018, 10:28 AM

Add a minimal option test (new heat-color option) in test/Other/2007-06-05-PassID.ll ?

otherwise LGTM

This revision is now accepted and ready to land.Jan 8 2018, 10:28 AM
sfertile added inline comments.Jan 17 2018, 2:46 PM
lib/Analysis/CallPrinter.cpp
286–310

this should be marked as 'override'

318

Should have 'override' here as well.

rcorcs updated this revision to Diff 130608.Jan 19 2018, 6:44 AM

I kept the print functions as they were already present in the code and this change is not related to the heat colors.

I added the 'override' attribute to the getAnalysisUsage functions in the CallGraph printers.

I also added some small tests to the file test/Other/2007-06-05-PassID.ll.

@rcorcs this has been accepted for quite a while, is there anything preventing integration? I'm looking forward to using colored graphs out of the box.

rcorcs updated this revision to Diff 152904.Jun 26 2018, 9:24 AM

Minor update to avoid conflict with the latest LLVM release.

Hi, I believe it is ready for integration.
But I don't think I have commit access.
Perhaps someone with access can commit this patch or let me know how I could do it myself.

Thanks!

rcorcs updated this revision to Diff 153492.Jun 29 2018, 7:32 AM

I've just noticed that last time I uploaded the wrong diff.

This is the actual diff that fixes all conflicts with the latest LLVM version.

I've just noticed that last time I uploaded the wrong diff.

This is the actual diff that fixes all conflicts with the latest LLVM version.

Thanks, I'll commit this for you.

This revision was automatically updated to reflect the committed changes.

Unfortunately I had to revert this (https://reviews.llvm.org/rL336000) since there were several build-bot breaks.

2 were related to Polly which I don't build so I missed that it has a dependency the CFG printer interface. It should be a simple to fix.
The third was a failure on the X86_64-sanitizer fast bot: FAIL: LLVM :: Other/2007-06-05-PassID.ll (20391 of 26226) which I'll have to look at a bit closer.

fedor.sergeev reopened this revision.Jul 31 2018, 12:42 PM
fedor.sergeev added a subscriber: fedor.sergeev.

Unfortunately I had to revert this (https://reviews.llvm.org/rL336000) since there were several build-bot breaks.

Shouldnt this be reopened?
Any plans to reintegrate this back - it seems to provide a very useful functionality...

This revision is now accepted and ready to land.Jul 31 2018, 12:43 PM

I have fixed the bug that this patch was producing on Polly.
https://reviews.llvm.org/D49154

The bug detected by the sanitizer is being discussed here:
https://reviews.llvm.org/rL336000

Except for the bug detected by the sanitizer, which seems to be related to the old pass manager, this patch is ready to be committed.

sfertile added a subscriber: bollu.Jul 31 2018, 1:17 PM

Except for the bug detected by the sanitizer, which seems to be related to the old pass manager, this patch is ready to be committed.

Sorry, I had some trouble with my email for a few days and missed that the polly problem has been fixed. The patch I posted in the comment on https://reviews.llvm.org/rL336000 fixes the sanitizer failure, can you update this patch with that change? With that and the polly patch this is ready to be commited again.

@bollu If you have commit access to both polly and llvm you can commit them both? Otherwise we can arrange to commit them at the same time.

rcorcs updated this revision to Diff 158745.Aug 2 2018, 6:35 AM

Fix of the sanitizer failure as suggested by @sfertile:
https://reviews.llvm.org/rL336000

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2019, 4:08 AM
Herald added a subscriber: hiraditya. · View Herald Transcript