Skip to content

Conversation

@ChrisDodd
Copy link
Contributor

No description provided.

@ChrisDodd ChrisDodd requested a review from jafingerhut October 12, 2024 01:22
@ChrisDodd
Copy link
Contributor Author

I'm unclear whether these changes need to be also in the asciidoc version, or if that happens automatically. Or perhaps we should wait until after the switchover to asciidoc and do it only there.

@jafingerhut
Copy link
Collaborator

I think it is worth discussing at the next LDWG meeting, exactly when Madoko -> AsciiDoc conversion happens.

Fortunately, for text paragraphs with little or no markup, it usually makes no difference. That said, the changes are so extensive for Madoko->AsciiDoc that I expect if that change is made before this PR, some small changes may be required in this PR. I think small changes to a PR like this to update it will not be wasted effort.

Copy link
Member

@vlstill vlstill left a comment

Choose a reason for hiding this comment

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

Looks good to me, except we should add wording that prohibits negative bit indices.

@jafingerhut
Copy link
Collaborator

jafingerhut commented Nov 14, 2024

@ChrisDodd Sorry for the inconvenience, but we have, after several months of planning, transitioned the P4 language specification source from Madoko to AsciiDoc. Thus most or all of your PRs will need to be updated so that they apply to that version. Hopefully this should be fairly straightforward, e.g. by looking at examples of how things like section headings, bullet items, etc. are marked up in the latest version of the file P4-16-spec.adoc

If you have questions on how to change things for AsciiDoc, feel free to ask. Note that creating a new PR with similar changes as this PR might be easier than updating this PR.

@jafingerhut jafingerhut added the ldwg-discussion Plan to discuss at next LDWG label Jan 23, 2025
Copy link
Collaborator

@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.

LGTM

@vlstill vlstill self-requested a review March 10, 2025 20:22
@jafingerhut
Copy link
Collaborator

@vlstill Please take another look to see if you have any further comments or questions, and if not, please approve.

@jafingerhut
Copy link
Collaborator

@vlstill Please take another look to see if you have any further comments or questions, and if not, please approve.

The intent is that any compile-time known bit, int, or int type value should be allowed for value L, and any run-time variable value with type bit<W>, since we can guarantee it is non-negative at compile time, but disallow runtime variable values with type int<W>.

Comment on lines +3589 to +3644
some architectures may require it to be compile-time known. It must also be
non-negative and in-range for the type. Some architectures may allow variable indexing,
in which case an out of range value will be equivalent to a shift of that amount.
Copy link
Member

Choose a reason for hiding this comment

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

I suggest wording this as for shifts, allowing only int or bit<W>, not int<W> (https://p4.org/wp-content/uploads/2024/10/P4-16-spec-v1.2.5.html#sec-a-note-about-shifts). This also matches what was discussed on the last LDWG. We cannot defer to the shift for the int<W> index as shift does not allow int<W> to avoid probelems with negative values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe add a precise definition for "non-negative" somewhere -- either a bit<W> value or a compile time known value that is >= 0. I don't think we need to (or should) specifically disallow int<W> compile-time-known values.

Copy link
Member

@vlstill vlstill Mar 26, 2025

Choose a reason for hiding this comment

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

I think the formulation that is used for RHS of shifts permits that:

[...] the right operand of shifts must be either an expression of type bit<S> or a compile-time known value that is a non-negative integer.

I suggest we use the same.

I think a restriction to operation types is something important enough to have it written in the operation description and not hidden in a definition of non-negativity.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ChrisDodd Any thoughts on updates to this PR to address this comment?

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 added a line here to clarify that the index must be a bit<W> type if it is not compile-time-known, in order to be compatible with shift.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. @vlstill Any additional comments, or does it look good to you?

Signed-off-by: Chris Dodd <[email protected]>
@jonathan-dilorenzo
Copy link
Collaborator

@vlstill, please take another look or let us know if this is ready to merge so I can merge it. Otherwise, @jonathan-dilorenzo and @vlstill can discuss this in the September LDWG.

Copy link
Collaborator

@jonathan-dilorenzo jonathan-dilorenzo left a comment

Choose a reason for hiding this comment

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

LGTM. Approving and merging. @vlstill, if you have remaining comments, let's fix them in a follow-up CL.

@jonathan-dilorenzo jonathan-dilorenzo merged commit 8485e46 into p4lang:main Oct 3, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ldwg-discussion Plan to discuss at next LDWG

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants