This is an archive of the discontinued LLVM Phabricator instance.

[MemProf] Optionally pass hot/cold hints to operator new
ClosedPublic

Authored by tejohnson on Apr 19 2023, 8:23 AM.

Details

Summary

Optionally (off by default) replace operator new() calls marked with a
hot or cold memprof attribute with an operator new() call that takes a
hot_cold_t parameter.

Currently this is supported by the open source version of tcmalloc, see:
https://github.com/google/tcmalloc/blob/master/tcmalloc/new_extension.h

Diff Detail

Event Timeline

tejohnson created this revision.Apr 19 2023, 8:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 19 2023, 8:23 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
tejohnson requested review of this revision.Apr 19 2023, 8:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 19 2023, 8:23 AM
snehasish added inline comments.Apr 19 2023, 10:38 AM
llvm/include/llvm/Analysis/TargetLibraryInfo.def
259

Should we add a pointer to tcmalloc new_extension here for the definition of hot_cold_t?

262

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

llvm/include/llvm/Transforms/Utils/BuildLibCalls.h
253

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"?

llvm/lib/Transforms/Utils/BuildLibCalls.cpp
1947

Should this be CamelCase?

1955

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

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
48

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?

1672

Should this be CamelCase?

tejohnson marked 3 inline comments as done.Apr 19 2023, 12:15 PM
tejohnson added inline comments.
llvm/include/llvm/Analysis/TargetLibraryInfo.def
262

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).

llvm/include/llvm/Transforms/Utils/BuildLibCalls.h
253

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?

llvm/lib/Transforms/Utils/BuildLibCalls.cpp
1955

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

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
48

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.

Address comments

snehasish accepted this revision.Apr 19 2023, 12:43 PM

lgtm

llvm/include/llvm/Transforms/Utils/BuildLibCalls.h
253

I don't have a strong opinion so we can leave it as is for now.

This revision is now accepted and ready to land.Apr 19 2023, 12:43 PM
This revision was landed with ongoing or failed builds.Apr 19 2023, 1:34 PM
This revision was automatically updated to reflect the committed changes.