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.

Sign up for GitHub

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

Jump to bottom

[#4883] Remove unused actions whose @name name starts with "__" #4900

Merged
merged 2 commits into from
Sep 17, 2024
Merged

[#4883] Remove unused actions whose @name name starts with "__" #4900

merged 2 commits into from
Sep 17, 2024

Conversation

kfcripps
Copy link
Contributor

@kfcripps kfcripps commented Sep 4, 2024

Fixes #4883

@kfcripps kfcripps added the core Topics concerning the core segments of the compiler (frontend, midend, parser) label Sep 4, 2024
fruffy
fruffy reviewed Sep 11, 2024
View reviewed changes
frontends/p4/unusedDeclarations.cpp
@@ -112,7 +112,7 @@ const IR::Node *RemoveUnusedDeclarations::process(const IR::IDeclaration *decl)
LOG3("Visiting " << decl);
if (decl->getName().name == IR::ParserState::verify && getParent<IR::P4Program>())
return decl->getNode();
if (decl->getName().name.startsWith("__"))
if (decl->externalName(decl->getName().name).startsWith("__"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

What happens when you have something like @name("a2") action _xyz() now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fruffy Please see the added test outputs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tagging @oleg-ran-amd for any additional comments

@kfcripps
Add additional test case
971db01 
Signed-off-by: Kyle Cripps <[email protected]>
fruffy
fruffy approved these changes Sep 11, 2024
View reviewed changes
testdata/p4_16_samples_outputs/issue4883_dup_has_returned_decl2.p4-stderr
@@ -0,0 +1,6 @@
issue4883_dup_has_returned_decl2.p4(34): [--Wwarn=uninitialized_use] warning: s.f may be uninitialized
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: This seems like noise which can be fixed by initializing s, making it part of C2's parameters. Feel free to ignore this if required for the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not remember if it is required for the test. @oleg-ran-amd If you think it's not required, feel free to push such changes to this branch.

fruffy
fruffy reviewed Sep 11, 2024
View reviewed changes
Copy link
Collaborator

@fruffy fruffy left a comment

Choose a reason for hiding this comment

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

Approving this, but maybe clearing up the renaming behavior of inlining is the better long-term solution. Personally, I do not see why inlining should use the control-plane name.

@kfcripps
Copy link
Contributor Author

Approving this, but maybe clearing up the renaming behavior of inlining is the better long-term solution. Personally, I do not see why inlining should use the control-plane name.

@oleg-ran-amd Do you want to try this approach instead? If so, I will not merge this yet and allow you to push such changes to this branch (or open a new PR if you prefer that).

@kfcripps
Copy link
Contributor Author

Approving this, but maybe clearing up the renaming behavior of inlining is the better long-term solution. Personally, I do not see why inlining should use the control-plane name.

@oleg-ran-amd Do you want to try this approach instead? If so, I will not merge this yet and allow you to push such changes to this branch (or open a new PR if you prefer that).

Or I can just merge this now and let you make the better long-term changes later. Let me know what you prefer.

@kfcripps
Copy link
Contributor Author

@oleg-ran-amd From @asl:

for 4900 – I’d rather implement proper approach with renaming rather than doing band-aid later

@kfcripps
Copy link
Contributor Author

Per DM with @oleg-ran-amd, I will merge this now and leave #4883 open for Oleg to look into the suggested better long-term solution later.

@kfcripps kfcripps added this pull request to the merge queue Sep 17, 2024
Merged via the queue into p4lang:main with commit b0890a8 Sep 17, 2024
18 checks passed
@kfcripps kfcripps deleted the 4883 branch September 17, 2024 19:31
@kfcripps kfcripps mentioned this pull request Sep 17, 2024
Open
linuxrocks123 pushed a commit to linuxrocks123/p4c that referenced this pull request Sep 18, 2024
@kfcripps @linuxrocks123
[p4lang#4883] Remove unused actions whose @name name starts with "_…
0bd4404 
…_" (p4lang#4900)

* Remove unused actions whose external name starts with '__'

Signed-off-by: Kyle Cripps <[email protected]>

* Add additional test case

Signed-off-by: Kyle Cripps <[email protected]>

---------

Signed-off-by: Kyle Cripps <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fruffy fruffy fruffy approved these changes

@asl asl Awaiting requested review from asl

@vlstill vlstill Awaiting requested review from vlstill

Assignees
No one assigned
Labels
core Topics concerning the core segments of the compiler (frontend, midend, parser)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unexpected "Duplicates declaration" error when P4 program contains no duplicate declarations
2 participants
@kfcripps @fruffy