MetaRenamer: replaced command line options with pass parameters #133975

egorshamshura · 2025-04-01T19:53:19Z

No description provided.

github-actions · 2025-04-01T19:53:43Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

arsenm · 2025-04-02T01:34:04Z

llvm/include/llvm/Transforms/Utils/MetaRenamer.h

+
+struct MetaRenamerOptions {
+  /// Prefixes for functions that don't need to be renamed, separated by a comma
+  std::string RenameExcludeFunctionPrefixes;


I assume all of these can be StringRef. The options shouldn't really need ownership over the string

arsenm · 2025-04-02T01:35:03Z

llvm/lib/Passes/PassBuilder.cpp

+      Result.RenameExcludeFunctionPrefixes = ParamName;
+    } else {
+      return make_error<StringError>(
+        formatv("invalid MetaRenamer pass parameter '{0}' ", ParamName)


Suggested change

formatv("invalid MetaRenamer pass parameter '{0}' ", ParamName)

formatv("invalid metarenamer pass parameter '{0}' ", ParamName)

These should probably refer to the form used in the passes format

arsenm · 2025-04-02T01:36:05Z

llvm/lib/Transforms/Utils/MetaRenamer.cpp

 void MetaRename(Module &M,
-                function_ref<TargetLibraryInfo &(Function &)> GetTLI) {
+                function_ref<TargetLibraryInfo &(Function &)> GetTLI,
+                MetaRenamerOptions const& Options) {


Suggested change

MetaRenamerOptions const& Options) {

const MetaRenamerOptions &Options) {

arsenm · 2025-04-02T01:40:14Z

llvm/test/Transforms/MetaRenamer/exclude-names.ll

@@ -1,4 +1,4 @@
-; RUN: opt -passes=metarenamer -rename-exclude-function-prefixes=my_func -rename-exclude-global-prefixes=my_global -rename-exclude-struct-prefixes=my_struct -rename-exclude-alias-prefixes=my_alias -S %s | FileCheck %s
+; RUN: opt -passes='metarenamer<rename-exclude-function-prefixes=my_func;rename-exclude-global-prefixes=my_global;rename-exclude-struct-prefixes=my_struct;rename-exclude-alias-prefixes=my_alias>' -S %s | FileCheck %s


The existing test coverage for all the options is lacking. Can you add a new test to make sure all the options parse? In particular, check the failed to parse case. Bonus points for a test which shows they work

Also these are somewhat unique in that the content is also a string, so there may be escape hazards to consider. Not sure there is any precedent for this in the existing options. Can you add some tests where the string value is quoted, and contains a ;?

Added invalid-pass-parameter test and semicolon-substr test

paperchalice · 2025-04-02T01:52:09Z

llvm/lib/Passes/PassRegistry.def

+    return MetaRenamerPass(Options);
+  },
+  parseMetaRenamerPassOptions,
+  "rename-exclude-function-prefixes=S;rename-exclude-alias-prefixes=S;rename-exclude-global-prefixes=S;rename-exclude-struct-prefixes=S;no-rename-only-inst;rename-only-inst")


Keep alphabetical order for each pass registry.

…phabetical order in PassRegistry

… quote test; updated parser

llvmbot · 2025-04-02T06:00:36Z

@llvm/pr-subscribers-llvm-transforms

Author: Shamshura Egor (egorshamshura)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/133975.diff

8 Files Affected:

(modified) llvm/include/llvm/Transforms/Utils/MetaRenamer.h (+26)
(modified) llvm/lib/Passes/PassBuilder.cpp (+60)
(modified) llvm/lib/Passes/PassRegistry.def (+6-2)
(modified) llvm/lib/Transforms/Utils/MetaRenamer.cpp (+8-37)
(modified) llvm/test/Transforms/MetaRenamer/exclude-names.ll (+2-1)
(added) llvm/test/Transforms/MetaRenamer/invalid-pass-parameter.ll (+9)
(modified) llvm/test/Transforms/MetaRenamer/only-inst.ll (+1-1)
(added) llvm/test/Transforms/MetaRenamer/semicolon-substr.ll (+8)

diff --git a/llvm/include/llvm/Transforms/Utils/MetaRenamer.h b/llvm/include/llvm/Transforms/Utils/MetaRenamer.h
index fff3dff75837e..4434fb4ac89a5 100644
--- a/llvm/include/llvm/Transforms/Utils/MetaRenamer.h
+++ b/llvm/include/llvm/Transforms/Utils/MetaRenamer.h
@@ -18,7 +18,33 @@
 #include "llvm/IR/PassManager.h"
 
 namespace llvm {
+
+struct MetaRenamerOptions {
+  /// Prefixes for functions that don't need to be renamed, separated by a comma
+  StringRef RenameExcludeFunctionPrefixes;
+
+  /// Prefixes for aliases that don't need to be renamed, separated by a comma
+  StringRef RenameExcludeAliasPrefixes;
+
+  /// Prefixes for global values that don't need to be renamed, separated by a
+  /// comma
+  StringRef RenameExcludeGlobalPrefixes;
+
+  /// Prefixes for structs that don't need to be renamed, separated by a comma
+  StringRef RenameExcludeStructPrefixes;
+
+  /// Only rename the instructions in the function
+  bool RenameOnlyInst = false;
+};
+
 struct MetaRenamerPass : PassInfoMixin<MetaRenamerPass> {
+private:
+  const MetaRenamerOptions Options;
+
+public:
+  MetaRenamerPass(MetaRenamerOptions Options = MetaRenamerOptions())
+      : Options(Options) {}
+
   PreservedAnalyses run(Module &, ModuleAnalysisManager &);
 };
 } // namespace llvm
diff --git a/llvm/lib/Passes/PassBuilder.cpp b/llvm/lib/Passes/PassBuilder.cpp
index 5cda1517e127d..2951959602afc 100644
--- a/llvm/lib/Passes/PassBuilder.cpp
+++ b/llvm/lib/Passes/PassBuilder.cpp
@@ -176,6 +176,7 @@
 #include "llvm/Passes/OptimizationLevel.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Regex.h"
@@ -947,6 +948,65 @@ parseLowerAllowCheckPassOptions(StringRef Params) {
   return Result;
 }
 
+Expected<MetaRenamerOptions> parseMetaRenamerPassOptions(StringRef Params) {
+  MetaRenamerOptions Result;
+  bool InQuotes = false;
+
+  while (!Params.empty()) {
+    StringRef ParamName;
+    size_t SplitPos = 0;
+
+    for (size_t I = 0; I < Params.size(); ++I) {
+      if (Params[I] == '"') {
+        InQuotes = !InQuotes;
+      } else if (Params[I] == ';' && !InQuotes) {
+        SplitPos = I;
+        break;
+      }
+    }
+
+    if (SplitPos == 0) {
+      ParamName = Params;
+      Params = "";
+    } else {
+      ParamName = Params.substr(0, SplitPos);
+      Params = Params.substr(SplitPos + 1);
+    }
+
+    bool Enable = !ParamName.consume_front("no-");
+
+    auto ExtractValue = [](StringRef Str) -> StringRef {
+      if (Str.starts_with("\"")) {
+        Str = Str.drop_front();
+        if (Str.ends_with("\"")) {
+          Str = Str.drop_back();
+        }
+      }
+      return Str;
+    };
+
+    if (ParamName == "rename-only-inst") {
+      Result.RenameOnlyInst = Enable;
+    } else if (ParamName.consume_front("rename-exclude-struct-prefixes=")) {
+      Result.RenameExcludeStructPrefixes = ExtractValue(ParamName);
+    } else if (ParamName.consume_front("rename-exclude-global-prefixes=")) {
+      Result.RenameExcludeGlobalPrefixes = ExtractValue(ParamName);
+    } else if (ParamName.consume_front("rename-exclude-alias-prefixes=")) {
+      Result.RenameExcludeAliasPrefixes = ExtractValue(ParamName);
+    } else if (ParamName.consume_front("rename-exclude-function-prefixes=")) {
+      Result.RenameExcludeFunctionPrefixes = ExtractValue(ParamName);
+    } else {
+      return make_error<StringError>(
+        formatv("invalid metarenamer pass parameter '{0}' ", ParamName)
+            .str(),
+        inconvertibleErrorCode());
+    }
+  }
+
+  return Result;
+}
+
+
 Expected<MemorySanitizerOptions> parseMSanPassOptions(StringRef Params) {
   MemorySanitizerOptions Result;
   while (!Params.empty()) {
diff --git a/llvm/lib/Passes/PassRegistry.def b/llvm/lib/Passes/PassRegistry.def
index 510a505995304..ffe2f2ab34d53 100644
--- a/llvm/lib/Passes/PassRegistry.def
+++ b/llvm/lib/Passes/PassRegistry.def
@@ -103,7 +103,6 @@ MODULE_PASS("pgo-force-function-attrs", PGOForceFunctionAttrsPass(PGOOpt ? PGOOp
 MODULE_PASS("memprof-context-disambiguation", MemProfContextDisambiguation())
 MODULE_PASS("memprof-module", ModuleMemProfilerPass())
 MODULE_PASS("mergefunc", MergeFunctionsPass())
-MODULE_PASS("metarenamer", MetaRenamerPass())
 MODULE_PASS("module-inline", ModuleInlinerPass())
 MODULE_PASS("name-anon-globals", NameAnonGlobalPass())
 MODULE_PASS("no-op-module", NoOpModulePass())
@@ -233,7 +232,12 @@ MODULE_PASS_WITH_PARAMS(
       return StructuralHashPrinterPass(errs(), Options);
     },
     parseStructuralHashPrinterPassOptions, "detailed;call-target-ignored")
-
+MODULE_PASS_WITH_PARAMS("metarenamer", "MetaRenamerPass",
+  [](MetaRenamerOptions Options) {
+    return MetaRenamerPass(Options);
+  },
+  parseMetaRenamerPassOptions,
+  "rename-exclude-alias-prefixes=S;rename-exclude-function-prefixes=S;rename-exclude-global-prefixes=S;rename-exclude-struct-prefixes=S;no-rename-only-inst;rename-only-inst")
 #undef MODULE_PASS_WITH_PARAMS
 
 #ifndef CGSCC_ANALYSIS
diff --git a/llvm/lib/Transforms/Utils/MetaRenamer.cpp b/llvm/lib/Transforms/Utils/MetaRenamer.cpp
index fd0112ae529cc..205236955c12c 100644
--- a/llvm/lib/Transforms/Utils/MetaRenamer.cpp
+++ b/llvm/lib/Transforms/Utils/MetaRenamer.cpp
@@ -35,36 +35,6 @@
 
 using namespace llvm;
 
-static cl::opt<std::string> RenameExcludeFunctionPrefixes(
-    "rename-exclude-function-prefixes",
-    cl::desc("Prefixes for functions that don't need to be renamed, separated "
-             "by a comma"),
-    cl::Hidden);
-
-static cl::opt<std::string> RenameExcludeAliasPrefixes(
-    "rename-exclude-alias-prefixes",
-    cl::desc("Prefixes for aliases that don't need to be renamed, separated "
-             "by a comma"),
-    cl::Hidden);
-
-static cl::opt<std::string> RenameExcludeGlobalPrefixes(
-    "rename-exclude-global-prefixes",
-    cl::desc(
-        "Prefixes for global values that don't need to be renamed, separated "
-        "by a comma"),
-    cl::Hidden);
-
-static cl::opt<std::string> RenameExcludeStructPrefixes(
-    "rename-exclude-struct-prefixes",
-    cl::desc("Prefixes for structs that don't need to be renamed, separated "
-             "by a comma"),
-    cl::Hidden);
-
-static cl::opt<bool>
-    RenameOnlyInst("rename-only-inst", cl::init(false),
-                   cl::desc("only rename the instructions in the function"),
-                   cl::Hidden);
-
 static const char *const metaNames[] = {
   // See http://en.wikipedia.org/wiki/Metasyntactic_variable
   "foo", "bar", "baz", "quux", "barney", "snork", "zot", "blam", "hoge",
@@ -129,7 +99,8 @@ void MetaRename(Function &F) {
 }
 
 void MetaRename(Module &M,
-                function_ref<TargetLibraryInfo &(Function &)> GetTLI) {
+                function_ref<TargetLibraryInfo &(Function &)> GetTLI,
+                const MetaRenamerOptions &Options) {
   // Seed our PRNG with simple additive sum of ModuleID. We're looking to
   // simply avoid always having the same function names, and we need to
   // remain deterministic.
@@ -143,10 +114,10 @@ void MetaRename(Module &M,
   SmallVector<StringRef, 8> ExcludedGlobalsPrefixes;
   SmallVector<StringRef, 8> ExcludedStructsPrefixes;
   SmallVector<StringRef, 8> ExcludedFuncPrefixes;
-  parseExcludedPrefixes(RenameExcludeAliasPrefixes, ExcludedAliasesPrefixes);
-  parseExcludedPrefixes(RenameExcludeGlobalPrefixes, ExcludedGlobalsPrefixes);
-  parseExcludedPrefixes(RenameExcludeStructPrefixes, ExcludedStructsPrefixes);
-  parseExcludedPrefixes(RenameExcludeFunctionPrefixes, ExcludedFuncPrefixes);
+  parseExcludedPrefixes(Options.RenameExcludeAliasPrefixes, ExcludedAliasesPrefixes);
+  parseExcludedPrefixes(Options.RenameExcludeGlobalPrefixes, ExcludedGlobalsPrefixes);
+  parseExcludedPrefixes(Options.RenameExcludeStructPrefixes, ExcludedStructsPrefixes);
+  parseExcludedPrefixes(Options.RenameExcludeFunctionPrefixes, ExcludedFuncPrefixes);
 
   auto IsNameExcluded = [](StringRef &Name,
                            SmallVectorImpl<StringRef> &ExcludedPrefixes) {
@@ -164,7 +135,7 @@ void MetaRename(Module &M,
            IsNameExcluded(Name, ExcludedFuncPrefixes);
   };
 
-  if (RenameOnlyInst) {
+  if (Options.RenameOnlyInst) {
     // Rename all functions
     for (auto &F : M) {
       if (ExcludeLibFuncs(F))
@@ -230,7 +201,7 @@ PreservedAnalyses MetaRenamerPass::run(Module &M, ModuleAnalysisManager &AM) {
   auto GetTLI = [&FAM](Function &F) -> TargetLibraryInfo & {
     return FAM.getResult<TargetLibraryAnalysis>(F);
   };
-  MetaRename(M, GetTLI);
+  MetaRename(M, GetTLI, Options);
 
   return PreservedAnalyses::all();
 }
diff --git a/llvm/test/Transforms/MetaRenamer/exclude-names.ll b/llvm/test/Transforms/MetaRenamer/exclude-names.ll
index 6e088ef7f4190..1002216f632ac 100644
--- a/llvm/test/Transforms/MetaRenamer/exclude-names.ll
+++ b/llvm/test/Transforms/MetaRenamer/exclude-names.ll
@@ -1,4 +1,5 @@
-; RUN: opt -passes=metarenamer -rename-exclude-function-prefixes=my_func -rename-exclude-global-prefixes=my_global -rename-exclude-struct-prefixes=my_struct -rename-exclude-alias-prefixes=my_alias -S %s | FileCheck %s
+; RUN: opt -passes='metarenamer<rename-exclude-function-prefixes=my_func;rename-exclude-global-prefixes=my_global;rename-exclude-struct-prefixes=my_struct;rename-exclude-alias-prefixes=my_alias>' -S %s | FileCheck %s
+; RUN: opt -passes='metarenamer<rename-exclude-function-prefixes="my_func";rename-exclude-global-prefixes="my_global";rename-exclude-struct-prefixes="my_struct";rename-exclude-alias-prefixes="my_alias">' -S %s | FileCheck %s
 
 ; Check that excluded names don't get renamed while all the other ones do
 
diff --git a/llvm/test/Transforms/MetaRenamer/invalid-pass-parameter.ll b/llvm/test/Transforms/MetaRenamer/invalid-pass-parameter.ll
new file mode 100644
index 0000000000000..2e672f9409b12
--- /dev/null
+++ b/llvm/test/Transforms/MetaRenamer/invalid-pass-parameter.ll
@@ -0,0 +1,9 @@
+; RUN: not opt -S -passes='metarenamer<invalid>' %s 2>&1 | FileCheck -check-prefix=ERR %s
+
+; ERR: invalid metarenamer pass parameter 'invalid'
+
+define i32 @0(i32, i32) {
+  %3 = add i32 %0, %1
+  ret i32 %3
+}
+
diff --git a/llvm/test/Transforms/MetaRenamer/only-inst.ll b/llvm/test/Transforms/MetaRenamer/only-inst.ll
index f13371519682f..c423bab08e301 100644
--- a/llvm/test/Transforms/MetaRenamer/only-inst.ll
+++ b/llvm/test/Transforms/MetaRenamer/only-inst.ll
@@ -1,4 +1,4 @@
-; RUN: opt -passes=metarenamer -rename-only-inst=1 -S < %s | FileCheck %s
+; RUN: opt -passes='metarenamer<rename-only-inst>' -S < %s | FileCheck %s
 target triple = "x86_64-pc-linux-gnu"
 
 ; CHECK: %struct.foo_xxx = type { i32, float, %struct.bar_xxx }
diff --git a/llvm/test/Transforms/MetaRenamer/semicolon-substr.ll b/llvm/test/Transforms/MetaRenamer/semicolon-substr.ll
new file mode 100644
index 0000000000000..ca76316189e01
--- /dev/null
+++ b/llvm/test/Transforms/MetaRenamer/semicolon-substr.ll
@@ -0,0 +1,8 @@
+; RUN: opt -passes='metarenamer<rename-exclude-function-prefixes="my_fu;nc">' -S %s | FileCheck %s
+
+; CHECK: define i32 @"my_fu;nc"(
+define i32 @"my_fu;nc"(i32, i32) {
+  %3 = add i32 %0, %1
+  ret i32 %3
+}
+

paperchalice · 2025-04-02T06:56:15Z

llvm/lib/Passes/PassBuilder.cpp

  return Result;
 }

+Expected<MetaRenamerOptions> parseMetaRenamerPassOptions(StringRef Params) {


We should unify the syntax to handle the case pass-name<arg=a list>, pass internalize has the same request but has a different syntax, pending on pass infrastructure maintainers.

paperchalice · 2025-04-02T06:59:00Z

llvm/lib/Passes/PassRegistry.def

+    return MetaRenamerPass(Options);
+  },
+  parseMetaRenamerPassOptions,
+  "rename-exclude-alias-prefixes=S;rename-exclude-function-prefixes=S;rename-exclude-global-prefixes=S;rename-exclude-struct-prefixes=S;no-rename-only-inst;rename-only-inst")


git clang-format --extensions=def might handle this case.

Thanks, fixed

…ternalize pass, changed tests

egorshamshura · 2025-04-07T19:19:31Z

@arsenm @paperchalice

aeubanks · 2025-04-07T19:38:52Z

given that this pass is just a debugging pass and the pass syntax is gnarly, I'm inclined to keep these as cl::opts, although I don't feel super strongly about that

arsenm · 2025-04-13T12:45:56Z

given that this pass is just a debugging pass and the pass syntax is gnarly, I'm inclined to keep these as cl::opts, although I don't feel super strongly about that

I think we should move towards purging all cl::opts

replaced command line options with pass parameters for MetaRenamer

ca8115a

egorshamshura mentioned this pull request Apr 1, 2025

MetaRenamer command line options should be replaced with pass parameters #133875

Open

arsenm reviewed Apr 2, 2025

View reviewed changes

arsenm requested review from aeubanks and paperchalice April 2, 2025 01:40

paperchalice reviewed Apr 2, 2025

View reviewed changes

egorshamshura added 2 commits April 2, 2025 04:53

changed std::string to StringRef, changed error message, sorted by al…

21e2a36

…phabetical order in PassRegistry

added invalid pass parameter test, added semicolon substr test, added…

61327d7

… quote test; updated parser

llvmbot added the llvm:transforms label Apr 2, 2025

paperchalice reviewed Apr 2, 2025

View reviewed changes

clang-formatted PassRegistry.def, changed syntax to be the same as in…

bbfaafc

…ternalize pass, changed tests

egorshamshura requested review from arsenm and paperchalice April 3, 2025 06:29

removed extra newline

e1c3ba7

	formatv("invalid MetaRenamer pass parameter '{0}' ", ParamName)
	formatv("invalid metarenamer pass parameter '{0}' ", ParamName)

	MetaRenamerOptions const& Options) {
	const MetaRenamerOptions &Options) {

		@@ -1,4 +1,4 @@
		; RUN: opt -passes=metarenamer -rename-exclude-function-prefixes=my_func -rename-exclude-global-prefixes=my_global -rename-exclude-struct-prefixes=my_struct -rename-exclude-alias-prefixes=my_alias -S %s \| FileCheck %s
		; RUN: opt -passes='metarenamer<rename-exclude-function-prefixes=my_func;rename-exclude-global-prefixes=my_global;rename-exclude-struct-prefixes=my_struct;rename-exclude-alias-prefixes=my_alias>' -S %s \| FileCheck %s

MetaRenamer: replaced command line options with pass parameters #133975

Are you sure you want to change the base?

MetaRenamer: replaced command line options with pass parameters #133975

Uh oh!

Conversation

egorshamshura commented Apr 1, 2025

Uh oh!

github-actions bot commented Apr 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

egorshamshura Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paperchalice Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

llvmbot commented Apr 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

egorshamshura commented Apr 7, 2025

Uh oh!

aeubanks commented Apr 7, 2025

Uh oh!

arsenm commented Apr 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

egorshamshura Apr 2, 2025 •

edited

Loading

paperchalice Apr 2, 2025 •

edited

Loading