Skip to content

MetaRenamer: replaced command line options with pass parameters #133975

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
egorshamshura wants to merge 5 commits into
base: main
Choose a base branch
from
Open

MetaRenamer: replaced command line options with pass parameters #133975

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ca8115a
replaced command line options with pass parameters for MetaRenamer
egorshamshura Apr 1, 2025
21e2a36
changed std::string to StringRef, changed error message, sorted by al…
egorshamshura Apr 2, 2025
61327d7
added invalid pass parameter test, added semicolon substr test, added…
egorshamshura Apr 2, 2025
bbfaafc
clang-formatted PassRegistry.def, changed syntax to be the same as in…
egorshamshura Apr 2, 2025
e1c3ba7
removed extra newline
egorshamshura Apr 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions llvm/include/llvm/Transforms/Utils/MetaRenamer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
std::string RenameExcludeFunctionPrefixes;
Copy link
Contributor

@arsenm arsenm Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@egorshamshura egorshamshura Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed


/// Prefixes for aliases that don't need to be renamed, separated by a comma
std::string RenameExcludeAliasPrefixes;

/// Prefixes for global values that don't need to be renamed, separated by a
/// comma
std::string RenameExcludeGlobalPrefixes;

/// Prefixes for structs that don't need to be renamed, separated by a comma
std::string 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
Expand Down
27 changes: 27 additions & 0 deletions llvm/lib/Passes/PassBuilder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -947,6 +948,32 @@ parseLowerAllowCheckPassOptions(StringRef Params) {
return Result;
}

Expected<MetaRenamerOptions> parseMetaRenamerPassOptions(StringRef Params) {
Copy link
Contributor

@paperchalice paperchalice Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@egorshamshura egorshamshura Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

MetaRenamerOptions Result;
while (!Params.empty()) {
StringRef ParamName;
std::tie(ParamName, Params) = Params.split(';');
bool Enable = !ParamName.consume_front("no-");
if (ParamName == "rename-only-inst") {
Result.RenameOnlyInst = Enable;
} else if (ParamName.consume_front("rename-exclude-struct-prefixes=")) {
Result.RenameExcludeStructPrefixes = ParamName;
} else if (ParamName.consume_front("rename-exclude-global-prefixes=")) {
Result.RenameExcludeGlobalPrefixes = ParamName;
} else if (ParamName.consume_front("rename-exclude-alias-prefixes=")) {
Result.RenameExcludeAliasPrefixes = ParamName;
} else if (ParamName.consume_front("rename-exclude-function-prefixes=")) {
Result.RenameExcludeFunctionPrefixes = ParamName;
} else {
return make_error<StringError>(
formatv("invalid MetaRenamer pass parameter '{0}' ", ParamName)
Copy link
Contributor

@arsenm arsenm Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@egorshamshura egorshamshura Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

.str(),
inconvertibleErrorCode());
}
}
return Result;
}

Expected<MemorySanitizerOptions> parseMSanPassOptions(StringRef Params) {
MemorySanitizerOptions Result;
while (!Params.empty()) {
Expand Down
8 changes: 6 additions & 2 deletions llvm/lib/Passes/PassRegistry.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down Expand Up @@ -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-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")
Copy link
Contributor

@paperchalice paperchalice Apr 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep alphabetical order for each pass registry.

Copy link
Contributor Author

@egorshamshura egorshamshura Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

#undef MODULE_PASS_WITH_PARAMS

#ifndef CGSCC_ANALYSIS
Expand Down
45 changes: 8 additions & 37 deletions llvm/lib/Transforms/Utils/MetaRenamer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -129,7 +99,8 @@ void MetaRename(Function &F) {
}

void MetaRename(Module &M,
function_ref<TargetLibraryInfo &(Function &)> GetTLI) {
function_ref<TargetLibraryInfo &(Function &)> GetTLI,
MetaRenamerOptions const& Options) {
Copy link
Contributor

@arsenm arsenm Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
MetaRenamerOptions const& Options) {
const MetaRenamerOptions &Options) {

Copy link
Contributor Author

@egorshamshura egorshamshura Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

// 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.
Expand All @@ -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) {
Expand All @@ -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))
Expand Down Expand Up @@ -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();
}
2 changes: 1 addition & 1 deletion llvm/test/Transforms/MetaRenamer/exclude-names.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Copy link
Contributor

@arsenm arsenm Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ;?

Copy link
Contributor Author

@egorshamshura egorshamshura Apr 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


; Check that excluded names don't get renamed while all the other ones do

Expand Down
2 changes: 1 addition & 1 deletion llvm/test/Transforms/MetaRenamer/only-inst.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 }
Expand Down
Loading