Page MenuHomePhabricator

Heat Coloring (2/3): Adding Heat Functionality to CFGPrinter
ClosedPublic

Authored by knaumov on Mar 31 2020, 12:27 PM.

Details

Summary

This patch is the second in the sequence of three patches related to the following diff (https://reviews.llvm.org/D73142). The first patch from the sequence that has introduced the structure for DOTFunction Information is found here - D76820. This patch introduces the heat coloring of the Control Flow Graph which is based on the relative "hotness" of each BB. All the functionality of this patch was developed and discussed in the preceding patches.

Diff Detail

Event Timeline

knaumov created this revision.Mar 31 2020, 12:27 PM
davidxl added inline comments.Mar 31 2020, 1:05 PM
llvm/include/llvm/Analysis/CFGPrinter.h
273–286

The formatting (indentation) looks wrong. Apply clang-format to fix it.

278

For color blinded folks, using different box size or box boundary thickness may be a better choice. Is it possible to add that?

llvm/include/llvm/Analysis/HeatUtils.h
30

These two methods do not belong to this patch.

knaumov updated this revision to Diff 254281.Apr 1 2020, 1:08 PM

Answering @davidxl 's comments:

  • Fixed the introduction of getNumOfCalls methods.
  • Concerning the color-blind accessibility - from my research, I have found that the combination of blue and red and all of the intermediate colors are well perceived by the majority of types of color-blind people. I have not found a way to make edges of the boxes gradually bolder to reflect the gradation from cold to hot methods. If any such solutions were to be found, it would be great to implement them, but my suggestion is to do it as a separate future patch while the current series of patches introduces only necessary functionality.
  • Clang-Format: I have applied the clang-format on the said file and found out that not only my changes were formatted, but most of the original file as well. I have reverted the clang-format, as it makes the diff big and hard to follow. Of course, once I am going to be pushing the changes, I will apply clang-format over all the files that were altered by my patches.

For clang-format, you can use clang/tools/clang-format/clang-format-diff.py to format diff only: https://clang.llvm.org/docs/ClangFormat.html.

davidxl accepted this revision.Apr 8 2020, 10:11 AM

lgtm

This revision is now accepted and ready to land.Apr 8 2020, 10:11 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 8 2020, 1:03 PM

I already hit one corner case and fixed it with https://github.com/llvm/llvm-project/commit/3847737fa489dc88d1d45bbedc550e87bf88fbe1 but I can totally see more coming with more tests being added. The test coverage is not sufficient and I certainly expect this code to hit more bugs in the future with the current state.

I don't have enough time to look at this whole patch and infrastructure carefully and certainly don't have time to fix it, so please do that since you are the author.

In particular, llvm/lib/Analysis/HeatUtils.cpp looks very scary. There are casts to double (double percent = log2(double(freq)) / log2(maxFreq);) that I don't understand, pieces like this looking very suspicious

if (freq > maxFreq)
  freq = maxFreq;

Corner cases handled in a very strange way

if (percent > 1.0)
  percent = 1.0;
if (percent < 0.0)
  percent = 0.0;

Casts from unsigned to doubles and back again (unsigned colorId = unsigned(round(percent * (heatSize - 1.0)));), and std::strings everywhere.

I would suggest thinking more about floating point arithmetics since the patch I provided fixes the case where percent is NaN and then being casted to unsigned and used as an index in the array...

asserts should be placed in many places to ensure everything is going as planned, if there are any assumptions under which your functions operate they should be documented (// Returns the maximum frequency of a BB in a function. and other comments don't tell anything I couldn't infer from the function signature and don't say anything about the requirements or returned value ranges) and mentioned in the code. I would also not recommend calling a value which is in [0, 1] range percent since percent is from 0 to 100. With all of this, it is really hard to understand the code and later fix it for anyone not familiar with it. And, again, I would expect more bugs to originate from this code if it stays the same.

Addressed to @kbobyrev:
Thank you for the detailed report! I will add new assert/verification code to cover the point you discussed.

Thank you! And I apologize for the bad tone. I was just very frustrated because I spent a long time trying to figure out what the issue is and it wasn't very clear from the first look, but that's no excuse for the style of my previous message. I do think this infrastructure is useful, so I am looking forward to the new changes!