-
Notifications
You must be signed in to change notification settings - Fork 11.7k
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] 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.
✅ With the latest revision this PR passed the C/C++ code formatter. |
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.
LG
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a comment at the constructor about these name restrictions?
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would something like this suffice?
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.
Yeah, looks great.
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Sounds good.
These classes mirror the existing FunctionPass/FunctionPassManager, and will be used by the sandbox vectorizer to run a pipeline of region passes.