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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson marked 3 inline comments as done.
tejohnson added inline comments.


================
Comment at: llvm/include/llvm/Analysis/TargetLibraryInfo.def:262
+TLI_DEFINE_STRING_INTERNAL("_Znam10hot_cold_t")
+TLI_DEFINE_SIG_INTERNAL(Ptr, Long, Bool)
+
----------------
snehasish wrote:
> 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 
Bool is documented at 8 bits on all targets: https://github.com/llvm/llvm-project/blob/main/llvm/lib/Analysis/TargetLibraryInfo.cpp#L47
(there is no Char as a result).


================
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,
----------------
snehasish wrote:
>  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"?
> 
> 
I'm not really sure yet, and I'm concerned that "Extended" isn't very clear (because presumably there could be many different types of operator new extensions in the future). So I think I would prefer to keep these specific to HotCold hints, and we can rename them if needed in the future. WDYT?


================
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);
----------------
snehasish wrote:
> 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
I think it is a good idea to call, in the event that the operator new libcalls are handled in any way there (none are so I didn't modify it to add the new ones). Note there is an existing comment about handling all libcalls, but I agree that is not in scope for this change: https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Utils/BuildLibCalls.cpp#L1225-L1226


================
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"));
----------------
snehasish wrote:
> 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? 
I think I would prefer to keep the option specific to hot/cold. The reason being that different allocator implementations may add support for different hints and other extensions at different times, and it is probably good to be able to enable them individually. I did add a comment here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148718