This is an archive of the discontinued LLVM Phabricator instance.

[Support] Extended llvm-profdata's merge functionality to exclude profiles from functions matching configurable patterns
Needs ReviewPublic

Authored by nilanjana_basu on Mar 30 2023, 4:28 PM.

Details

Summary

For debugging PGO performance, it is useful to have a facility to filter out profiles at a function-level granularity while merging. This will help in checking the impact of a set of function profiles on performance.

Diff Detail

Event Timeline

nilanjana_basu created this revision.Mar 30 2023, 4:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2023, 4:28 PM

Added error reporting for options that are not yet supported with -exclude-function option

minor cosmetic changes

nilanjana_basu published this revision for review.Apr 10 2023, 4:26 PM
nilanjana_basu edited the summary of this revision. (Show Details)
nilanjana_basu added reviewers: paquette, snehasish, vsk, xur.
Herald added a project: Restricted Project. · View Herald TranscriptApr 10 2023, 4:27 PM
paquette added inline comments.Apr 10 2023, 4:43 PM
llvm/test/tools/llvm-profdata/merge-filtering.test
58

I'd expect the regular expression here to be something like foo.*, not foo?

llvm/tools/llvm-profdata/llvm-profdata.cpp
339

To me, this looks like it's matching a substring, not a regex.

/// Return true if the given string is a substring of *this, and false
/// otherwise.
[[nodiscard]] bool contains(StringRef Other) const {
  return find(Other) != npos;
}

I'd expect this to use the utilities from llvm/include/llvm/Support/Regex.h

346

clang-format should remove these braces

1281

I think you can de Morgan this:

"Exclude functions whose names match any regex in a semicolon-separated list"

nilanjana_basu marked 4 inline comments as done.

Updated function pattern to use Regex, and added corresponding tests.

llvm/test/tools/llvm-profdata/merge-filtering.test
58

Added new cases for regex.

llvm/tools/llvm-profdata/llvm-profdata.cpp
346

It didn't, but I removed it manually.

Do we need to keep the comma-separated list now that we're using a regex?

Removed redundant feature of semicolon-separated exclude patterns, since regex can handle that

Do we need to keep the comma-separated list now that we're using a regex?

It was left over by mistake. Thanks for spotting it. Removed it.

Gerolf added a subscriber: Gerolf.Jun 13 2023, 9:12 PM
Gerolf added inline comments.
llvm/tools/llvm-profdata/llvm-profdata.cpp
1314

The check above is for !SuppInstrWithSample, so this code could issue a wrong error message.

nilanjana_basu marked an inline comment as done.Jun 16 2023, 6:04 PM
nilanjana_basu added inline comments.
llvm/tools/llvm-profdata/llvm-profdata.cpp
1314

The check above is for !SupplInstrWithSample.empty(). I don't see how this is a wrong error message, since exclude-function functionality is not supported with the -supplement-instr-with-sample option.

nilanjana_basu marked an inline comment as done.Jul 10 2023, 12:06 PM
nilanjana_basu added inline comments.
llvm/tools/llvm-profdata/llvm-profdata.cpp
1314

@Gerolf : If I misunderstood the issue, can you elaborate on this?