[SandboxVec] Add pass to create Regions from metadata. Generalize SandboxVec pass pipelines. #112288
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…dboxVec pass pipelines. My previous attempt (llvm#111904) hacked creation of Regions from metadata into the bottom-up vectorizer. I got some feedback that it should be its own pass. So now we have two SandboxIR function passes (`BottomUpVec` and `RegionsFromMetadata`) that are interchangeable, and we could have other SandboxIR function passes doing other kinds of transforms, so this commit revamps pipeline creation and parsing. First, `sandboxir::PassManager::setPassPipeline` now accepts pass arguments in angle brackets. Pass arguments are arbitrary strings that must be parsed by each pass, the only requirement is that nested angle bracket pairs must be balanced, to allow for nested pipelines with more arguments. For example: ``` bottom-up-vec<region-pass-1,region-pass-2<arg>,region-pass-3> ``` This has complicated the parser a little bit (the loop over pipeline characters now contains a small state machine), and we now have some new test cases to exercise the new features. The main SandboxVectorizerPass now contains a customizable pipeline of SandboxIR function passes, defined by the `sbvec-passes` flag. Region passes for the bottom-up vectorizer pass are now in pass arguments (like in the example above). Because we have now several classes that can build sub-pass pipelines, I've moved the logic that interacts with PassRegistry.def into its own files (PassBuilder.{h,cpp} so it can be easily reused. Finally, I've added a `RegionsFromMetadata` function pass, which will allow us to run region passes in isolation from lit tests without relying on the bottom-up vectorizer, and a new lit test that does exactly this. Note that the new pipeline parser now allows empty pipelines. This is useful for testing. For example, if we use ``` -sbvec-passes="bottom-up-vec<>" ``` SandboxVectorizer converts LLVM IR to SandboxIR and runs the bottom-up vectorizer, but no region passes afterwards. ``` -sbvec-passes="" ``` SandboxVectorizer converts LLVM IR to SandboxIR and runs no passes on it. This is useful to exercise SandboxIR conversion on its own.
slackito
requested review from
alinas,
vporpo,
tschuett,
aeubanks,
tmsri and
Sterling-Augustine
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-vectorizers Author: Jorge Gorbe Moya (slackito) ChangesMy previous attempt (#111904) hacked creation of Regions from metadata into the bottom-up vectorizer. I got some feedback that it should be its own pass. So now we have two SandboxIR function passes ( First, This has complicated the parser a little bit (the loop over pipeline characters now contains a small state machine), and we now have some new test cases to exercise the new features. The main SandboxVectorizerPass now contains a customizable pipeline of SandboxIR function passes, defined by the Because we have now several classes that can build sub-pass pipelines, I've moved the logic that interacts with PassRegistry.def into its own files (PassBuilder.{h,cpp} so it can be easily reused. Finally, I've added a Note that the new pipeline parser now allows empty pipelines. This is useful for testing. For example, if we use SandboxVectorizer converts LLVM IR to SandboxIR and runs the bottom-up vectorizer, but no region passes afterwards. SandboxVectorizer converts LLVM IR to SandboxIR and runs no passes on it. This is useful to exercise SandboxIR conversion on its own. Patch is 28.69 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/112288.diff 17 Files Affected:
diff --git a/llvm/include/llvm/SandboxIR/Pass.h b/llvm/include/llvm/SandboxIR/Pass.h
index 211f10f5d57c5c..5ed9d7442ee70c 100644
--- a/llvm/include/llvm/SandboxIR/Pass.h
+++ b/llvm/include/llvm/SandboxIR/Pass.h
@@ -43,7 +43,7 @@ class Pass {
LLVM_DUMP_METHOD virtual void dump() const;
#endif
/// Similar to print() but adds a newline. Used for testing.
- void printPipeline(raw_ostream &OS) const { OS << Name << "\n"; }
+ virtual void printPipeline(raw_ostream &OS) const { OS << Name << "\n"; }
};
/// A pass that runs on a sandbox::Function.
diff --git a/llvm/include/llvm/SandboxIR/PassManager.h b/llvm/include/llvm/SandboxIR/PassManager.h
index 247c43615f5766..705bd356c5155e 100644
--- a/llvm/include/llvm/SandboxIR/PassManager.h
+++ b/llvm/include/llvm/SandboxIR/PassManager.h
@@ -50,40 +50,130 @@ class PassManager : public ParentPass {
}
using CreatePassFunc =
- std::function<std::unique_ptr<ContainedPass>(StringRef)>;
+ std::function<std::unique_ptr<ContainedPass>(StringRef, StringRef)>;
/// Parses \p Pipeline as a comma-separated sequence of pass names and sets
/// the pass pipeline, using \p CreatePass to instantiate passes by name.
///
- /// After calling this function, the PassManager contains only the specified
- /// pipeline, any previously added passes are cleared.
+ /// Passes can have arguments, for example:
+ /// "pass1<arg1,arg2>,pass2,pass3<arg3,arg4>"
+ ///
+ /// The arguments between angle brackets are treated as a mostly opaque string
+ /// and each pass is responsible for parsing its arguments. The exception to
+ /// this are nested angle brackets, which must match pair-wise to allow
+ /// arguments to contain nested pipelines, like:
+ ///
+ /// "pass1<subpass1,subpass2<arg1,arg2>,subpass3>"
+ ///
+ /// An empty args string is treated the same as no args, so "pass" and
+ /// "pass<>" are equivalent.
void setPassPipeline(StringRef Pipeline, CreatePassFunc CreatePass) {
static constexpr const char EndToken = '\0';
+ static constexpr const char BeginArgsToken = '<';
+ static constexpr const char EndArgsToken = '>';
static constexpr const char PassDelimToken = ',';
assert(Passes.empty() &&
"setPassPipeline called on a non-empty sandboxir::PassManager");
- // Add EndToken to the end to ease parsing.
- std::string PipelineStr = std::string(Pipeline) + EndToken;
- int FlagBeginIdx = 0;
- for (auto [Idx, C] : enumerate(PipelineStr)) {
- // Keep moving Idx until we find the end of the pass name.
- bool FoundDelim = C == EndToken || C == PassDelimToken;
- if (!FoundDelim)
- continue;
- unsigned Sz = Idx - FlagBeginIdx;
- std::string PassName(&PipelineStr[FlagBeginIdx], Sz);
- FlagBeginIdx = Idx + 1;
+ // Accept an empty pipeline as a special case. This can be useful, for
+ // example, to test conversion to SandboxIR without running any passes on
+ // it.
+ if (Pipeline.empty())
+ return;
+ // Add EndToken to the end to ease parsing.
+ std::string PipelineStr = std::string(Pipeline) + EndToken;
+ Pipeline = StringRef(PipelineStr);
+
+ enum {
+ ScanName, // reading a pass name
+ ScanArgs, // reading a list of args
+ ArgsEnded, // read the last '>' in an args list, must read delimiter next
+ } State;
+ State = ScanName;
+ int PassBeginIdx = 0;
+ int ArgsBeginIdx;
+ StringRef PassName;
+ StringRef PassArgs;
+ int NestedArgs = 0;
+
+ auto AddPass = [this, CreatePass](StringRef PassName, StringRef PassArgs) {
+ if (PassName.empty()) {
+ errs() << "Found empty pass name.\n";
+ exit(1);
+ }
// Get the pass that corresponds to PassName and add it to the pass
// manager.
- auto Pass = CreatePass(PassName);
+ auto Pass = CreatePass(PassName, PassArgs);
if (Pass == nullptr) {
errs() << "Pass '" << PassName << "' not registered!\n";
exit(1);
}
addPass(std::move(Pass));
+ };
+ for (auto [Idx, C] : enumerate(Pipeline)) {
+ switch (State) {
+ case ScanName:
+ if (C == BeginArgsToken) {
+ // Save pass name for later and begin scanning args.
+ PassName = Pipeline.slice(PassBeginIdx, Idx);
+ ArgsBeginIdx = Idx + 1;
+ ++NestedArgs;
+ State = ScanArgs;
+ break;
+ }
+ if (C == EndArgsToken) {
+ errs() << "Unexpected '>' in pass pipeline.\n";
+ exit(1);
+ }
+ if (C == EndToken || C == PassDelimToken) {
+ // Delimiter found, add the pass (with empty args), stay in the
+ // ScanName state.
+ AddPass(Pipeline.slice(PassBeginIdx, Idx), StringRef());
+ PassBeginIdx = Idx + 1;
+ }
+ break;
+ case ScanArgs:
+ // While scanning args, we only care about making sure nesting of angle
+ // brackets is correct.
+ if (C == BeginArgsToken) {
+ ++NestedArgs;
+ break;
+ }
+ if (C == EndArgsToken) {
+ --NestedArgs;
+ if (NestedArgs == 0) {
+ // Done scanning args.
+ PassArgs = Pipeline.slice(ArgsBeginIdx, Idx);
+ AddPass(PassName, PassArgs);
+ State = ArgsEnded;
+ } else if (NestedArgs < 0) {
+ errs() << "Unbalanced '>' in pass pipeline.\n";
+ exit(1);
+ }
+ break;
+ }
+ if (C == EndToken) {
+ errs() << "Missing '>' in pass pipeline. End-of-string reached while "
+ "reading arguments for pass '"
+ << PassName << "'.\n";
+ exit(1);
+ }
+ break;
+ case ArgsEnded:
+ // Once we're done scanning args, only a delimiter is valid. This avoids
+ // accepting strings like "foo<args><more-args>" or "foo<args>bar".
+ if (C == EndToken || C == PassDelimToken) {
+ PassBeginIdx = Idx + 1;
+ State = ScanName;
+ } else {
+ errs() << "Expected delimiter or end-of-string after pass "
+ "arguments.\n";
+ exit(1);
+ }
+ break;
+ }
}
}
@@ -101,7 +191,7 @@ class PassManager : public ParentPass {
}
#endif
/// Similar to print() but prints one pass per line. Used for testing.
- void printPipeline(raw_ostream &OS) const {
+ void printPipeline(raw_ostream &OS) const override {
OS << this->getName() << "\n";
for (const auto &PassPtr : Passes)
PassPtr->printPipeline(OS);
diff --git a/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/Passes/BottomUpVec.h b/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/Passes/BottomUpVec.h
index 02abdf0a1ef0df..5cd47efd6b3462 100644
--- a/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/Passes/BottomUpVec.h
+++ b/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/Passes/BottomUpVec.h
@@ -13,15 +13,15 @@
#define LLVM_TRANSFORMS_VECTORIZE_SANDBOXVECTORIZER_PASSES_BOTTOMUPVEC_H
#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/StringRef.h"
#include "llvm/SandboxIR/Constant.h"
#include "llvm/SandboxIR/Pass.h"
#include "llvm/SandboxIR/PassManager.h"
+#include "llvm/Support/raw_ostream.h"
#include "llvm/Transforms/Vectorize/SandboxVectorizer/Legality.h"
namespace llvm::sandboxir {
-class RegionPassManager;
-
class BottomUpVec final : public FunctionPass {
bool Change = false;
LegalityAnalysis Legality;
@@ -32,8 +32,12 @@ class BottomUpVec final : public FunctionPass {
RegionPassManager RPM;
public:
- BottomUpVec();
+ BottomUpVec(StringRef Pipeline);
bool runOnFunction(Function &F) final;
+ void printPipeline(raw_ostream &OS) const final {
+ OS << getName() << "\n";
+ RPM.printPipeline(OS);
+ }
};
} // namespace llvm::sandboxir
diff --git a/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/Passes/PrintInstructionCount.h b/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/Passes/PrintInstructionCount.h
new file mode 100644
index 00000000000000..9d88bc82803847
--- /dev/null
+++ b/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/Passes/PrintInstructionCount.h
@@ -0,0 +1,23 @@
+#ifndef LLVM_TRANSFORMS_VECTORIZE_SANDBOXVECTORIZER_PASSES_PRINTINSTRUCTIONCOUNT_H
+#define LLVM_TRANSFORMS_VECTORIZE_SANDBOXVECTORIZER_PASSES_PRINTINSTRUCTIONCOUNT_H
+
+#include "llvm/SandboxIR/Pass.h"
+#include "llvm/SandboxIR/Region.h"
+
+namespace llvm::sandboxir {
+
+/// A Region pass that prints the instruction count for the region to stdout.
+/// Used to test -sbvec-passes while we don't have any actual optimization
+/// passes.
+class PrintInstructionCount final : public RegionPass {
+public:
+ PrintInstructionCount() : RegionPass("null") {}
+ bool runOnRegion(Region &R) final {
+ outs() << "InstructionCount: " << std::distance(R.begin(), R.end()) << "\n";
+ return false;
+ }
+};
+
+} // namespace llvm::sandboxir
+
+#endif // LLVM_TRANSFORMS_VECTORIZE_SANDBOXVECTORIZER_PASSES_PRINTINSTRUCTIONCOUNTPASS_H
diff --git a/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/Passes/RegionsFromMetadata.h b/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/Passes/RegionsFromMetadata.h
new file mode 100644
index 00000000000000..09347558f940fa
--- /dev/null
+++ b/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/Passes/RegionsFromMetadata.h
@@ -0,0 +1,34 @@
+//===- RegionsFromMetadata.h ------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// A SandboxIR function pass that builds regions from IR metadata and then runs
+// a pipeline of region passes on them. This is useful to test region passes in
+// isolation without relying on the output of the bottom-up vectorizer.
+//
+
+#ifndef LLVM_TRANSFORMS_VECTORIZE_SANDBOXVECTORIZER_PASSES_REGIONSFROMMETADATA_H
+#define LLVM_TRANSFORMS_VECTORIZE_SANDBOXVECTORIZER_PASSES_REGIONSFROMMETADATA_H
+
+#include "llvm/ADT/StringRef.h"
+#include "llvm/SandboxIR/Pass.h"
+#include "llvm/SandboxIR/PassManager.h"
+
+namespace llvm::sandboxir {
+
+class RegionsFromMetadata final : public FunctionPass {
+ // The PM containing the pipeline of region passes.
+ RegionPassManager RPM;
+
+public:
+ RegionsFromMetadata(StringRef Pipeline);
+ bool runOnFunction(Function &F) final;
+};
+
+} // namespace llvm::sandboxir
+
+#endif // LLVM_TRANSFORMS_VECTORIZE_SANDBOXVECTORIZER_PASSES_REGIONSFROMMETADATA_H
diff --git a/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/SandboxVectorizer.h b/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/SandboxVectorizer.h
index b7cb418c00326a..b83744cf9e6cb6 100644
--- a/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/SandboxVectorizer.h
+++ b/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/SandboxVectorizer.h
@@ -11,7 +11,7 @@
#include <memory>
#include "llvm/IR/PassManager.h"
-#include "llvm/Transforms/Vectorize/SandboxVectorizer/Passes/BottomUpVec.h"
+#include "llvm/SandboxIR/PassManager.h"
namespace llvm {
@@ -20,8 +20,8 @@ class TargetTransformInfo;
class SandboxVectorizerPass : public PassInfoMixin<SandboxVectorizerPass> {
TargetTransformInfo *TTI = nullptr;
- // The main vectorizer pass.
- sandboxir::BottomUpVec BottomUpVecPass;
+ // A pipeline of SandboxIR function passes run by the vectorizer.
+ sandboxir::FunctionPassManager FPM;
bool runImpl(Function &F);
diff --git a/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/SandboxVectorizerPassBuilder.h b/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/SandboxVectorizerPassBuilder.h
new file mode 100644
index 00000000000000..e3d6ecae836fce
--- /dev/null
+++ b/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/SandboxVectorizerPassBuilder.h
@@ -0,0 +1,32 @@
+//===- SandboxVectorizerPassBuilder.h ---------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// Utility functions so passes with sub-pipelines can create SandboxVectorizer
+// passes without replicating the same logic in each pass.
+//
+#ifndef LLVM_TRANSFORMS_VECTORIZE_SANDBOXVECTORIZER_SANDBOXVECTORIZERPASSBUILDER_H
+#define LLVM_TRANSFORMS_VECTORIZE_SANDBOXVECTORIZER_SANDBOXVECTORIZERPASSBUILDER_H
+
+#include "llvm/ADT/StringRef.h"
+#include "llvm/SandboxIR/Pass.h"
+
+#include <memory>
+
+namespace llvm::sandboxir {
+
+class SandboxVectorizerPassBuilder {
+public:
+ static std::unique_ptr<FunctionPass> createFunctionPass(StringRef Name,
+ StringRef Args);
+ static std::unique_ptr<RegionPass> createRegionPass(StringRef Name,
+ StringRef Args);
+};
+
+} // namespace llvm::sandboxir
+
+#endif // LLVM_TRANSFORMS_VECTORIZE_SANDBOXVECTORIZER_SANDBOXVECTORIZERPASSBUILDER_H
diff --git a/llvm/lib/Transforms/Vectorize/CMakeLists.txt b/llvm/lib/Transforms/Vectorize/CMakeLists.txt
index 9c2e7c1e0c5bc7..f4e98e576379a4 100644
--- a/llvm/lib/Transforms/Vectorize/CMakeLists.txt
+++ b/llvm/lib/Transforms/Vectorize/CMakeLists.txt
@@ -6,7 +6,9 @@ add_llvm_component_library(LLVMVectorize
SandboxVectorizer/DependencyGraph.cpp
SandboxVectorizer/Interval.cpp
SandboxVectorizer/Passes/BottomUpVec.cpp
+ SandboxVectorizer/Passes/RegionsFromMetadata.cpp
SandboxVectorizer/SandboxVectorizer.cpp
+ SandboxVectorizer/SandboxVectorizerPassBuilder.cpp
SandboxVectorizer/SeedCollector.cpp
SLPVectorizer.cpp
Vectorize.cpp
diff --git a/llvm/lib/Transforms/Vectorize/SandboxVectorizer/Passes/BottomUpVec.cpp b/llvm/lib/Transforms/Vectorize/SandboxVectorizer/Passes/BottomUpVec.cpp
index 77198f932a3ec0..1a4b70c4267c7f 100644
--- a/llvm/lib/Transforms/Vectorize/SandboxVectorizer/Passes/BottomUpVec.cpp
+++ b/llvm/lib/Transforms/Vectorize/SandboxVectorizer/Passes/BottomUpVec.cpp
@@ -7,42 +7,17 @@
//===----------------------------------------------------------------------===//
#include "llvm/Transforms/Vectorize/SandboxVectorizer/Passes/BottomUpVec.h"
+
#include "llvm/ADT/SmallVector.h"
#include "llvm/SandboxIR/Function.h"
#include "llvm/SandboxIR/Instruction.h"
-#include "llvm/Support/CommandLine.h"
-#include "llvm/Transforms/Vectorize/SandboxVectorizer/Passes/NullPass.h"
+#include "llvm/Transforms/Vectorize/SandboxVectorizer/SandboxVectorizerPassBuilder.h"
namespace llvm::sandboxir {
-static cl::opt<bool>
- PrintPassPipeline("sbvec-print-pass-pipeline", cl::init(false), cl::Hidden,
- cl::desc("Prints the pass pipeline and returns."));
-
-/// A magic string for the default pass pipeline.
-static const char *DefaultPipelineMagicStr = "*";
-
-static cl::opt<std::string> UserDefinedPassPipeline(
- "sbvec-passes", cl::init(DefaultPipelineMagicStr), cl::Hidden,
- cl::desc("Comma-separated list of vectorizer passes. If not set "
- "we run the predefined pipeline."));
-
-static std::unique_ptr<RegionPass> createRegionPass(StringRef Name) {
-#define REGION_PASS(NAME, CREATE_PASS) \
- if (Name == NAME) \
- return std::make_unique<decltype(CREATE_PASS)>(CREATE_PASS);
-#include "PassRegistry.def"
- return nullptr;
-}
-
-BottomUpVec::BottomUpVec() : FunctionPass("bottom-up-vec"), RPM("rpm") {
- // Create a pipeline to be run on each Region created by BottomUpVec.
- if (UserDefinedPassPipeline == DefaultPipelineMagicStr) {
- // TODO: Add default passes to RPM.
- } else {
- // Create the user-defined pipeline.
- RPM.setPassPipeline(UserDefinedPassPipeline, createRegionPass);
- }
+BottomUpVec::BottomUpVec(StringRef Pipeline)
+ : FunctionPass("bottom-up-vec"), RPM("rpm") {
+ RPM.setPassPipeline(Pipeline, SandboxVectorizerPassBuilder::createRegionPass);
}
// TODO: This is a temporary function that returns some seeds.
@@ -82,11 +57,6 @@ void BottomUpVec::vectorizeRec(ArrayRef<Value *> Bndl) {
void BottomUpVec::tryVectorize(ArrayRef<Value *> Bndl) { vectorizeRec(Bndl); }
bool BottomUpVec::runOnFunction(Function &F) {
- if (PrintPassPipeline) {
- RPM.printPipeline(outs());
- return false;
- }
-
Change = false;
// TODO: Start from innermost BBs first
for (auto &BB : F) {
diff --git a/llvm/lib/Transforms/Vectorize/SandboxVectorizer/Passes/PassRegistry.def b/llvm/lib/Transforms/Vectorize/SandboxVectorizer/Passes/PassRegistry.def
index bbb0dcba1ea516..0dc72842f1abe0 100644
--- a/llvm/lib/Transforms/Vectorize/SandboxVectorizer/Passes/PassRegistry.def
+++ b/llvm/lib/Transforms/Vectorize/SandboxVectorizer/Passes/PassRegistry.def
@@ -14,9 +14,19 @@
// NOTE: NO INCLUDE GUARD DESIRED!
#ifndef REGION_PASS
-#define REGION_PASS(NAME, CREATE_PASS)
+#define REGION_PASS(NAME, CLASS_NAME)
#endif
-REGION_PASS("null", NullPass())
+REGION_PASS("null", ::llvm::sandboxir::NullPass)
+REGION_PASS("print-instruction-count", ::llvm::sandboxir::PrintInstructionCount)
#undef REGION_PASS
+
+#ifndef FUNCTION_PASS_WITH_PARAMS
+#define FUNCTION_PASS_WITH_PARAMS(NAME, CLASS_NAME)
+#endif
+
+FUNCTION_PASS_WITH_PARAMS("bottom-up-vec", ::llvm::sandboxir::BottomUpVec)
+FUNCTION_PASS_WITH_PARAMS("regions-from-metadata", ::llvm::sandboxir::RegionsFromMetadata)
+
+#undef FUNCTION_PASS_WITH_PARAMS
diff --git a/llvm/lib/Transforms/Vectorize/SandboxVectorizer/Passes/RegionsFromMetadata.cpp b/llvm/lib/Transforms/Vectorize/SandboxVectorizer/Passes/RegionsFromMetadata.cpp
new file mode 100644
index 00000000000000..4cebbdcd3192a6
--- /dev/null
+++ b/llvm/lib/Transforms/Vectorize/SandboxVectorizer/Passes/RegionsFromMetadata.cpp
@@ -0,0 +1,30 @@
+//===- RegionsFromMetadata.cpp - A helper to test RegionPasses -----------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/Transforms/Vectorize/SandboxVectorizer/Passes/RegionsFromMetadata.h"
+
+#include "llvm/SandboxIR/Region.h"
+#include "llvm/Transforms/Vectorize/SandboxVectorizer/SandboxVectorizerPassBuilder.h"
+
+namespace llvm::sandboxir {
+
+RegionsFromMetadata::RegionsFromMetadata(StringRef Pipeline)
+ : FunctionPass("regions-from-metadata"), RPM("rpm") {
+ RPM.setPassPipeline(Pipeline, SandboxVectorizerPassBuilder::createRegionPass);
+}
+
+bool RegionsFromMetadata::runOnFunction(Function &F) {
+ SmallVector<std::unique_ptr<sandboxir::Region>> Regions =
+ sandboxir::Region::createRegionsFromMD(F);
+ for (auto &R : Regions) {
+ RPM.runOnRegion(*R);
+ }
+ return false;
+}
+
+} // namespace llvm::sandboxir
diff --git a/llvm/lib/Transforms/Vectorize/SandboxVectorizer/SandboxVectorizer.cpp b/llvm/lib/Transforms/Vectorize/SandboxVectorizer/SandboxVectorizer.cpp
index ba4899cc624e99..c68f9482e337dd 100644
--- a/llvm/lib/Transforms/Vectorize/SandboxVectorizer/SandboxVectorizer.cpp
+++ b/llvm/lib/Transforms/Vectorize/SandboxVectorizer/SandboxVectorizer.cpp
@@ -9,14 +9,39 @@
#include "llvm/Transforms/Vectorize/SandboxVectorizer/SandboxVectorizer.h"
#include "llvm/Analysis/TargetTransformInfo.h"
#include "llvm/SandboxIR/Constant.h"
-#include "llvm/Transforms/Vectorize/SandboxVectorizer/Passes/BottomUpVec.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Transforms/Vectorize/SandboxVectorizer/SandboxVectorizerPassBuilder.h"
using namespace llvm;
#define SV_NAME "sandbox-vectorizer"
#define DEBUG_TYPE SV_NAME
-SandboxVectorizerPass::SandboxVectorizerPass() = default;
+static cl::opt<bool>
+ PrintPassPipeline("sbvec-print-pass-pipeline", cl::init(false), cl::Hidden,
+ cl::desc("Prints the pass pipeline and returns."));
+
+/// A magic string for the default pass pipeline.
+static const char *DefaultPipelineMagicStr = "*";
+
+static cl::opt<std::string> UserDefinedPassPipeline...
[truncated]
|
aeubanks
reviewed
Oct 15, 2024
vporpo
reviewed
Oct 15, 2024
| @@ -43,7 +43,7 @@ class Pass { | |||
| LLVM_DUMP_METHOD virtual void dump() const; | |||
| #endif | |||
| /// Similar to print() but adds a newline. Used for testing. | |||
| void printPipeline(raw_ostream &OS) const { OS << Name << "\n"; } | |||
| virtual void printPipeline(raw_ostream &OS) const { OS << Name << "\n"; } | |||
There was a problem hiding this comment.
Shouldn't this be guarde by #ifndef NDEBUG ?
There was a problem hiding this comment.
That's the code that implements the -sbvec-print-pass-pipeline flag. And it wasn't guarded before. Do we want the flag to only work in debug builds? If so, should we guard the flag as well? (also the tests that rely on printing the pass pipeline will become unsupported in release builds) I think those questions are out of scope for this PR.
There was a problem hiding this comment.
Ah, it's because of the tests, makes sense.
| @@ -50,40 +50,130 @@ class PassManager : public ParentPass { | |||
| } | |||
|
|
|||
| using CreatePassFunc = | |||
| std::function<std::unique_ptr<ContainedPass>(StringRef)>; | |||
| std::function<std::unique_ptr<ContainedPass>(StringRef, StringRef)>; | |||
There was a problem hiding this comment.
Could you add a brief comment that somehow explains what each argument is, perhaps something like: /// CreatePassFunc(PassName, PassArgs)
| std::string PipelineStr = std::string(Pipeline) + EndToken; | ||
| Pipeline = StringRef(PipelineStr); | ||
|
|
||
| enum { |
There was a problem hiding this comment.
It's a small, local enum, and IMO it worked well as an anonymous enum (less verbose, can use the name State for the variable). Switched to enum class State, renamed the variable to CurrentState.
There was a problem hiding this comment.
I think you just get better warnings with an enum class.
| State = ScanName; | ||
| int PassBeginIdx = 0; | ||
| int ArgsBeginIdx; | ||
| StringRef PassName; |
There was a problem hiding this comment.
I would expect this to be declared closer to where it's used, like for example at the top of the for loop (line116).
There was a problem hiding this comment.
The AddPass lambda is also only used in the loop. But sure, moved all the state variables right before the loop and after the code for AddPass.
| int PassBeginIdx = 0; | ||
| int ArgsBeginIdx; | ||
| StringRef PassName; | ||
| StringRef PassArgs; |
There was a problem hiding this comment.
Same, line 148, StringRef PassArgs = Pipeline.slice(...) ?
There was a problem hiding this comment.
A previous version of the code needed a PassArgs variable because it called AddPass later in the ArgsEnded state. Now we don't even need it, I've inlined Pipeline.slice(...) into the call to AddPass.
| #endif | ||
|
|
||
| REGION_PASS("null", NullPass()) | ||
| REGION_PASS("null", ::llvm::sandboxir::NullPass) |
There was a problem hiding this comment.
Why do we need to specify the namespace ::llvm::sandboxir:: here?
There was a problem hiding this comment.
We don't NEED to, but with the vectorizer having code in both the llvm and llvm::sandboxir namespaces, I thought this makes it easier to include the .def file without having to care about the surrounding namespace.
Now that I've introduced the SandboxVectorizerPassBuilder helper it's less of a concern because that's probably the only place that will need to include the .def file. I think it doesn't hurt, but if you'd prefer not having the qualifier I can remove it.
There was a problem hiding this comment.
I don't feel strongly about it, but if it works fine without the qualifiers I would prefer them that way because it's just easier to read.
| EXPECT_DEATH(FPM2.setPassPipeline(">foo", CreatePass), ".*"); | ||
| EXPECT_DEATH(FPM2.setPassPipeline("<>foo", CreatePass), ".*"); | ||
| EXPECT_DEATH(FPM2.setPassPipeline("foo<args><more-args>", CreatePass), ".*"); | ||
| EXPECT_DEATH(FPM2.setPassPipeline("foo<args>bar", CreatePass), ".*"); |
There was a problem hiding this comment.
EXPECT_DEATH(FPM2.setPassPipeline("foo<bar"));
| ".*empty pass name.*"); | ||
|
|
||
| // Mismatched argument brackets. | ||
| EXPECT_DEATH(FPM2.setPassPipeline("foo<", CreatePass), ".*"); |
There was a problem hiding this comment.
Try to match an important word from the error message, like .*WORD.* rather than .*
|
|
||
| RegionsFromMetadata::RegionsFromMetadata(StringRef Pipeline) | ||
| : FunctionPass("regions-from-metadata"), RPM("rpm") { | ||
| RPM.setPassPipeline(Pipeline, SandboxVectorizerPassBuilder::createRegionPass); |
There was a problem hiding this comment.
Would it be possible to set the pipeline in RPM's constructor? rather than having to call setPassPipeline ?
There was a problem hiding this comment.
It's possible. Added a PassManager(StringRef Name, StringRef Pipeline, CreatePassFunc CreatePass) constructor to PassManager and similar ones to FunctionPassManager and RegionPassManager. Now both RegionsFromMetadata and BottomUpVec initialize the pipeline using the new RPM constructor
What's the advantage you see in doing so?
There was a problem hiding this comment.
It's just that it forces you to initialize the pipeline when you define a pass.
We don't have a null function pass (we could add it!) but also nothing prevents us from using I added a test case for it, and it helped me realize (and fix) that I hadn't added a |
DanielCChen
pushed a commit
to DanielCChen/llvm-project
that referenced
this pull request
Oct 16, 2024
…dboxVec pass pipelines. (llvm#112288) My previous attempt (llvm#111904) hacked creation of Regions from metadata into the bottom-up vectorizer. I got some feedback that it should be its own pass. So now we have two SandboxIR function passes (`BottomUpVec` and `RegionsFromMetadata`) that are interchangeable, and we could have other SandboxIR function passes doing other kinds of transforms, so this commit revamps pipeline creation and parsing. First, `sandboxir::PassManager::setPassPipeline` now accepts pass arguments in angle brackets. Pass arguments are arbitrary strings that must be parsed by each pass, the only requirement is that nested angle bracket pairs must be balanced, to allow for nested pipelines with more arguments. For example: ``` bottom-up-vec<region-pass-1,region-pass-2<arg>,region-pass-3> ``` This has complicated the parser a little bit (the loop over pipeline characters now contains a small state machine), and we now have some new test cases to exercise the new features. The main SandboxVectorizerPass now contains a customizable pipeline of SandboxIR function passes, defined by the `sbvec-passes` flag. Region passes for the bottom-up vectorizer pass are now in pass arguments (like in the example above). Because we have now several classes that can build sub-pass pipelines, I've moved the logic that interacts with PassRegistry.def into its own files (PassBuilder.{h,cpp} so it can be easily reused. Finally, I've added a `RegionsFromMetadata` function pass, which will allow us to run region passes in isolation from lit tests without relying on the bottom-up vectorizer, and a new lit test that does exactly this. Note that the new pipeline parser now allows empty pipelines. This is useful for testing. For example, if we use ``` -sbvec-passes="bottom-up-vec<>" ``` SandboxVectorizer converts LLVM IR to SandboxIR and runs the bottom-up vectorizer, but no region passes afterwards. ``` -sbvec-passes="" ``` SandboxVectorizer converts LLVM IR to SandboxIR and runs no passes on it. This is useful to exercise SandboxIR conversion on its own.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
My previous attempt (#111904) hacked creation of Regions from metadata into the bottom-up vectorizer. I got some feedback that it should be its own pass. So now we have two SandboxIR function passes (
BottomUpVecandRegionsFromMetadata) that are interchangeable, and we could have other SandboxIR function passes doing other kinds of transforms, so this commit revamps pipeline creation and parsing.First,
sandboxir::PassManager::setPassPipelinenow accepts pass arguments in angle brackets. Pass arguments are arbitrary strings that must be parsed by each pass, the only requirement is that nested angle bracket pairs must be balanced, to allow for nested pipelines with more arguments. For example:This has complicated the parser a little bit (the loop over pipeline characters now contains a small state machine), and we now have some new test cases to exercise the new features.
The main SandboxVectorizerPass now contains a customizable pipeline of SandboxIR function passes, defined by the
sbvec-passesflag. Region passes for the bottom-up vectorizer pass are now in pass arguments (like in the example above).Because we have now several classes that can build sub-pass pipelines, I've moved the logic that interacts with PassRegistry.def into its own files (PassBuilder.{h,cpp} so it can be easily reused.
Finally, I've added a
RegionsFromMetadatafunction pass, which will allow us to run region passes in isolation from lit tests without relying on the bottom-up vectorizer, and a new lit test that does exactly this.Note that the new pipeline parser now allows empty pipelines. This is useful for testing. For example, if we use
SandboxVectorizer converts LLVM IR to SandboxIR and runs the bottom-up vectorizer, but no region passes afterwards.
SandboxVectorizer converts LLVM IR to SandboxIR and runs no passes on it. This is useful to exercise SandboxIR conversion on its own.