-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
[SandboxIR] PassManager #107932
[SandboxIR] PassManager #107932
Conversation
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.
All pretty standard.
OS << "("; | ||
for (auto [Idx, Pass] : enumerate(Passes)) { | ||
OS << Pass->getName(); | ||
if (Idx + 1 != Passes.size()) |
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.
can this use interleaveComma
?
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.
interleaveComma
inserts a whitespace after the comma, so I just used interleave()
// | ||
// Registers and executes the Sandbox IR passes. | ||
// | ||
// The pass manager contains an ordered sequence of passes that it runs in |
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.
it seems error prone for the pass manager to not own the pass (e.g. stack references). e.g. in the new pass manager if you want to run a pass multiple times, you just create multiple instances of it
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.
The pass registry would own the passes and would also provide the name-to-pass lookup so that you can build the pipeline from a pipeline string like "pass1,pass2".
If the pass manager owns the passes, then how is the mapping from names to passes handled?
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.
see includes of PassRegistry.def
, especially in PassBuilder.cpp
. it's basically a string comparison chain
I think your way is fine, as long as your explanation of the registry owning passes is documented
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 see, it shouldn't be too hard to follow a similar design. Let me try it out.
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 think I will stick with this design for now. Using a PassRegistry.def complicates the design quite a bit and I am not sure it is worth it at this point. We can revisit it later if needed.
// | ||
// Registers and executes the Sandbox IR passes. | ||
// | ||
// The pass manager contains an ordered sequence of passes that it runs in |
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.
see includes of PassRegistry.def
, especially in PassBuilder.cpp
. it's basically a string comparison chain
I think your way is fine, as long as your explanation of the registry owning passes is documented
This patch implements a simple pass manager for Sandbox IR.
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/30/builds/5871 Here is the relevant piece of the build log for the reference
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/146/builds/1123 Here is the relevant piece of the build log for the reference
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/136/builds/883 Here is the relevant piece of the build log for the reference
|
This patch implements a simple pass manager for Sandbox IR.
This patch implements a simple pass manager for Sandbox IR.