[PATCH] D147285: [Support] Extended llvm-profdata's merge functionality to exclude profiles from functions matching configurable patterns

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.


================
Comment at: llvm/test/tools/llvm-profdata/merge-filtering.test:57
+# excluding profile for one pattern matching multiple functions
+RUN: llvm-profdata merge --exclude-function "foo" --text %p/Inputs/basic.proftext -o - | FileCheck %s --check-prefix=MERGE-EXCL-SINGLE-PAT
+MERGE-EXCL-SINGLE-PAT-NOT: foo
----------------
I'd expect the regular expression here to be something like `foo.*`, not `foo`?


================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:338
+        getline(ExcludeFuncSS, ExcludeFuncPat, ';');
+        if (!ExcludeFuncPat.empty() && FuncName.contains(ExcludeFuncPat)) {
+          ToBeExcluded = true;
----------------
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`


================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:345
+
+    if (ToBeExcluded) {
+      continue;
----------------
clang-format should remove these braces


================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:1280
+      cl::desc("Include profiles from only those functions where names don't "
+               "match all the regexes separated by a semi-colon"));
 
----------------
I think you can de Morgan this:

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147285/new/

https://reviews.llvm.org/D147285