[OptBisect][IR] Adding a new OptPassGate for disabling passes via name #145059
Conversation
|
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 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. |
|
@llvm/pr-subscribers-llvm-ir Author: Cristian Assaiante (cristianassaiante) ChangesI would like to propose a new pass gate that allows selective disabling of one or more passes via the clang command line using the Example: Pass names are matched using case-insensitive comparisons. However, note that special characters, including spaces, must be included exactly as they appear in the pass names. Additionally, a To validate this add-on, a test file has also been provided. It reuses the same infrastructure as the opt-bisect test, but disables three specific passes and checks the output to ensure the expected behavior. Full diff: https://github.com/llvm/llvm-project/pull/145059.diff 3 Files Affected:
diff --git a/clang/test/CodeGen/opt-disable.c b/clang/test/CodeGen/opt-disable.c
new file mode 100644
index 0000000000000..ee90fc5620d65
--- /dev/null
+++ b/clang/test/CodeGen/opt-disable.c
@@ -0,0 +1,13 @@
+// REQUIRES: x86-registered-target
+
+// Make sure opt-bisect works through both pass managers
+//
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -O1 %s -mllvm -opt-disable="inlinerpass,SROAPass,machine code sinking" -mllvm -opt-disable-verbose -emit-obj -o /dev/null 2>&1 | FileCheck %s
+
+// CHECK-NOT: DISABLE: running pass InlinerPass
+// CHECK-NOT: DISABLE: running pass SROAPass
+// CHECK-NOT: DISABLE: running pass Machine code sinking
+// Make sure that legacy pass manager is running
+// CHECK: Instruction Selection
+
+int func(int a) { return a; }
diff --git a/llvm/include/llvm/IR/OptBisect.h b/llvm/include/llvm/IR/OptBisect.h
index be6aef3298b23..51c3a8040da9b 100644
--- a/llvm/include/llvm/IR/OptBisect.h
+++ b/llvm/include/llvm/IR/OptBisect.h
@@ -17,6 +17,7 @@
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Compiler.h"
#include <limits>
+#include <set>
namespace llvm {
@@ -82,6 +83,44 @@ class LLVM_ABI OptBisect : public OptPassGate {
int LastBisectNum = 0;
};
+/// This class implements a mechanism to disable passes and individual
+/// optimizations at compile time based on a command line option
+/// (-opt-disable) in order to study how single transformations, or
+/// combinations thereof, affect the IR.
+class LLVM_ABI OptDisable : public OptPassGate {
+public:
+ /// Default constructor. Initializes the state to empty set. The disabling
+ /// will be enabled by the cl::opt call-back when the command line option
+ /// is processed.
+ /// Clients should not instantiate this class directly. All access should go
+ /// through LLVMContext.
+ OptDisable() = default;
+
+ virtual ~OptDisable() = default;
+
+ /// Checks the pass name to determine if the specified pass should run.
+ ///
+ /// The method prints the name of the pass, and whether or not the pass
+ /// will be executed. It returns true if the pass should run, i.e. if
+ /// its name is was not provided via command line.
+ ///
+ /// Most passes should not call this routine directly. Instead, it is called
+ /// through helper routines provided by the base classes of the pass. For
+ /// instance, function passes should call FunctionPass::skipFunction().
+ bool shouldRunPass(const StringRef PassName,
+ StringRef IRDescription) override;
+
+ /// Parses the command line argument to extract the names of the passes
+ /// to be disabled. Multiple pass names can be provided with comma separation.
+ void setDisabled(StringRef Passes);
+
+ /// isEnabled() should return true before calling shouldRunPass().
+ bool isEnabled() const override { return !DisabledPasses.empty(); }
+
+private:
+ std::set<std::string> DisabledPasses = {};
+};
+
/// Singleton instance of the OptBisect class, so multiple pass managers don't
/// need to coordinate their uses of OptBisect.
LLVM_ABI OptPassGate &getGlobalPassGate();
diff --git a/llvm/lib/IR/OptBisect.cpp b/llvm/lib/IR/OptBisect.cpp
index 559b199445366..aa1dbdfdbebd4 100644
--- a/llvm/lib/IR/OptBisect.cpp
+++ b/llvm/lib/IR/OptBisect.cpp
@@ -17,6 +17,7 @@
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/raw_ostream.h"
#include <cassert>
+#include <sstream>
using namespace llvm;
@@ -37,8 +38,8 @@ static cl::opt<bool> OptBisectVerbose(
cl::desc("Show verbose output when opt-bisect-limit is set"), cl::Hidden,
cl::init(true), cl::Optional);
-static void printPassMessage(const StringRef &Name, int PassNum,
- StringRef TargetDesc, bool Running) {
+static void printBisectPassMessage(const StringRef &Name, int PassNum,
+ StringRef TargetDesc, bool Running) {
StringRef Status = Running ? "" : "NOT ";
errs() << "BISECT: " << Status << "running pass "
<< "(" << PassNum << ") " << Name << " on " << TargetDesc << "\n";
@@ -51,10 +52,64 @@ bool OptBisect::shouldRunPass(const StringRef PassName,
int CurBisectNum = ++LastBisectNum;
bool ShouldRun = (BisectLimit == -1 || CurBisectNum <= BisectLimit);
if (OptBisectVerbose)
- printPassMessage(PassName, CurBisectNum, IRDescription, ShouldRun);
+ printBisectPassMessage(PassName, CurBisectNum, IRDescription, ShouldRun);
return ShouldRun;
}
const int OptBisect::Disabled;
-OptPassGate &llvm::getGlobalPassGate() { return getOptBisector(); }
+static OptDisable &getOptDisabler() {
+ static OptDisable OptDisabler;
+ return OptDisabler;
+}
+
+static cl::opt<std::string> OptDisablePass(
+ "opt-disable", cl::Hidden, cl::init(""), cl::Optional,
+ cl::cb<void, std::string>([](std::string Passes) {
+ getOptDisabler().setDisabled(Passes);
+ }),
+ cl::desc("Optimization pass(es) to disable (comma separated)"));
+
+static cl::opt<bool>
+ OptDisableVerbose("opt-disable-verbose",
+ cl::desc("Show verbose output when opt-disable is set"),
+ cl::Hidden, cl::init(false), cl::Optional);
+
+static void printDisablePassMessage(const StringRef &Name, StringRef TargetDesc,
+ bool Running) {
+ StringRef Status = Running ? "" : "NOT ";
+ errs() << "DISABLE: " << Status << "running pass " << Name << " on "
+ << TargetDesc << "\n";
+}
+
+void OptDisable::setDisabled(StringRef Passes) {
+ std::stringstream StrStream(Passes.str());
+ std::string Token;
+
+ while (std::getline(StrStream, Token, ',')) {
+ if (!Token.empty()) {
+ std::transform(Token.begin(), Token.end(), Token.begin(), ::tolower);
+ DisabledPasses.insert(Token);
+ }
+ }
+}
+
+bool OptDisable::shouldRunPass(const StringRef PassName,
+ StringRef IRDescription) {
+ assert(isEnabled());
+
+ std::string LowerName = PassName.str();
+ std::transform(LowerName.begin(), LowerName.end(), LowerName.begin(),
+ ::tolower);
+
+ bool ShouldRun = DisabledPasses.find(LowerName) == DisabledPasses.end();
+ if (OptDisableVerbose)
+ printDisablePassMessage(PassName, IRDescription, ShouldRun);
+ return ShouldRun;
+}
+
+OptPassGate &llvm::getGlobalPassGate() {
+ if (getOptDisabler().isEnabled())
+ return getOptDisabler();
+ return getOptBisector();
+}
|
|
@llvm/pr-subscribers-clang Author: Cristian Assaiante (cristianassaiante) ChangesI would like to propose a new pass gate that allows selective disabling of one or more passes via the clang command line using the Example: Pass names are matched using case-insensitive comparisons. However, note that special characters, including spaces, must be included exactly as they appear in the pass names. Additionally, a To validate this add-on, a test file has also been provided. It reuses the same infrastructure as the opt-bisect test, but disables three specific passes and checks the output to ensure the expected behavior. Full diff: https://github.com/llvm/llvm-project/pull/145059.diff 3 Files Affected:
diff --git a/clang/test/CodeGen/opt-disable.c b/clang/test/CodeGen/opt-disable.c
new file mode 100644
index 0000000000000..ee90fc5620d65
--- /dev/null
+++ b/clang/test/CodeGen/opt-disable.c
@@ -0,0 +1,13 @@
+// REQUIRES: x86-registered-target
+
+// Make sure opt-bisect works through both pass managers
+//
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -O1 %s -mllvm -opt-disable="inlinerpass,SROAPass,machine code sinking" -mllvm -opt-disable-verbose -emit-obj -o /dev/null 2>&1 | FileCheck %s
+
+// CHECK-NOT: DISABLE: running pass InlinerPass
+// CHECK-NOT: DISABLE: running pass SROAPass
+// CHECK-NOT: DISABLE: running pass Machine code sinking
+// Make sure that legacy pass manager is running
+// CHECK: Instruction Selection
+
+int func(int a) { return a; }
diff --git a/llvm/include/llvm/IR/OptBisect.h b/llvm/include/llvm/IR/OptBisect.h
index be6aef3298b23..51c3a8040da9b 100644
--- a/llvm/include/llvm/IR/OptBisect.h
+++ b/llvm/include/llvm/IR/OptBisect.h
@@ -17,6 +17,7 @@
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Compiler.h"
#include <limits>
+#include <set>
namespace llvm {
@@ -82,6 +83,44 @@ class LLVM_ABI OptBisect : public OptPassGate {
int LastBisectNum = 0;
};
+/// This class implements a mechanism to disable passes and individual
+/// optimizations at compile time based on a command line option
+/// (-opt-disable) in order to study how single transformations, or
+/// combinations thereof, affect the IR.
+class LLVM_ABI OptDisable : public OptPassGate {
+public:
+ /// Default constructor. Initializes the state to empty set. The disabling
+ /// will be enabled by the cl::opt call-back when the command line option
+ /// is processed.
+ /// Clients should not instantiate this class directly. All access should go
+ /// through LLVMContext.
+ OptDisable() = default;
+
+ virtual ~OptDisable() = default;
+
+ /// Checks the pass name to determine if the specified pass should run.
+ ///
+ /// The method prints the name of the pass, and whether or not the pass
+ /// will be executed. It returns true if the pass should run, i.e. if
+ /// its name is was not provided via command line.
+ ///
+ /// Most passes should not call this routine directly. Instead, it is called
+ /// through helper routines provided by the base classes of the pass. For
+ /// instance, function passes should call FunctionPass::skipFunction().
+ bool shouldRunPass(const StringRef PassName,
+ StringRef IRDescription) override;
+
+ /// Parses the command line argument to extract the names of the passes
+ /// to be disabled. Multiple pass names can be provided with comma separation.
+ void setDisabled(StringRef Passes);
+
+ /// isEnabled() should return true before calling shouldRunPass().
+ bool isEnabled() const override { return !DisabledPasses.empty(); }
+
+private:
+ std::set<std::string> DisabledPasses = {};
+};
+
/// Singleton instance of the OptBisect class, so multiple pass managers don't
/// need to coordinate their uses of OptBisect.
LLVM_ABI OptPassGate &getGlobalPassGate();
diff --git a/llvm/lib/IR/OptBisect.cpp b/llvm/lib/IR/OptBisect.cpp
index 559b199445366..aa1dbdfdbebd4 100644
--- a/llvm/lib/IR/OptBisect.cpp
+++ b/llvm/lib/IR/OptBisect.cpp
@@ -17,6 +17,7 @@
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/raw_ostream.h"
#include <cassert>
+#include <sstream>
using namespace llvm;
@@ -37,8 +38,8 @@ static cl::opt<bool> OptBisectVerbose(
cl::desc("Show verbose output when opt-bisect-limit is set"), cl::Hidden,
cl::init(true), cl::Optional);
-static void printPassMessage(const StringRef &Name, int PassNum,
- StringRef TargetDesc, bool Running) {
+static void printBisectPassMessage(const StringRef &Name, int PassNum,
+ StringRef TargetDesc, bool Running) {
StringRef Status = Running ? "" : "NOT ";
errs() << "BISECT: " << Status << "running pass "
<< "(" << PassNum << ") " << Name << " on " << TargetDesc << "\n";
@@ -51,10 +52,64 @@ bool OptBisect::shouldRunPass(const StringRef PassName,
int CurBisectNum = ++LastBisectNum;
bool ShouldRun = (BisectLimit == -1 || CurBisectNum <= BisectLimit);
if (OptBisectVerbose)
- printPassMessage(PassName, CurBisectNum, IRDescription, ShouldRun);
+ printBisectPassMessage(PassName, CurBisectNum, IRDescription, ShouldRun);
return ShouldRun;
}
const int OptBisect::Disabled;
-OptPassGate &llvm::getGlobalPassGate() { return getOptBisector(); }
+static OptDisable &getOptDisabler() {
+ static OptDisable OptDisabler;
+ return OptDisabler;
+}
+
+static cl::opt<std::string> OptDisablePass(
+ "opt-disable", cl::Hidden, cl::init(""), cl::Optional,
+ cl::cb<void, std::string>([](std::string Passes) {
+ getOptDisabler().setDisabled(Passes);
+ }),
+ cl::desc("Optimization pass(es) to disable (comma separated)"));
+
+static cl::opt<bool>
+ OptDisableVerbose("opt-disable-verbose",
+ cl::desc("Show verbose output when opt-disable is set"),
+ cl::Hidden, cl::init(false), cl::Optional);
+
+static void printDisablePassMessage(const StringRef &Name, StringRef TargetDesc,
+ bool Running) {
+ StringRef Status = Running ? "" : "NOT ";
+ errs() << "DISABLE: " << Status << "running pass " << Name << " on "
+ << TargetDesc << "\n";
+}
+
+void OptDisable::setDisabled(StringRef Passes) {
+ std::stringstream StrStream(Passes.str());
+ std::string Token;
+
+ while (std::getline(StrStream, Token, ',')) {
+ if (!Token.empty()) {
+ std::transform(Token.begin(), Token.end(), Token.begin(), ::tolower);
+ DisabledPasses.insert(Token);
+ }
+ }
+}
+
+bool OptDisable::shouldRunPass(const StringRef PassName,
+ StringRef IRDescription) {
+ assert(isEnabled());
+
+ std::string LowerName = PassName.str();
+ std::transform(LowerName.begin(), LowerName.end(), LowerName.begin(),
+ ::tolower);
+
+ bool ShouldRun = DisabledPasses.find(LowerName) == DisabledPasses.end();
+ if (OptDisableVerbose)
+ printDisablePassMessage(PassName, IRDescription, ShouldRun);
+ return ShouldRun;
+}
+
+OptPassGate &llvm::getGlobalPassGate() {
+ if (getOptDisabler().isEnabled())
+ return getOptDisabler();
+ return getOptBisector();
+}
|
There was a problem hiding this comment.
Test this via opt please, not clang.
|
|
||
| // CHECK-NOT: DISABLE: running pass InlinerPass | ||
| // CHECK-NOT: DISABLE: running pass SROAPass | ||
| // CHECK-NOT: DISABLE: running pass Machine code sinking |
There was a problem hiding this comment.
We should be operating on the pass name as it is spelled in the pass pipeline, not the class name (that is sroa, not SROAPass here).
There was a problem hiding this comment.
MapClassName2PassName is the keyword to look for.
There was a problem hiding this comment.
WIP. Currently the PassName provided to the shouldRunPass is obtained from the PassRegistry. If I understood correctly, you are suggesting to use the names that are used to enable passes in opt, which is the PassArgument within PassInfo.
I'm updating the code so that the PassArgument is provided to the gate. To avoid changing the log format of the bisect tool, I will also add a function that finds the PassName from the PassArgument in the registry.
The MapClassName2PassName is done via PassInstrumentationCallbacks which are not trivial to obtain within the gate. Or at least, that is the case from my current (not that extensive) knowledge of the infrastructure.
There was a problem hiding this comment.
Update. Unfortunately the solution I had in mind only works with legacy pm. I still have not found a solution for this.
There was a problem hiding this comment.
After further investigation, we found out that there are two aspects that may interfere with the feasibility of the suggested update.
- With the new pass manager, the
OptPassGateis implemented using PassInstrumentation and the call toshouldRundone inrunBeforePassalways has the full pass name (e.g.,SROAPass) as argument. So, to support the shorter names, a more extensive refactor of this may be needed, maybe with the addition of a names map. - If we take a look at how
optprocesses the shorter names for enabling passes via cli, it leverages theprintPassNamesmethod ofPassBuilder. Unfortunately, this method cannot be accessed within thellvm/IR/OptBisect.cppfile, since it resides inllvm/Passes.
Cristian has been studying the impact of optimizations on debug information and thus the impact on AutoFDO profile quality. @mtrofin and I encouraged him to put this patch up for review so that we can reproduce his findings on internal workloads and share the insights with the rest of the community. I think this functionality will also help identify passes which drop debug information and help with the work proposed in this RFC. cc: @jmorse @SLTozer |
As it happens, I have been using a downstream modification of opt-bisect as part of my triage process for bugs detected with debugify, by counting optimization passes and printing them alongside detected bugs, making it easy to create reproducers using It would be useful in general for determining passes responsible for dropping debug information, however - I've previously spent time working on finding pass configurations that produce better debug info than the existing O2/O3 pipelines without compromising too greatly on performance - this feature would have made that process easier, and I suspect will make it easier for others to do similar investigations in future, so I give a vote for adding this! |
| /// through LLVMContext. | ||
| OptDisable() = default; | ||
|
|
||
| virtual ~OptDisable() = default; |
There was a problem hiding this comment.
you don't need this. Also, if you did: ~OptDisable() override
| @@ -37,8 +37,8 @@ static cl::opt<bool> OptBisectVerbose( | |||
| cl::desc("Show verbose output when opt-bisect-limit is set"), cl::Hidden, | |||
| cl::init(true), cl::Optional); | |||
|
|
|||
| static void printPassMessage(const StringRef &Name, int PassNum, | |||
| StringRef TargetDesc, bool Running) { | |||
| static void printBisectPassMessage(const StringRef &Name, int PassNum, | |||
There was a problem hiding this comment.
I'd recommend doing the name refactoring separately as a NFC change (Keeps this change smaller)
| return OptDisabler; | ||
| } | ||
|
|
||
| static cl::opt<std::string> OptDisablePass( |
There was a problem hiding this comment.
I think you can use cl::list<std::string>
| cl::desc("Optimization pass(es) to disable (comma separated)")); | ||
|
|
||
| static cl::opt<bool> | ||
| OptDisableVerbose("opt-disable-verbose", |
There was a problem hiding this comment.
I was confused by the flag when I read the commit message, I think I understand it now: it's verbosity of the opt-disable "thing". I read it initially as "disable verbosity" but everything says how it enables verbosity, except for the flag name itself :)
the best suggestion I can come with would be opt-disable-enable-verbosity. Not proud of it, but thinking that at least the reader is given pause to read the docstring of the flag.
Also, maybe say in the doc string that you're affecting the "opt-disable" pass. Then the dissonance may solve itself (for the reader).
|
|
||
| /// Parses the command line argument to extract the names of the passes | ||
| /// to be disabled. Multiple pass names can be provided with comma separation. | ||
| void setDisabled(StringRef Passes); |
There was a problem hiding this comment.
I think that if you model the command line flag as cl::list, this part of the API may be simpler, ptal.
| @@ -0,0 +1,13 @@ | |||
| // REQUIRES: x86-registered-target | |||
There was a problem hiding this comment.
should the test be in llvm/test? it's not tied to clang
I would like to propose a new pass gate that allows selective disabling of one or more passes via the clang command line using the
-opt-disableoption. Passes to be disabled should be specified as a comma-separated list of their names.The implementation resides in the same file as the bisection tool. The
getGlobalPassGate()function returns the currently enabled gate.Example:
-opt-disable="PassA,PassB"Pass names are matched using case-insensitive comparisons. However, note that special characters, including spaces, must be included exactly as they appear in the pass names.
Additionally, a
-opt-disable-verboseflag has been introduced to enable verbose output when this functionality is in use. When enabled, it prints the status of all passes (either running or NOT running), similar to the default behavior of-opt-bisect-limit. This flag is disabled by default, which is the opposite of the-opt-bisect-verboseflag (which defaults to enabled).To validate this add-on, a test file has also been provided. It reuses the same infrastructure as the opt-bisect test, but disables three specific passes and checks the output to ensure the expected behavior.