This is an archive of the discontinued LLVM Phabricator instance.

[llvm] Provide utility function for MD_prof
ClosedPublic

Authored by paulkirth on Jun 29 2022, 2:53 PM.

Details

Summary

Currently, there is significant code duplication for dealing with
MD_prof metadata throughout the compiler. These utility functions can
improve code reuse and simplify boilerplate code when dealing with
profiling metadata, such as branch weights. The inent is to provide a
uniform set of APIs that allow common tasks, such as identifying
specific types of MD_prof metadata and extracting branch weights.

Future patches can build on this initial implementation and clean up the
different implementations across the compiler.

Diff Detail

Event Timeline

paulkirth created this revision.Jun 29 2022, 2:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2022, 2:53 PM
paulkirth updated this revision to Diff 446985.Jul 22 2022, 2:41 PM

Simplify helper functions and remove helpers for MD_prof types other than branch_weights

paulkirth updated this revision to Diff 447025.Jul 22 2022, 7:30 PM

Refactor interfaces

paulkirth updated this revision to Diff 447030.Jul 22 2022, 7:41 PM

Add assert back

paulkirth updated this revision to Diff 447033.Jul 22 2022, 7:51 PM

Small fixup from rebase

There's an argument to be made that most of these utility functions should just be part of Metadata.cpp. Separating them seems like a good idea, but as I've implemented this and begun to integrate their use in D128860, I've found that there seems to be significant overlap w/ some of the utility functions in Metadata.cpp

I'll defer to reviewers on precisely where this should live, since there are a few good alternatives in my mind.

paulkirth published this revision for review.Jul 22 2022, 8:38 PM
paulkirth added reviewers: tejohnson, phosek, bogner.
Herald added a project: Restricted Project. · View Herald TranscriptJul 22 2022, 8:39 PM

There's an argument to be made that most of these utility functions should just be part of Metadata.cpp.

It's easy enough to merge these into Metadata.cpp later if we want to. I think a separate utils library is fine for now.

llvm/include/llvm/IR/ProfDataUtils.h
43

typo totla/total

47

Would "extractRawBranchWeights" be a better name here? I guess this matches the naming of the user, but I think it's a bit hard to tell the difference between this and the extractBranchWeights functions from the name/doc comments as is.

llvm/lib/IR/ProfDataUtils.cpp
51

Can we actually get here if we have valid metadata? This looks like something is malformed

paulkirth added inline comments.Jul 26 2022, 11:29 AM
llvm/include/llvm/IR/ProfDataUtils.h
47

Maybe we should leave this one as is, and rename the others to extractRawBranchWeights? This one seems really clear to me that its extracting the total of all weights.

One wrinkle here is that to me extractRawBranchWeights communicates more that we're extracting a series of weights in their raw form. Under that reading I'd expect it to be a SmallVector<ConstantInt*>, i.e., the raw format in the metadata.

In the meantime I'm going to look around the codebase a bit and see if I can either find a pattern we can use or think of another more communicative name.

llvm/lib/IR/ProfDataUtils.cpp
51

That may be true but most of the places where we're extracting branch weights have similar checks.

Two examples are:
https://github.com/llvm/llvm-project/blob/3a2d7d8ad5291102ad98b8269b9c2171d3d58d76/llvm/lib/Analysis/BranchProbabilityInfo.cpp#L404
https://github.com/llvm/llvm-project/blob/3a2d7d8ad5291102ad98b8269b9c2171d3d58d76/llvm/lib/IR/Metadata.cpp#L1510

It looks like we didn't follow that pattern in Transform/Utils/MisExpect.cpp, but that may be a mistake on my part.

I can seen an argument in both cases, and I'm not sure one is better than the other. Since I don't have a strong opinion, I'm happy to replace the check with an assert though if you think that's the better choice.

paulkirth updated this revision to Diff 447862.Jul 26 2022, 3:55 PM

respond to comments

  • Modify API name for extracting the total weight to match the existing API.
  • Add specialized version of extractBranchWeights for branches and selects to ease replacing uses in LLVM.
  • Replace checks on values of metadata w/ asserts that the metadata is not malformed
bogner accepted this revision.Jul 27 2022, 11:37 AM
This revision is now accepted and ready to land.Jul 27 2022, 11:37 AM
paulkirth updated this revision to Diff 448149.Jul 27 2022, 1:42 PM

cleanup and formatting

This revision was landed with ongoing or failed builds.Jul 27 2022, 2:14 PM
This revision was automatically updated to reflect the committed changes.