@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
kfcripps
left a comment
There was a problem hiding this comment.
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.
28b0e73 to
93a1677
Compare
asl
left a comment
There was a problem hiding this comment.
I'm still not being a big fan of mixing two orthogonal things – command line options of a particular implementation and overall semantics of enable/disable several things.
But given that it is an opt-in feature in the annotation parser, I guess it should be ok.
93a1677 to
b9f0156
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>