This is an archive of the discontinued LLVM Phabricator instance.

[MemProf] Update metadata during inlining
ClosedPublic

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

Details

Summary

Update both memprof and callsite metadata to reflect inlined functions.

For callsite metadata this is simply a concatenation of each cloned
call's call stack with that of the inlined callsite's.

For memprof metadata, each profiled memory info block (MIB) is either
moved to the cloned allocation call or left on the original allocation
call depending on whether its context matches the newly refined call
stack context on the cloned call. We also reapply context trimming
optimizations based on the refined set of contexts on each of the calls
(cloned and original).

Depends on D128142.

Diff Detail

Event Timeline

tejohnson created this revision.Jun 19 2022, 10:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 19 2022, 10:19 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
tejohnson requested review of this revision.Jun 19 2022, 10:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 19 2022, 10:19 AM
tejohnson updated this revision to Diff 441543.Jun 30 2022, 3:29 PM

Rebase on top of analysis utilities extracted into D128854.

snehasish accepted this revision.Sep 29 2022, 1:52 PM

lgtm

llvm/lib/Analysis/MemoryBuiltins.cpp
320 ↗(On Diff #463021)

I guess this was removed from the diff due to the rebase?

llvm/lib/Transforms/Utils/InlineFunction.cpp
805

This will return true if one or both are empty. Do you want that to count as a common prefix?

817

const std::vector since it doesn't look like we modify the contents?

823

Spelling out the type here would be good. From the name I would have guessed this is a MemInfoBlock but its actually Metadata type.

836

Should we call this method propagateMemProfHelper or similar? As an overload it was a bit confusing to distinguish.

881

I think if we check the else condition first we can return early and reduce the nesting here

if (NewMIBList.empty()) {
  removeMemProfMetaData
  removeCallsiteMetaData
  return
}
916

auto& to avoid copying the pair?

919

Can you add a comment on why either of these could be null when cast to CallBase?

936

I think the guidance is to use braces in the outer block here since there are multiple levels of nesting.
https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

llvm/test/Transforms/Inline/memprof_inline.ll
2

Can you add the c++ code for memprof_inline.cc in the comments here? I think it might be useful to track which callsites we expect metadata on before reading through the verbose IR below.

This revision is now accepted and ready to land.Sep 29 2022, 1:52 PM
tejohnson updated this revision to Diff 464400.Sep 30 2022, 2:22 PM
tejohnson marked 9 inline comments as done.

Address comments

llvm/lib/Analysis/MemoryBuiltins.cpp
320 ↗(On Diff #463021)

Yeah, hmm, it looks like this was here due to a bad rebase (notice there is another copy of this 2 functions up).

llvm/lib/Transforms/Utils/InlineFunction.cpp
805

The assumption is that neither should be empty. Added an assert.

919

Done and also removed the unnecessary null guard of ClonedCall below.

llvm/test/Transforms/Inline/memprof_inline.ll
2

Done here and for memprof_inline2.ll

This revision was landed with ongoing or failed builds.Sep 30 2022, 4:46 PM
This revision was automatically updated to reflect the committed changes.