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.

Sign up for GitHub

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

[experimental] Allow actions to instantiate externs #3938

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

[experimental] Allow actions to instantiate externs #3938

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions frontends/p4/validateParsedProgram.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ void ValidateParsedProgram::postorder(const IR::Declaration_Instance *decl) {
::error(ErrorType::ERR_INVALID, "%1%: invalid instance name.", decl);
if (findContext<IR::BlockStatement>() && // we're looking for the apply block
findContext<IR::P4Control>() && // of a control
!findContext<IR::P4Action>() && // experimental: allow externs in actions
!findContext<IR::Declaration_Instance>()) { // but not in an instance initializer
::error(ErrorType::ERR_INVALID,
"%1%: invalid declaration. Instantiations cannot be in a control 'apply' block.",
Expand All @@ -161,10 +162,12 @@ void ValidateParsedProgram::postorder(const IR::Declaration_Instance *decl) {
::error(ErrorType::ERR_INVALID,
"%1%: invalid declaration. Instantiations cannot be in a function or method.",
decl);
#if 0
auto inAction = findContext<IR::P4Action>();
if (inAction != nullptr)
::error(ErrorType::ERR_INVALID, "%1%: declaration. Instantiations not allowed in actions.",
decl);
#endif
}

/// Constant names cannot be underscore
Expand Down
26 changes: 26 additions & 0 deletions testdata/p4_16_samples/action-register.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
#include <core.p4>

extern Local<D> {
Local();
D read();
void write(in D data);
}

control C() {
action RW() {
Local<bit<32>>() local;
Copy link
Contributor

Choose a reason for hiding this comment

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

How many instantiations does this create? I assume from the following comment on the merge request:

This shows an example that would allow the effect of Direct registers in P4.

... that there is a separate instance per P4 table entry that invokes this action, and also per each apply block call-site that directly invokes this action, rather than there being e.g. one instance or one per P4 table or anything like that. And further, there's no kind of connection between any of the instances.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We probably would restrict such actions to being called from tables only. At least that is the use case that is requested.
Your interpretation is the intended one otherwise.

local.write(local.read() + 1);
}
table t {
actions = { RW; }
}
apply {
t.apply();
}
}

control CTRL();
package top(CTRL _c);

top(C()) main;

28 changes: 28 additions & 0 deletions testdata/p4_16_samples_outputs/action-register-first.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
#include <core.p4>

extern Local<D> {
Local();
D read();
void write(in D data);
}

control C() {
action RW() {
Local<bit<32>>() local;
local.write(local.read() + 32w1);
}
table t {
actions = {
RW();
@defaultonly NoAction();
}
default_action = NoAction();
}
apply {
t.apply();
}
}

control CTRL();
package top(CTRL _c);
top(C()) main;
36 changes: 36 additions & 0 deletions testdata/p4_16_samples_outputs/action-register-frontend.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
#include <core.p4>

extern Local<D> {
Local();
D read();
void write(in D data);
}

control C() {
@name("C.tmp") bit<32> tmp;
@name("C.tmp_0") bit<32> tmp_0;
@name("C.tmp_1") bit<32> tmp_1;
@noWarn("unused") @name(".NoAction") action NoAction_1() {
}
@name("C.RW") action RW() {
@name("C.local") Local<bit<32>>() local_0;
tmp = local_0.read();
tmp_0 = tmp + 32w1;
tmp_1 = tmp_0;
local_0.write(tmp_1);
}
@name("C.t") table t_0 {
actions = {
RW();
@defaultonly NoAction_1();
}
default_action = NoAction_1();
}
apply {
t_0.apply();
}
}

control CTRL();
package top(CTRL _c);
top(C()) main;
32 changes: 32 additions & 0 deletions testdata/p4_16_samples_outputs/action-register-midend.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
#include <core.p4>

extern Local<D> {
Local();
D read();
void write(in D data);
}

control C() {
@name("C.tmp") bit<32> tmp;
@noWarn("unused") @name(".NoAction") action NoAction_1() {
}
@name("C.RW") action RW() {
@name("C.local") Local<bit<32>>() local_0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this change permit multiple actions in the same control C that each declare a local extern instance with the same name local? Local declarations inside of action bodies definitely seems to imply that this should be possible.

Note that adding the action name to the auto-generated @name annotation IS NOT ENOUGH to make that possible right now, because we also allow multiple actions with the same name to be defined in the same control.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

two externs declarations in separate actions will be distinct objects, even if they have the same local name.

Copy link
Contributor

Choose a reason for hiding this comment

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

The @name annotation "C.local" is "enclosing_control_name.local_instance_name_inside_action", yes?

I guess I will try out this PR's changes myself and find out, but what if there was another action that also had an instance with the same name local inside of control C? Would their @name annotations be identical? That should be considered a bug, shouldn't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

See the attached test program action-register2.p4, which is just a minor variation of your action-register.p4. Look at the output program from the last FrontEnd pass. It has two DirectRegister instances with @name("C.local").
It seems like a bug to me that p4c is intentionally generating identical @name annotations for two different instances.

If you do not consider that a bug, why not?

What can a control plane API generation pass do with this that is possibly useful?

Copy link
Contributor

Choose a reason for hiding this comment

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

Attaching zip file this time.
p4c-pr-3938-maybe-bug.zip

Copy link
Contributor

Choose a reason for hiding this comment

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

Note: I am not saying that I think if this is considered a bug, that it needs to be fixed right away. It would be best to worry about it only if we do move forward with enhancing the language with the ability to instantiate extern objects within action bodies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The right solution is to also use the action name as part of the generated name.

Copy link
Contributor

Choose a reason for hiding this comment

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

But don't we allow more than one action with the same name, too?

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 am not sure we do. Even if we do, they can use annotations to specify different names.

tmp = local_0.read();
local_0.write(tmp + 32w1);
}
@name("C.t") table t_0 {
actions = {
RW();
@defaultonly NoAction_1();
}
default_action = NoAction_1();
}
apply {
t_0.apply();
}
}

control CTRL();
package top(CTRL _c);
top(C()) main;
26 changes: 26 additions & 0 deletions testdata/p4_16_samples_outputs/action-register.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
#include <core.p4>

extern Local<D> {
Local();
D read();
void write(in D data);
}

control C() {
action RW() {
Local<bit<32>>() local;
local.write(local.read() + 1);
}
table t {
actions = {
RW;
}
}
apply {
t.apply();
}
}

control CTRL();
package top(CTRL _c);
top(C()) main;
Empty file added testdata/p4_16_samples_outputs/action-register.p4-stderr
View file
Open in desktop
Empty file.