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

Allow implicit cast for directionless args #1341

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jaehyun1ee
Copy link
Contributor

I would like to propose allowing implicit cast for directionless method / function / constructor arguments, following the discussion made in #1312.

Regarding it, the current spec section 8.20 mentions that:

The calling convention is copy-in/copy-out (Section 6.8). For generic functions the type arguments can be explicitly specified in the function call. The compiler only inserts implicit casts for direction in arguments to methods or functions as described in Section 8.11. The types for all other arguments must match the parameter types exactly.

So as per the spec, we cannot apply implicit cast for directionless method / function / constructor arguments.

As a side note, directionless action arguments can be implicitly cast as per the spec since it mentions the following in section 6.8.1.

Actions can also be explicitly invoked using function call syntax, either from a control block or from another action. In this case, values for all action parameters must be supplied explicitly, including values for the directionless parameters. The directionless parameters in this case behave like in parameters.

So, like the case for action, I am proposing to allow implicit cast for directionless method / function / constructor arguments.

I believe it better reflects the use cases in the p4c test suite. Below is a drop-down list of tests that expect such a feature.

List of tests
  • extern-funcs-bmv2.p4
  • extern2.p4
  • gauntlet_extern_arguments_2.p4
  • gauntlet_hdr_in_value-bmv2.p4
  • issue1001-1-bmv2.p4
  • issue1001-bmv2.p4
  • issue1043-bmv2.p4
  • issue1334.p4
  • issue1642-bmv2.p4
  • issue1653-bmv2.p4
  • issue1653-complex-bmv2.p4
  • issue1660-bmv2.p4
  • issue1765-1-bmv2.p4
  • issue2648.p4
  • issue3246-1.p4
  • issue383-bmv2.p4
  • issue562-bmv2.p4
  • issue933-1.p4
  • named-arg1.p4
  • v1model-special-ops-bmv2.p4
  • bfd_offload.p4
  • calc-ebpf.p4
  • constructor_cast.p4
  • issue1006.p4
  • issue1097-2-bmv2.p4
  • issue1097-bmv2.p4
  • issue1814-1-bmv2.p4
  • issue1814-bmv2.p4
  • issue1958.p4
  • issue2844-enum.p4
  • issue298-bmv2.p4
  • issue4288.p4
  • issue754.p4
  • list7.p4
  • pr1363.p4
  • psa-action-profile1.p4
  • psa-action-profile3.p4
  • psa-action-profile4.p4
  • psa-basic-counter-bmv2.p4
  • psa-counter1.p4
  • psa-counter2.p4
  • psa-counter3.p4
  • psa-custom-type-counter-index.p4
  • psa-end-of-ingress-test-bmv2.p4
  • psa-example-dpdk-byte-alignment_1.p4
  • psa-example-dpdk-byte-alignment_2.p4
  • psa-example-dpdk-byte-alignment_3.p4
  • psa-example-dpdk-byte-alignment_5.p4
  • psa-example-dpdk-byte-alignment_6.p4
  • psa-example-dpdk-byte-alignment_7.p4
  • psa-example-dpdk-byte-alignment_8.p4
  • psa-example-dpdk-byte-alignment_9.p4
  • pse-example-dpdk-counter.p4
  • psa-example-dpdk-externs.p4
  • psa-example-dpdk-meter-execute-err.p4
  • psa-example-dpdk-meter.p4
  • psa-example-dpdk-meter1.p4
  • psa-example-dpdk-varbit-bmv2.p4
  • psa-meter1.p4
  • psa-meter3.p4
  • psa-meter7-bmv2.p4
  • psa-random.p4
  • psa-register-complex-bmv2.p4
  • psa-register-read-write-2-bmv2.p4
  • psa-register-read-write-bmv2.p4
  • psa-register1.p4
  • psa-register2.p4
  • psa-register3.p4
  • rcp.p4
  • rcp1.p4
  • register-serenum-bmv2.p4
  • simple-firewall_ubpf.p4
  • unused-counter-bmv2.p4
  • value-sets.p4

@jaehyun1ee jaehyun1ee added the ldwg-discussion Plan to discuss at next LDWG label Dec 4, 2024
@rcgoodfellow
Copy link
Collaborator

From LDWG: This is clarifying and making something more explicit that was implicit in the spec before. We have tests currently that have this assumption baked in, and p4c implements the behavior described in the change.

Comment on lines +4883 to 4884
inserts implicit casts for direction `in` or directionless arguments to methods or
functions as described in <<sec-casts>>. The types for all
Copy link
Contributor

Choose a reason for hiding this comment

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

One more thing, the sentence says "to methods or functions", but there are two more cases that could be relevant:

  • directionless arguments of actions with argument explicitly supplied in P4 (I think you have mentioned that for this case the casts are already allowed somewhere);
  • arguments of extern and control/parser instances are are directionless arguments, and implicit casts could be applied here too (i.e. Register<bit<32>, bit<3>>(8, 42) -- the intializer type is presumably bit<32>, but one expects int argument will work).

So essentially I would suggest this should be allowed in any cases where a directionless argument can happen (except for directionless argument value coming from control plane, but there I can't see a way how one could want to cast it).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the comment :) For arguments to control/parser instances, I understand them as arguments to the "apply" method of the control/parser instances. So I think it roughly corresponds to the explanation "... or directionless arguments to method or functions as described ...". For example, the spec mentions:

To invoke the services of another control, it must be first instantiated; the services of an instance are invoked by calling it using its apply method. (14.4)

It would be nice to enumerate all different kinds of invocations somewhere in the spec, perhaps in the section describing the calling convention. Yet, I don't have a good idea regarding, exactly where in that section, and with what explanation should we insert.

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