[PATCH] D77161: Heat Coloring (2/3): Adding Heat Functionality to CFGPrinter

[Kirill Bobyrev via Phabricator via llvm-commits]

kbobyrev added a comment.

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::string`s 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...

`assert`s 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77161/new/

https://reviews.llvm.org/D77161