-
Notifications
You must be signed in to change notification settings - Fork 453
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
@command_line annotation #5123
base: main
Are you sure you want to change the base?
@command_line annotation #5123
Conversation
ChrisDodd
commented
Feb 13, 2025
- per-target selectable support, though most should want it
Would be great to show the utility with one of the existing parametrized tests. https://github.com/p4lang/p4c/blob/main/backends/p4test/CMakeLists.txt#L118 I also think it makes sense to add target and arch as arguments to the annotation to make sure that only the right compiler picks up the option. This might make this parser safer. |
bfb4f08
to
dddf817
Compare
I added a
I would think it would be common to want arguments for multiple targets when you have a program that will actually compile on multiple targets. On can use |
0deb9d7
to
28b0e73
Compare
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 doubt that AMD's backend would want this (I think @asl mentioned reasons that it might not be a good idea (although I don't personally have any opinion about whether it's a good or bad idea)), but as long as it's easy for a backend to toggle this annotation to off (as you've done in this PR), then I'm not opposed to these changes.
This reminds me of issue #4907. When I created it, I was thinking of having a way to specify compilation options in the source file some other way, for example via the LLVM Lit
(https://llvm.org/docs/CommandGuide/lit.html) utility.
@sepanchi Sergey, have you had a chance to give any thought to this?
Maybe adding this annotation will avoid the need to use something like Lit
.
state start { | ||
transition s1; | ||
} |
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.
Why does start
get moved here in this test?
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.
This is something the ParsersUroll
pass does . I checked these outputs against the deleted ones in parser-unroll (which was the outputs for the tests with the explicit --loopsUnroll
argument in ctest) and they match.
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.
Are the midend outputs changing now because:
- Previously, these outputs were the result of running the corresponding tests without
--loopsUnroll
. The outputs in p4_16_samples_outputs/parser-unroll/ were the result of running the same set of tests with--loopsUnroll
. - Now, we are only running each of these tests one time - only with
--loopsUnroll
. So the set of outputs corresponding to no--loopsUnroll
were deleted and replaced by outputs of the same set of tests corresponding to having--loopsUnroll
.
Is that correct?
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, exactly.
28b0e73
to
93a1677
Compare
- per-target selectable support, though most should want it Signed-off-by: Chris Dodd <cdodd@nvidia.com>
- remove duplicate runs now unneeded Signed-off-by: Chris Dodd <cdodd@nvidia.com>