[PATCH] D148718: [MemProf] Optionally pass hot/cold hints to operator new

[Snehasish Kumar via Phabricator via llvm-commits]

snehasish added inline comments.


================
Comment at: llvm/include/llvm/Analysis/TargetLibraryInfo.def:259
 
+/// void *operator new[](unsigned long, hot_cold_t)
+TLI_DEFINE_ENUM_INTERNAL(Znam10hot_cold_t)
----------------
Should we add a pointer to tcmalloc new_extension here for the definition of hot_cold_t?


================
Comment at: llvm/include/llvm/Analysis/TargetLibraryInfo.def:262
+TLI_DEFINE_STRING_INTERNAL("_Znam10hot_cold_t")
+TLI_DEFINE_SIG_INTERNAL(Ptr, Long, Bool)
+
----------------
For hot_cold_t we need 1b, but sizeof Bool is implementation defined [1]. Is this a concern for the use here? Do we have a Char type here?

[1] https://stackoverflow.com/a/4897859 


================
Comment at: llvm/include/llvm/Transforms/Utils/BuildLibCalls.h:253
+  /// Emit a call to the hot/cold operator new function.
+  Value *emitHotColdNew(Value *Num, IRBuilderBase &B,
+                        const TargetLibraryInfo *TLI, LibFunc NewFunc,
----------------
 I think in the future we will have more hints to pass. Would that modify this method or add additional methods here? If we modify these should we name these to be more generic like "emitExtendedNew"?




================
Comment at: llvm/lib/Transforms/Utils/BuildLibCalls.cpp:1943
+                            const TargetLibraryInfo *TLI, LibFunc NewFunc,
+                            uint8_t hot_cold) {
+  Module *M = B.GetInsertBlock()->getModule();
----------------
Should this be CamelCase?


================
Comment at: llvm/lib/Transforms/Utils/BuildLibCalls.cpp:1951
+                                               Num->getType(), B.getInt8Ty());
+  inferNonMandatoryLibFuncAttrs(M, Name, *TLI);
+  CallInst *CI = B.CreateCall(Func, {Num, B.getInt8(hot_cold)}, Name);
----------------
I don't see any handling of the new functions here [1], do we need to call this? I guess the right thing to do here is extend the switch case to cover all the new libcalls but that's not within scope for this patch.


[1] https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Utils/BuildLibCalls.cpp#L272


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:48
+static cl::opt<bool>
+    OptimizeHotColdNew("optimize-hot-cold-new", cl::Hidden, cl::init(false),
+                       cl::desc("Enable hot/cold operator new library calls"));
----------------
Similar to the comment above, should we keep this generic? E.g. EnableExtendedNew and flag "enable-extended-new". 

Also I'm concerned that someone will enable this by looking at the flag description without realizing the need for memprof metadata and an allocator which implements the extension. Maybe we can leave more hints in the flag description? 


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:1669
+
+  uint8_t hot_cold;
+  if (CI->getAttributes().getFnAttr("memprof").getValueAsString() == "cold")
----------------
Should this be CamelCase?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148718