[OptBisect] Add opt-bisect-skip option for skipping passes during bisection #152393
Conversation
|
@llvm/pr-subscribers-llvm-ir Author: Yonah Goldberg (YonahGoldberg) ChangesAdd -opt-bisect-skip=<idx> option for skipping pass at index <idx> in the pipeline. OptBisect allows you to binary search for a pass causing some error. It is useful as a next step to disable that pass as a sanity check and to see if the issue is isolated to one pass. Full diff: https://github.com/llvm/llvm-project/pull/152393.diff 3 Files Affected:
diff --git a/llvm/include/llvm/IR/OptBisect.h b/llvm/include/llvm/IR/OptBisect.h
index d813ae933d65e..9dcc0077552d4 100644
--- a/llvm/include/llvm/IR/OptBisect.h
+++ b/llvm/include/llvm/IR/OptBisect.h
@@ -14,6 +14,7 @@
#ifndef LLVM_IR_OPTBISECT_H
#define LLVM_IR_OPTBISECT_H
+#include "llvm/ADT/SmallSet.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/ADT/StringSet.h"
#include "llvm/Support/Compiler.h"
@@ -67,7 +68,13 @@ class LLVM_ABI OptBisect : public OptPassGate {
StringRef IRDescription) const override;
/// isEnabled() should return true before calling shouldRunPass().
- bool isEnabled() const override { return BisectLimit != Disabled; }
+ bool isEnabled() const override {
+ return BisectLimit != Disabled || !BisectSkipNumbers.empty();
+ }
+
+ /// Add pass at index SkipNumber to the list of passes to skip
+ /// during bisection.
+ void addSkip(int SkipNumber) { BisectSkipNumbers.insert(SkipNumber); }
/// Set the new optimization limit and reset the counter. Passing
/// OptBisect::Disabled disables the limiting.
@@ -81,6 +88,7 @@ class LLVM_ABI OptBisect : public OptPassGate {
private:
int BisectLimit = Disabled;
mutable int LastBisectNum = 0;
+ SmallSet<int, 4> BisectSkipNumbers;
};
/// This class implements a mechanism to disable passes and individual
diff --git a/llvm/lib/IR/OptBisect.cpp b/llvm/lib/IR/OptBisect.cpp
index 29ca268408265..defb8a98f9716 100644
--- a/llvm/lib/IR/OptBisect.cpp
+++ b/llvm/lib/IR/OptBisect.cpp
@@ -37,6 +37,11 @@ static cl::opt<int> OptBisectLimit("opt-bisect-limit", cl::Hidden,
}),
cl::desc("Maximum optimization to perform"));
+static cl::opt<int> OptBisectSkip(
+ "opt-bisect-skip", cl::Hidden, cl::init(OptBisect::Disabled), cl::Optional,
+ cl::cb<void, int>([](int PassNum) { getOptBisector().addSkip(PassNum); }),
+ cl::desc("Skip pass at the given index in the optimization pipeline"));
+
static cl::opt<bool> OptBisectVerbose(
"opt-bisect-verbose",
cl::desc("Show verbose output when opt-bisect-limit is set"), cl::Hidden,
@@ -66,7 +71,9 @@ bool OptBisect::shouldRunPass(StringRef PassName,
assert(isEnabled());
int CurBisectNum = ++LastBisectNum;
- bool ShouldRun = (BisectLimit == -1 || CurBisectNum <= BisectLimit);
+ bool ShouldRun = (BisectLimit == -1 || BisectLimit == Disabled ||
+ CurBisectNum <= BisectLimit) &&
+ !BisectSkipNumbers.contains(CurBisectNum);
if (OptBisectVerbose)
printPassMessage(PassName, CurBisectNum, IRDescription, ShouldRun);
return ShouldRun;
diff --git a/llvm/test/Other/opt-bisect-skip.ll b/llvm/test/Other/opt-bisect-skip.ll
new file mode 100644
index 0000000000000..da94ad6230527
--- /dev/null
+++ b/llvm/test/Other/opt-bisect-skip.ll
@@ -0,0 +1,15 @@
+; Test that verifies functionality for -opt-bisect-skip
+
+; RUN: opt -O1 -opt-bisect-skip=3 -opt-bisect-skip=7 %s 2>&1 | FileCheck %s --check-prefix=CHECK-DISABLE-PASS
+; CHECK-DISABLE-PASS: BISECT: running pass (1) annotation2metadata on [module]
+; CHECK-DISABLE-PASS: BISECT: running pass (2) forceattrs on [module]
+; CHECK-DISABLE-PASS: BISECT: NOT running pass (3) inferattrs on [module]
+; CHECK-DISABLE-PASS: BISECT: running pass (4) lower-expect on foo
+; CHECK-DISABLE-PASS: BISECT: running pass (5) simplifycfg on foo
+; CHECK-DISABLE-PASS: BISECT: running pass (6) sroa on foo
+; CHECK-DISABLE-PASS: BISECT: NOT running pass (7) early-cse on foo
+; CHECK-DISABLE-PASS: BISECT: running pass (8) openmp-opt on [module]
+
+define void @foo() {
+ ret void
+}
\ No newline at end of file
|
llvm/test/Other/opt-bisect-skip.ll
Outdated
| @@ -0,0 +1,15 @@ | |||
| ; Test that verifies functionality for -opt-bisect-skip | |||
|
|
|||
| ; RUN: opt -O1 -opt-bisect-skip=3 -opt-bisect-skip=7 %s 2>&1 | FileCheck %s --check-prefix=CHECK-DISABLE-PASS | |||
There was a problem hiding this comment.
It would be nice to support this as a comma separated list, similar to "opt-disable".
llvm/lib/IR/OptBisect.cpp
Outdated
| @@ -37,6 +37,11 @@ static cl::opt<int> OptBisectLimit("opt-bisect-limit", cl::Hidden, | |||
| }), | |||
| cl::desc("Maximum optimization to perform")); | |||
|
|
|||
| static cl::opt<int> OptBisectSkip( | |||
| "opt-bisect-skip", cl::Hidden, cl::init(OptBisect::Disabled), cl::Optional, | |||
There was a problem hiding this comment.
Nit: this name is a bit odd since we're not really bisecting. Not sure what a better name would be maybe "opt-disable-indices" but I'm not crazy about that either?
There was a problem hiding this comment.
I changed to your naming scheme bc I agree it's better, open to other suggestions though.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| @@ -67,7 +68,13 @@ class LLVM_ABI OptBisect : public OptPassGate { | |||
| StringRef IRDescription) const override; | |||
|
|
|||
| /// isEnabled() should return true before calling shouldRunPass(). | |||
| bool isEnabled() const override { return BisectLimit != Disabled; } | |||
| bool isEnabled() const override { | |||
| return BisectLimit != Disabled || !BisectSkipNumbers.empty(); | |||
There was a problem hiding this comment.
It's looks very odd.
Imagine a user sets bisect limit to disable all passes, and also passes an option to disable a few specific passes.
My mental model is that BisectLimit == Disabled should be functionally equivalent to BisectSkipNumbers containing complete set of passes, and when it's in effect BisectSkipNumbers should effectively become a no-op.
Right now we'll end up running the passes, despite the user asking for the opposite.
There was a problem hiding this comment.
I think it would be better to make opt-bisect work the same way that debug-counter works nowadays, that is by specifying a list of ranges. So that you can use something like -opt-bisect=1-1000 to get something similar to the current opt-bisect-limit, but also -opt-bisect=1-10,20-30 to skip a certain range of passes, etc.
Add -opt-bisect-skip=<idx> option for skipping pass at index <idx> in the pipeline.
OptBisect allows you to binary search for a pass causing some error. It is useful as a next step to disable that pass as a sanity check and to see if the issue is isolated to one pass.