This is an archive of the discontinued LLVM Phabricator instance.

[MemProf] Add memprof metadata related analysis utilities
ClosedPublic

Authored by tejohnson on Jun 29 2022, 1:43 PM.

Details

Summary

Adds a number of utilities that are used to help create and update
memprof related metadata. These will be used during profile matching
and annotation, as well as by the inliner when updating the metadata.
Also adds unit tests for the utilities.

See also related RFCs:
RFC: Sanitizer-based Heap Profiler [1]
RFC: A binary serialization format for MemProf [2]
RFC: IR metadata format for MemProf [3]
(Note that the IR metadata format has changed from the RFC during
implementation, as described in the preceeding patch adding the basic
metadata and verification support.)

Depends on D128141.

Diff Detail

Event Timeline

tejohnson created this revision.Jun 29 2022, 1:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2022, 1:43 PM
tejohnson requested review of this revision.Jun 29 2022, 1:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2022, 1:43 PM

Thanks for splitting out the util change and adding unit tests. It made it much easier to review!

llvm/include/llvm/Analysis/MemoryProfileInfo.h
42

I think defining static inline functions in a header will create an instance for each module this file is included in. I think this is a discouraged pattern. Should we move this and the other helper below to the cpp instead?

99

This function is unused in this patch. Perhaps you meant to use it at L100? It would flip the if condition though.

106

I think this function is public for testing only and callers should prefer using the interface addCallStack(MDNode *MIB);? If so can you document this in the comment.

113

Can you elaborate on the rationale for function attributes for single allocation types? It does look like its simpler to access in the code. Is it also because it reduces overhead? If so can you document the rationale here.

llvm/lib/Analysis/MemoryProfileInfo.cpp
78

I think this switch can be replaced with popcount which is a little easier to follow.

const unsigned PopCount = countPopulation(AllocTypes);
assert(PopCount != 0);
return PopCount > 1;

https://github.com/llvm/llvm-project/blob/655bea4226b401a11164f99c6344e38d8742b8e4/llvm/include/llvm/Support/MathExtras.h#L567

129

Can we just inline the getMIBAllocType call into L135 and avoid the single use AllocType var?

130

Prefer a smallvector or maybe add CallStack.reserve since we know the number of operands?

132

nit: A descriptive name like StackHash would make it easier to follow.

166

!Node->Callers.empty()

212

Add a comment that addCallstack has not been called yet?
Also !Alloc->Callers.empty() since we just want to assert that Callers has some entries.

216

Consider inlining the comment into the assert, e.g.
assert(MIBCallStack.size() == 1 && "Comment");

llvm/unittests/Analysis/MemoryProfileInfoTest.cpp
175

Calling this StackId would make the test easier to follow. Also applies for the other tests.

176

This should probably be ASSERT since if the first stackid isn't 1 then it doesn't make much sense to continue.

263

I guess having a repeated stack id is feasible with recursive calls during profiling. It doesn't matter much since its adding a function attribute in this case.

300

I think this test is redundant since at this point we have already tested

  • the ability to trim contexts in TrimmedMIBContext
  • the ability to extract the metadata node and call the underlying addCallStack() correctly in SimplifyMIBToAttribute.

Up to you if you want to keep it but I'm slightly in favour of removing this test if it doesn't cover any new logic.

tejohnson marked 11 inline comments as done.

Address comments

llvm/include/llvm/Analysis/MemoryProfileInfo.h
99

It's used in a follow on. I think it is more natural to use the Alloc variable directly there on L100. So instead I added a couple calls, before and after calling addCallStack on a new Trie, to invoke and test it.

106

No, it is used during matching in D128142.

113

Yes it is lighter weight and simpler to communicate to the lib call handling. It is the way the cloning code I've been testing tags allocation calls once we create allocation call clones with single allocation types. Added comment.

llvm/lib/Analysis/MemoryProfileInfo.cpp
130

Decided to use reserve since we know the number of entries we'll need.

llvm/unittests/Analysis/MemoryProfileInfoTest.cpp
263

I think this is a typo in the test. In the follow on matching patch we collapse recursion. I've removed it here for simplicity.

300

I wanted to have a test that put these together since is functionality that will be used to update after inlining. I agree the components are tested separately already, but I kind of like having this for completeness. If you prefer though I can remove it.

lgtm, will go through D128142 soon. Thanks!

llvm/unittests/Analysis/MemoryProfileInfoTest.cpp
300

Keeping as is for completeness sounds reasonable to me.

snehasish accepted this revision.Jul 21 2022, 12:27 PM
This revision is now accepted and ready to land.Jul 21 2022, 12:27 PM
tejohnson updated this revision to Diff 446626.Jul 21 2022, 1:45 PM

Fix bug with use of popcount results that was breaking a few tests.

This revision was landed with ongoing or failed builds.Jul 21 2022, 1:46 PM
This revision was automatically updated to reflect the committed changes.