-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[SandboxVectorizer] Use sbvec-passes flag to create a pipeline of Region passes after BottomUpVec. #111223
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.
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
[SandboxVectorizer] Use sbvec-passes flag to create a pipeline of Region passes after BottomUpVec. #111223
Changes from 5 commits
1c1145a
674c91e
ee3e3d3
4b4c063
1772712
fe5f7d8
196b167
314bf8f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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 |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| //===- PassRegistry.def - Registry of passes --------------------*- 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 | ||
| // | ||
| //===----------------------------------------------------------------------===// | ||
| // | ||
| // This file is used as the registry of sub-passes that are part of the | ||
| // SandboxVectorizer pass. | ||
| // | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| // NOTE: NO INCLUDE GUARD DESIRED! | ||
|
|
||
| #ifndef REGION_PASS | ||
| #define REGION_PASS(NAME, CREATE_PASS) | ||
| #endif | ||
|
|
||
| REGION_PASS("null", NullPass()) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we also need some cleanup code at the end like: to avoid getting a warning of redefining REGION_PASS if we ever decide to include the .def file more than once
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also why use
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just followed the pattern for the existing |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that it's the pass managers that own the passes it looks a bit out of place that populating them is done with a static function, because the only users of it are the pass managers.
Wdyt about moving this to the PassManager base class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do this, then sandboxir::PassManager has to know about vectorizer-specific passes. Is this a desired outcome?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant making
parseAndCreatePassPipelinea member function ofPassManagersuch that we call:instead of:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. But
parseAndCreatePassPipelineneeds to know about the existing vectorizer passes in order to instantiate them.Do you want llvm/lib/SandboxIR/PassManager.cpp to include all the passes from llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/Passes/* ? Should we move
RegionPassManagerback into the vectorizer?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, conceptually parsing the pass pipeline string and building passes should not be a static function in BottomUpVec.cpp because it is a pass-manager task, not a vectorization-specific task.
Oh yes, the creator function would need to have access to the passes. Hmm could we perhaps keep the creator function here in a lambda
createPass(Name)and pass the lambda as an argument to the parse function?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Left
createPassas a static function in BottomUpVec.h instead of a lambda because the includes and defines needed for the .def file messed up indentation and I preferred to keep that ugliness contained in its own function.I also added a new unit test, similar to the now deleted PassRegistry test, that checks the pipeline parsing function (we also have some very basic coverage from lit tests)