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

Specify compile-time error to specify explicit priorities in const entries #1259

Conversation

jafingerhut
Copy link
Collaborator

No description provided.

@jafingerhut
Copy link
Collaborator Author

I would argue that this change could be considered a clarification in the spec, not a change, but in truth a strict reading of the earlier version of the spec could easily be interpreted to imply that it was allowed to specify explicit priority values within const entries.

The open source p4c implementation gives a compile-time error if you attempt to specify priorities explicitly within const entries, so this change makes the spec more closely match that implementation.

@jafingerhut
Copy link
Collaborator Author

@jnfoster @jonathan-dilorenzo If there is room on the schedule for the next P4 LDWG meeting, I would like to talk about this PR.

@thantry Please review this to see if it meets with your approval, and/or forward to others you would like to review it. The intent is to simply say explicitly in the language spec that when you declare const entries, you are not allowed to specify entry priorities in the source code. In that case, the relative priority values will always be the relative order that the entries appear in the source code, with earlier entries having higher match priority than later entries. The runtime API is NOT allowed to change any of these entries in any way, so it should be unimportant whether there are "gaps" between priority values or not for later insertion of new entries, because no new entries can be added to such a table.

The main benefit of having this change in the spec is that it simplifies the implementation a little bit, not to have to deal with const entries and explicit user-provided entry priorities.

@jafingerhut
Copy link
Collaborator Author

Additional details, if you are curious: This simplification also helps simplify the P4Runtime API specification additions required in order to support initial entries [1], i.e entries without const before it, again because it avoids introducing new scenarios for the long-existing const entries that have been around since 2016.

[1] p4lang/p4runtime#432

@jonathan-dilorenzo
Copy link
Collaborator

One consideration where we might want to allow explicit priorities for const entries is when combined with for loops. This would let you write e.g. one loop for odd priority entries with common structure and another for even priority entries with different common structure.

@thantry, do you know if there's some use-case like this?

Otherwise, specifying the relevant compile-time error seems like an excellent choice!

@thantry
Copy link

thantry commented Aug 8, 2023 via email

@jafingerhut
Copy link
Collaborator Author

@jonathan-dilorenzo I am fine either way on the question of whether const entries should allow the explicit specification of priority values, but at least Hari is not aware of a near-term future need for this, and I am not, either.

It is something that could be generalized in the future in the specification and p4c implementation, of course, but at least p4c right now does not allow it.

@jonathan-dilorenzo
Copy link
Collaborator

Sounds good! Thanks for the due diligence Andy. Let's stick with this for now and then we can revisit if we have a use-case (or if the operational semantics being worked on become much weirder or something).

@jnfoster
Copy link
Collaborator

Can we have a Changelog for this one?

@jafingerhut
Copy link
Collaborator Author

As discussed at the 2023-Aug-28 language design work group meeting, I will close this PR, allowing the possibility that a future version of p4c will support specifying priority values explicitly inside of a list of entries in const entries (which it gives an error for today). I will file an issue on p4c indicating that it does not support this, in case someone comes across this limitation and wonders why.

@jafingerhut
Copy link
Collaborator Author

Closing for reasons mentioned in previous comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants