[PATCH] D149932: [MemProf] Add hot allocation type

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Looks great, thanks! Just a few minor comments.



================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:340
 // Allocation type assigned to an allocation reached by a given context.
 // More can be added but initially this is just noncold and cold.
 // Values should be powers of two so that they can be ORed, in particular to
----------------
Update comment


================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:616
     return const_cast<GlobalValueSummary &>(
-                         static_cast<const AliasSummary *>(this)->getAliasee());
   }
----------------
I'm seeing some whitespace differences on otherwise unchanged lines in this patch. Did you run some kind of automatic formatting on the whole file? We'll want to avoid unrelated formatting changes in the patch, even if this used clang-format with the LLVM style (https://clang.llvm.org/docs/ClangFormatStyleOptions.html)


================
Comment at: llvm/lib/Analysis/MemoryProfileInfo.cpp:36
 
+// Lower bound on average life time accesses density (total life time access
+// density / alloc count) for marking an allocation hot.
----------------
nit: other places here use lifetime as one word, maybe switch to that for consistency.


================
Comment at: llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp:42
 #include "llvm/Transforms/IPO.h"
+#include <cstdint>
 #include <sstream>
----------------
Is this change needed?


================
Comment at: llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp:1951
+// helper function to check an AllocType is cold or notcold or both.
+bool checkColdorNotCold(uint8_t AllocType) {
+  return (AllocType == (uint8_t)AllocationType::Cold) ||
----------------
s/or/Or/ in the func name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149932