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

Relax runtime and compile-time callsite restrictions #1356

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jaehyun1ee
Copy link
Contributor

Following the discussion in #1349, this proposes some relaxation to Appendix F: Restrictions on compile time and run time calls.

First, for runtime calls,

(Current)

image

(Proposed)

image

(1) It adds a new column top level and refines the column parser or control top level (it is written parser or top level in the image, but it is likely a typo introduced while migrating to asciidoc) to top level within a parser or control.

(2) The explainer for the table has been modified such that the row extern means: both an extern method and a function, while it previously meant only the former.

(3) It allows function calls at both top level and top level within a parser or control.

This reflects the use case demonstrated in p4c tests: issue1538.p4, issue2957.p4, issue2488-bmv2.p4, issue2543-1.p4.

For instance,

bit<16> incr(in bit<16> x) {
    return x + 1;
}
parser ParserImpl(packet_in packet,
                  out headers hdr,
                  inout metadata meta,
                  inout standard_metadata_t standard_metadata) {
    bit<16> tmp_port = incr((bit<16>) standard_metadata.ingress_port);
    ...
}

(4) It allows extern calls at a function. This reflects the use case in issue3274.p4, issue3274-2.p4, issue3292.p4, and issue4500.p4.

extern bit<32> f(in bit<32> x, out bit<16> y);
bit<32> x() {
    return f(x = 1, y = _);
}

Second, for compile-time constructor calls,

(Current)

image

(Proposed)

image

The column for package has been modified to allow instantiation of package, parser, control, and extern. Here, "this type can be instantiated in a package" means, that, this type can be instantiated as a constructor parameter for a package.

Note that a package instantiation can only happen at the top-level, while it involves nameless instantiations of package, parser, control, and extern objects as constructor arguments. With the current restriction, there is no clear way of classifying such nameless instantiation according to the table. But with the revised restriction, we may consider them as valid.

Signed-off-by: Jaehyun Lee <99jaehyunlee@gmail.com>
Signed-off-by: Jaehyun Lee <99jaehyunlee@gmail.com>
Signed-off-by: Jaehyun Lee <99jaehyunlee@gmail.com>
@jaehyun1ee jaehyun1ee added the ldwg-discussion Plan to discuss at next LDWG label Dec 8, 2024
@rcgoodfellow
Copy link
Collaborator

LDWG notes:

General consensus is to accept this PR. However, there should be an addition to specify that some extern functions may not be accepted by some backends at the top level.


| .>| parser state .>| control apply block .>| parser or top level .>| action .>| extern .>| function
| .>| top level .>| parser state .>| control apply block .>| top level within a parser or control .>| action .>| extern .>| function
Copy link
Collaborator

Choose a reason for hiding this comment

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

@rcgoodfellow Regarding your question during the meeting about the meaning of "top level" and "top level within a parser or control", if the text above the table said the following, would that make it clear for you?

"Top level" refers to the part of a P4 program that one is in at the beginning of the file, where all declarations have global scope, i.e. not within any control, parser, or function definition.

"Top level within or control" means either within a parser definition, but outside of any of its parser state definitions, or within a control definition, but outside of its apply { ... } block.

Copy link
Collaborator

Choose a reason for hiding this comment

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

SGTM, thanks!


| extern | yes | yes | yes | yes | no | no
| extern | yes | yes | yes | yes | yes | no | yes
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggest adding the following text before this table to elaborate on the "yes" in the "top level" column for the row "extern":

While the language syntactically allows extern function or method calls at the top level of a program, an implementation may restrict which extern functions or methods it allows in these places. For example, an implementation might only support extern functions or method calls with the annotation @pure. Any such calls are executed before the first packet is processed by the device.

(I am less sure about whether the last sentence is worth including. It could open up a can of worms that I have not yet thought of.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a related question on top-level function/method calls.

I believe the only place where top-level function/method calls can happen is as constant initializers, e.g.,

const bit<32> c = f(...);

because the syntax only allows:

declaration
    : constantDeclaration
    | externDeclaration
    | actionDeclaration
    | parserDeclaration
    | typeDeclaration
    | controlDeclaration
    | instantiation
    | errorDeclaration
    | matchKindDeclaration
    | functionDeclaration
    ;

Then, the function/methods must produce local compile-time known values, since constant initializers should be local compile-time known.

... but is it? According to section 18.1, no sorts of function/method call is categorized as local compile-time known.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch.

It seems possible that we could define a subset of function/method calls as local compile-time known values, but this seems like it might be a fairly nit-picky thing to do well, e.g. only pure functions whose parameters are all local compile-time known. It seems very odd to me to call an expression with a function call, where the function call includes side effects on extern state, a local compile-time known expression, but perhaps it could be reasonable. I just don't know if we want to go that far into the rabbit hole on this one.

Perhaps we just put "no" for function/method calls in the "top level" column of the table for now, and generalize it later if someone really wants to?

Are any function/method calls supported at the top level by p4c today?

Copy link
Contributor Author

@jaehyun1ee jaehyun1ee Dec 10, 2024

Choose a reason for hiding this comment

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

I agree it will quickly become a rabbit hole. Consulting to my frontend implementation, it seems like static_assert is the only case where any kind of call (except constructor calls) is made in the top level.

struct Headers {
}

struct Meta {
}

const bool test = static_assert(V1MODEL_VERSION >= 20160101, "V1 model version is not >= 20160101");

Copy link
Collaborator

Choose a reason for hiding this comment

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

So it seems like we might want to define that static_assert returns a compile-time known value (I don't see a reason why we need to prepend "local" to that). The spec already says that the parameters to static_assert must be compile-time known values.

Perhaps in the "top level" column, we could say "no" for extern function/method calls, but with a footnote explaining the exception that static_assert calls are allowed at the top level?

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 agree on saying "no" for function/method calls from the top-level, but with a footnote excusing static_assert.

Just to clarify, this PR also makes the following change,

in a select expression. The row for `extern` describes where extern method
- calls can be made from.
+ or function calls can be made from.

so extern functions belong to row "extern", and not "function".

And about "local" compile-time known, I suppose it should be "local" because section 11.1 Constants says:

The initializer expression must be a local compile-time known value.

, which also suggests that the parameters to static_assert should be local compile-time known rather than just compile-time known.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we have an "extern" row and a "function" row, then I believe that (perhaps confusingly, and worth saying explicitly somewhere), that "extern" means "extern function or extern method", and "function" means "function, but NOT and extern function".

It seems reasonable to me to also say "no" for column "top level", row "function", too, for the same reasons that it could get pretty nit-picky very quickly to define which subset of functions are supported. For example, functions can call extern functions or methods in their body.

I guess that at the top level, everything that is compile-time known is also local compile-time known, because there can be no constructor parameters in scope at the top level. I don't have a strong preference for whether "lcoal" is there or not.

Copy link
Contributor Author

@jaehyun1ee jaehyun1ee Dec 10, 2024

Choose a reason for hiding this comment

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

I also think that being compile-time known in the top level would imply being a local compile-time known value. But this being the case, I think making it explicitly written in the spec (e.g., "static_assert returns a local-compile time known value") would be more accurate, and also in line with section 18.1 that makes a clear definition of the two concepts.

I guess this is not directly related to this PR, as the revisions to Appendix F need not discuss local compile-time known or compile-time known - ness. Maybe there should be a separate PR, revising the section for static_assert :)

…op level

Signed-off-by: Jaehyun Lee <99jaehyunlee@gmail.com>
@jaehyun1ee
Copy link
Contributor Author

I've made edits, reflecting the discussions made above.

  • Adds explainer on function row, and top level within a parser or control column.

  • Disallow function calls from the top-level, with a footnote excusing static_assert calls.

  • Regarding the extern row, there was an explainer already:

    Note that while the extern row shows that extern methods can be called from many places, particular externs may have additional restrictions not listed in this table. Any such restrictions should be documented in the description for each extern, as part of the documentation for the architecture that defines the extern.

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.

3 participants