[SandboxIR] Add RegionPass/RegionPassManager #110933
Conversation
These classes mirror the existing FunctionPass/FunctionPassManager, and will be used by the sandbox vectorizer to run a pipeline of region passes.
slackito
requested review from
alinas,
vporpo,
tschuett,
aeubanks,
tmsri and
Sterling-Augustine
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| TestNamePass(llvm::StringRef Name) : RegionPass(Name) {} | ||
| bool runOnRegion(Region &F) { return false; } | ||
| }; | ||
| EXPECT_DEATH(TestNamePass("white space"), ".*whitespace.*"); |
There was a problem hiding this comment.
Do we have a comment at the constructor about these name restrictions?
There was a problem hiding this comment.
The base Pass class does, kinda (it mentions it can't contain spaces, but it doesn't mention it can't begin with a -). And the comment is not for the constructor but for the Name member. So, technically the answer to your question is "no, not really, and neither does FunctionPass".
There was a problem hiding this comment.
Maybe we should have some kind of comment that describes the allowed names or points to the code that does the checks.
There was a problem hiding this comment.
Would something like this suffice?
There was a problem hiding this comment.
The Name field in the Pass class already has a comment with the reasoning.
/// The pass name. This is also used as a command-line flag and should not
/// contain whitespaces.
There was a problem hiding this comment.
ah ok
unrelated to this patch, I'm not sure why we don't allow "-" as the first char. it's not conventional to start a pass name with "-", but there are plenty of other worse things you could do, like start with "("
There was a problem hiding this comment.
Yes, using an illegal name shouldn't be too much of an issue. Feel free to drop the comments you added.
There was a problem hiding this comment.
I'll merge as-is if you don't mind. The comments are a slight improvement IMO (nothing wrong with documenting the public interface of the class) and I don't want to push another revision and start pre-merge checks again.
These classes mirror the existing FunctionPass/FunctionPassManager, and will be used by the sandbox vectorizer to run a pipeline of region passes.