-
Notifications
You must be signed in to change notification settings - Fork 446
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
Add check of uniqueness of control plane names #3228
base: main
Are you sure you want to change the base?
Conversation
By any chance, is this intended to address an issue like the one described in this issue? #2755 One of the example programs on that issue has identical |
See also #3037, which didn't work. |
I tried building p4c with the proposed changes on this PR after 3 commits, and ran it locally on my system. It fails for a very simple P4 program with 2 tables, both of which have
As far as the original source code given to the compiler, there are no name conflicts at all. The error messages occur, I believe, because of the way that the p4c LocalizeAllActions frontend pass duplicates actions in the IR when the same action is an action of multiple P4 tables, and automatically creates identical See #2755 for additional notes and details. I would love to have a meeting of the minds of P4 Runtime API folks, compiler folks, etc. to see what can or should be done about this. I have ideas described on that issue, but not sure what the best way to go here is. |
Thanks for your feedback and referencing related issues. I agree that there is no clear fix and it has to be further discussed. |
@dmatousek I have scheduled a meeting with a few Intel folks knowledgeable about the P4 compiler to discuss a possible way forward on issue #2755. I will update that issue when/if there is any news to share. |
@dmatousek I spoke with some folks at Intel including @hanw about the underlying issue here, and he had a suggestion for an experiment to try: put the @name conflict checking somewhere in the front end before the LocalizeAllActions pass, which is one that we know intentionally creates copies of actions with identical @name annotations. If there is a @name conflict before that pass, it should be one that the P4 developer themselves wrote. The only disadvantage I can think of to such an approach is that now P4_16 source code produced as the output of passes after LocalizeAllActions could cause errors when used as input to p4c. I do not know how serious that is, but perhaps having a command line option that skips the new @name conflict check would help with that. |
I have created a PR that attempts to use the approach described in my earlier comment quoted above. It can be found here: #4951 |
Control plane names are not checked for uniqueness. It may lead to various issues in subsequent passes when uniqueness is assumed.