Page MenuHomePhabricator

[MemProf] Basic metadata support and verification
ClosedPublic

Authored by tejohnson on Jun 19 2022, 10:14 AM.

Details

Summary

Add basic support for the MemProf metadata (!memprof and !callsite)
which was initially described in "RFC: IR metadata format for MemProf"
(https://discourse.llvm.org/t/rfc-ir-metadata-format-for-memprof/59165).

The bulk of the patch is verification support, along with some tests.

There are a couple of changes to the format described in the original
RFC:

Initial measurements suggested that a tree format for the stack ids in
the contexts would be more efficient, but subsequent evaluation with
large applications showed that in fact the cost of the additional
metadata nodes required by this deduplication scheme overwhelmed the
benefit from sharing stack id nodes. Therefore, the implementation here
and in follow on patches utilizes a simpler scheme of lists of stack id
integers in the memprof profile contexts and callsite metadata. The
follow on matching patch employs context trimming optimizations to
reduce the cost.

Secondly, instead of verbosely listing all profiled fields in each
profiled context (memory info block or MIB), and deferring the
interpretation of the profile data, the profile data is evaluated and
converted into string tags specifying the behavior (e.g. "cold") during
profile matching. This reduces the verbosity of the profile metadata,
and allows additional context trimming optimizations. As a result, the
named metadata schema description is also no longer needed.

Diff Detail

Event Timeline

tejohnson created this revision.Jun 19 2022, 10:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 19 2022, 10:14 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
tejohnson requested review of this revision.Jun 19 2022, 10:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 19 2022, 10:14 AM
snehasish accepted this revision.Jun 21 2022, 10:01 AM

lgtm with a couple of minor comments.

llvm/lib/IR/Verifier.cpp
4534

Instruction& I is unused.

Perhaps add a check here that I is a CallInst? If you do then also add a test for it.

4535

Perhaps add a comment here about whats being verified. The transition from callsite verification to callstack may be hard to follow. How about something like --

// Here we verify the callstack profile annotated by memprof. This callsite is a part of a profiled allocation callstack.

This revision is now accepted and ready to land.Jun 21 2022, 10:01 AM
Enna1 added a subscriber: Enna1.Jun 22 2022, 1:36 AM
Enna1 added inline comments.
llvm/test/Verifier/memprof-metadata-good.ll
30

IIUC, does this !4 = !{i64 123, i64 789, i64 678} means we have a call stack: test3() -> test1() -> malloc()
If so, the callee in test3() should be test1() instead of test2() ? :

define i32* @test3() {
entry:
  %call = call noundef i32* @test1(), !callsite !7
  ret i32* %call
}
tejohnson marked 3 inline comments as done.

Address comments

llvm/lib/IR/Verifier.cpp
4534

Good idea, added this check both here and in visitMemProfMetadata, and added checks for both in the bad metadata test.

llvm/test/Verifier/memprof-metadata-good.ll
30

Indeed! This is a good catch, mistake in my hand crafted test case IR. Fixed. (Although it doesn't matter for verification and in general we can't detect this kind of issue since the call chains can cross module boundaries.)

This revision was landed with ongoing or failed builds.Wed, Jul 20, 3:31 PM
This revision was automatically updated to reflect the committed changes.