[SandboxIR] PassManager #107932
Merged
[SandboxIR] PassManager #107932
Conversation
This file contains 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
vporpo
requested review from
slackito,
alinas,
tschuett,
aeubanks,
tmsri and
Sterling-Augustine
Sterling-Augustine
approved these changes
Sep 9, 2024
aeubanks
reviewed
Sep 9, 2024
| OS << "("; | ||
| for (auto [Idx, Pass] : enumerate(Passes)) { | ||
| OS << Pass->getName(); | ||
| if (Idx + 1 != Passes.size()) |
There was a problem hiding this comment.
can this use interleaveComma?
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
aeubanks
approved these changes
Sep 10, 2024
| // | ||
| // 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.
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 |
VitaNuo
pushed a commit
to VitaNuo/llvm-project
that referenced
this pull request
Sep 12, 2024
This patch implements a simple pass manager for Sandbox IR.
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.
This patch implements a simple pass manager for Sandbox IR.