Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,19 @@

namespace llvm::sandboxir {

class RegionPassManager;

class BottomUpVec final : public FunctionPass {
bool Change = false;
LegalityAnalysis Legality;
void vectorizeRec(ArrayRef<Value *> Bndl);
void tryVectorize(ArrayRef<Value *> Seeds);

[[maybe_unused]] RegionPassManager *RPM;

public:
BottomUpVec() : FunctionPass("bottom-up-vec") {}
BottomUpVec(RegionPassManager *RPM)
: FunctionPass("bottom-up-vec"), RPM(RPM) {}
bool runOnFunction(Function &F) final;
};

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
#ifndef LLVM_TRANSFORMS_VECTORIZE_SANDBOXVECTORIZER_PASSES_NULLPASS_H
#define LLVM_TRANSFORMS_VECTORIZE_SANDBOXVECTORIZER_PASSES_NULLPASS_H

#include "llvm/SandboxIR/Pass.h"

namespace llvm::sandboxir {

class Region;

/// A Region pass that does nothing, for use as a placeholder in tests.
class NullPass final : public RegionPass {
public:
NullPass() : RegionPass("null") {}
bool runOnRegion(Region &R) final { return false; }
};

} // namespace llvm::sandboxir

#endif // LLVM_TRANSFORMS_VECTORIZE_SANDBOXVECTORIZER_PASSES_NULLPASS_H
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,11 @@
#ifndef LLVM_TRANSFORMS_VECTORIZE_SANDBOXVECTORIZER_SANDBOXVECTORIZER_H
#define LLVM_TRANSFORMS_VECTORIZE_SANDBOXVECTORIZER_SANDBOXVECTORIZER_H

#include <memory>

#include "llvm/IR/PassManager.h"
#include "llvm/SandboxIR/PassManager.h"
#include "llvm/Transforms/Vectorize/SandboxVectorizer/Passes/BottomUpVec.h"

namespace llvm {

Expand All @@ -17,10 +21,22 @@ class TargetTransformInfo;
class SandboxVectorizerPass : public PassInfoMixin<SandboxVectorizerPass> {
TargetTransformInfo *TTI = nullptr;

public:
PreservedAnalyses run(Function &F, FunctionAnalysisManager &AM);
// Used to build a RegionPass pipeline to be run on Regions created by the
// bottom-up vectorization pass.
sandboxir::PassRegistry PR;

// The main vectorizer pass.
std::unique_ptr<sandboxir::BottomUpVec> BottomUpVecPass;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this a member variable? It's created and used only within runOnFunction() at the moment, so making it a class member implies that it used across multiple member functions of SandboxVectorizerPass. Are you planning to move it to a separate class that SandboxVectorizerPass inherits from?

Copy link
Contributor

Choose a reason for hiding this comment

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

if we move away from cl::opt("sbvec-passes") and to proper pass parameters, we would want the LLVM pass to hold the region pass manager in the class, rather than keep it local to runImpl, because we'd specify the region pass pipeline via the constructor

but with cl::opt I don't think it really matters

Copy link
Contributor

Choose a reason for hiding this comment

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

actually, I thought that BottomUpVecPass would own the region pass manager so it could invoke it when it creates a region

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

making it a class member implies that it used across multiple member functions of SandboxVectorizerPass

Not the only reason possible to make something a member. Making it local to runImpl means constructing the BottomUpVec pass every time, which seems like a waste to me.

Also, conceptually I see the SandboxVectorizer pass as a wrapper around BottomUpVec that handles the creation of sandbox IR. I don't see anything wrong with the wrapper owning the thing it's wrapping.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not the only reason possible to make something a member. Making it local to runImpl means constructing the BottomUpVec pass every time, which seems like a waste to me.

Ah yes, I forgot that we were building the pass pipeline over and over again.

Copy link
Contributor

Choose a reason for hiding this comment

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

actually, I thought that BottomUpVecPass would own the region pass manager so it could invoke it when it creates a region

I missed BottomUpVec also having a reference to the region pass manager


// The PM containing the pipeline of region passes. It's owned by the pass
// registry.
sandboxir::RegionPassManager *RPM;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same


bool runImpl(Function &F);

public:
SandboxVectorizerPass();
PreservedAnalyses run(Function &F, FunctionAnalysisManager &AM);
};

} // namespace llvm
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ bool BottomUpVec::runOnFunction(Function &F) {
// TODO: Replace with proper SeedCollector function.
auto Seeds = collectSeeds(BB);
// TODO: Slice Seeds into smaller chunks.
// TODO: If vectorization succeeds, run the RegionPassManager on the
// resulting region.
if (Seeds.size() >= 2)
tryVectorize(Seeds);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "llvm/SandboxIR/PassManager.h"
#include "llvm/Support/CommandLine.h"
#include "llvm/Transforms/Vectorize/SandboxVectorizer/Passes/BottomUpVec.h"
#include "llvm/Transforms/Vectorize/SandboxVectorizer/Passes/NullPass.h"

using namespace llvm;

Expand All @@ -30,6 +31,57 @@ cl::opt<std::string> UserDefinedPassPipeline(
cl::desc("Comma-separated list of vectorizer passes. If not set "
"we run the predefined pipeline."));

static void registerAllRegionPasses(sandboxir::PassRegistry &PR) {
PR.registerPass(std::make_unique<sandboxir::NullPass>());
}

static sandboxir::RegionPassManager &
parseAndCreatePassPipeline(sandboxir::PassRegistry &PR, StringRef Pipeline) {
static constexpr const char EndToken = '\0';
// Add EndToken to the end to ease parsing.
std::string PipelineStr = std::string(Pipeline) + EndToken;
int FlagBeginIdx = 0;
auto &RPM = static_cast<sandboxir::RegionPassManager &>(
PR.registerPass(std::make_unique<sandboxir::RegionPassManager>("rpm")));

for (auto [Idx, C] : enumerate(PipelineStr)) {
// Keep moving Idx until we find the end of the pass name.
bool FoundDelim = C == EndToken || C == PR.PassDelimToken;
if (!FoundDelim)
continue;
unsigned Sz = Idx - FlagBeginIdx;
std::string PassName(&PipelineStr[FlagBeginIdx], Sz);
FlagBeginIdx = Idx + 1;

// Get the pass that corresponds to PassName and add it to the pass manager.
auto *Pass = PR.getPassByName(PassName);
if (Pass == nullptr) {
errs() << "Pass '" << PassName << "' not registered!\n";
exit(1);
}
// TODO: Add a type check here. The downcast is correct as long as
// registerAllRegionPasses only registers regions passes.
RPM.addPass(static_cast<sandboxir::RegionPass *>(Pass));
}
return RPM;
}

SandboxVectorizerPass::SandboxVectorizerPass() {
registerAllRegionPasses(PR);

// Create a pipeline to be run on each Region created by BottomUpVec.
if (UserDefinedPassPipeline == DefaultPipelineMagicStr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same for this stuff. Not sure why they should be in the pass constructor.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why recreate the pipeline for each function we ever run the vectorizer on, if it's defined by a command-line flag and won't change during a compiler invocation?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah totally forgot that we were rebuilding it every time.

// Create the default pass pipeline.
RPM = &static_cast<sandboxir::RegionPassManager &>(PR.registerPass(
std::make_unique<sandboxir::FunctionPassManager>("rpm")));
// TODO: Add passes to the default pipeline.
} else {
// Create the user-defined pipeline.
RPM = &parseAndCreatePassPipeline(PR, UserDefinedPassPipeline);
}
BottomUpVecPass = std::make_unique<sandboxir::BottomUpVec>(RPM);
}

PreservedAnalyses SandboxVectorizerPass::run(Function &F,
FunctionAnalysisManager &AM) {
TTI = &AM.getResult<TargetIRAnalysis>(F);
Expand All @@ -56,31 +108,13 @@ bool SandboxVectorizerPass::runImpl(Function &LLVMF) {
return false;
}

sandboxir::Context Ctx(LLVMF.getContext());
// Create SandboxIR for `LLVMF`.
sandboxir::Function &F = *Ctx.createFunction(&LLVMF);
// Create the passes and register them with the PassRegistry.
sandboxir::PassRegistry PR;
auto &BottomUpVecPass = static_cast<sandboxir::FunctionPass &>(
PR.registerPass(std::make_unique<sandboxir::BottomUpVec>()));

sandboxir::FunctionPassManager *PM = nullptr;
if (UserDefinedPassPipeline == DefaultPipelineMagicStr) {
// Create the default pass pipeline.
PM = &static_cast<sandboxir::FunctionPassManager &>(PR.registerPass(
std::make_unique<sandboxir::FunctionPassManager>("pm")));
PM->addPass(&BottomUpVecPass);
} else {
// Create the user-defined pipeline.
PM = &PR.parseAndCreatePassPipeline(UserDefinedPassPipeline);
}

if (PrintPassPipeline) {
Copy link
Contributor

Choose a reason for hiding this comment

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

oh, is this the only reason SandboxVectorizerPass has a reference to the region pass manager? I think ideally only the BoUpVectorizer has a reference to it, and we can call BottomUpVecPass->printPipeline(), which forwards to RPM->printPipeline()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I'm fine with BottomUpVec owning the region pass manager instead. I considered it and took the lazy way out because it made it easier to print the pipeline here :)

I'll change it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done. I'm not sure I'm completely happy with the pattern where the RegionPassManager object is actually owned by the PassRegistry, but I don't think it will cause any problems either and if we go with it we can always change it afterwards.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, all the non-owning pointers make me sus

PM->printPipeline(outs());
RPM->printPipeline(outs());
return false;
}

// Run the pass pipeline.
bool Change = PM->runOnFunction(F);
return Change;
// Create SandboxIR for LLVMF and run BottomUpVec on it.
sandboxir::Context Ctx(LLVMF.getContext());
sandboxir::Function &F = *Ctx.createFunction(&LLVMF);
return BottomUpVecPass->runOnFunction(F);
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,7 @@

; This checks the default pass pipeline for the sandbox vectorizer.
define void @pipeline() {
; CHECK: pm
; CHECK: bottom-up-vec
; CHECK: rpm
; CHECK-EMPTY:
ret void
}
8 changes: 4 additions & 4 deletions llvm/test/Transforms/SandboxVectorizer/user_pass_pipeline.ll
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
; RUN: opt -passes=sandbox-vectorizer -sbvec-print-pass-pipeline -sbvec-passes=bottom-up-vec,bottom-up-vec %s -disable-output | FileCheck %s
; RUN: opt -passes=sandbox-vectorizer -sbvec-print-pass-pipeline -sbvec-passes=null,null %s -disable-output | FileCheck %s

; !!!WARNING!!! This won't get updated by update_test_checks.py !

; This checks the user defined pass pipeline.
define void @pipeline() {
; CHECK: pm
; CHECK: bottom-up-vec
; CHECK: bottom-up-vec
; CHECK: rpm
; CHECK: null
; CHECK: null
; CHECK-EMPTY:
ret void
}
Loading