Skip to content
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

warn on unused controls/parsers #4440

Merged
merged 3 commits into from
Mar 3, 2024
Merged

warn on unused controls/parsers #4440

merged 3 commits into from
Mar 3, 2024

Conversation

grg
Copy link
Contributor

@grg grg commented Feb 20, 2024

Add warnings for unused P4Control and P4Parser objects to match P4Table warnings. Addresses issue #4439.

@@ -1,4 +1,7 @@
issue3379-1.p4(14): [--Wwarn=unused] warning: t4: unused instance
mypackaget<bit>(MyParser1()) t4;
^^
issue3379-1.p4(3): [--Wwarn=unused] warning: Parser MyParser1 is not used; removing
parser MyParser1(in bit tt) {
^^^^^^^^^
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@grg Do you know why Myparser1 is considered unused in this test program?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that is because there is no main declaration.

Copy link
Contributor

@jafingerhut jafingerhut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left one question about a warning message for one of the test programs, but if there is a good explanation for that one, the rest all looks good to me.

@grg
Copy link
Contributor Author

grg commented Feb 21, 2024

@jafingerhut / @fruffy: Any thoughts on whether these warnings should be added? I opened a ticket (#4439) to get feedback on this. My opinion is that the extra warnings should be added for consistency.

@fruffy
Copy link
Collaborator

fruffy commented Feb 21, 2024

@jafingerhut / @fruffy: Any thoughts on whether these warnings should be added? I opened a ticket (#4439) to get feedback on this. My opinion is that the extra warnings should be added for consistency.

I do not have any strong opinions but I think your change makes sense.

But what about multi-pipe programs? Do the warnings work correctly here? I guess they should since the parser/control is also removed.

@grg
Copy link
Contributor Author

grg commented Feb 21, 2024

@jafingerhut / @fruffy: Any thoughts on whether these warnings should be added? I opened a ticket (#4439) to get feedback on this. My opinion is that the extra warnings should be added for consistency.

I do not have any strong opinions but I think your change makes sense.

But what about multi-pipe programs? Do the warnings work correctly here? I guess they should since the parser/control is also removed.

It works as expected with multi-pipe programs: a warning is emitted only if a control/parser is never used in any of the pipes. If it's used in at least one pipe then no warning is emitted.

@grg grg marked this pull request as ready for review February 21, 2024 17:28
grg added 3 commits February 21, 2024 11:28
Add warnings for unused P4Control and P4Parser objects to match P4Table
warnings. Addresses issue #4439.
Call externalName() to get the control/parser block name without
"control"/"parser" prepended to the front.
@grg grg force-pushed the grgibb/issue_4439 branch from bfad107 to 9c0fcd1 Compare February 21, 2024 17:28
@grg
Copy link
Contributor Author

grg commented Feb 23, 2024

@asl / @ChrisDodd Is this proposed change likely to impact either of you negatively?

@asl
Copy link
Contributor

asl commented Feb 23, 2024

@asl / @ChrisDodd Is this proposed change likely to impact either of you negatively?

I believe we are ok. Cross-checking with @kfcripps

@fruffy fruffy added the core Topics concerning the core segments of the compiler (frontend, midend, parser) label Feb 23, 2024
@jafingerhut
Copy link
Contributor

I see 3 approvals on this. Are we waiting for explicit approval from @ChrisDodd ?

@grg grg added this pull request to the merge queue Mar 3, 2024
@grg
Copy link
Contributor Author

grg commented Mar 3, 2024

I see 3 approvals on this. Are we waiting for explicit approval from @ChrisDodd ?

I've added it to the merge queue. This was put on the back burner last week while I was working on upstreaming the midend def-use pass from Tofino.

Merged via the queue into main with commit f2be187 Mar 3, 2024
16 checks passed
@grg grg deleted the grgibb/issue_4439 branch March 3, 2024 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Topics concerning the core segments of the compiler (frontend, midend, parser)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants