[experimental] Allow actions to instantiate externs #3938
Conversation
Signed-off-by: Mihai Budiu <mbudiu@vmware.com>
| @noWarn("unused") @name(".NoAction") action NoAction_1() { | ||
| } | ||
| @name("C.RW") action RW() { | ||
| @name("C.local") Local<bit<32>>() local_0; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
two externs declarations in separate actions will be distinct objects, even if they have the same local name.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Attaching zip file this time.
p4c-pr-3938-maybe-bug.zip
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The right solution is to also use the action name as part of the generated name.
There was a problem hiding this comment.
But don't we allow more than one action with the same name, too?
There was a problem hiding this comment.
I am not sure we do. Even if we do, they can use annotations to specify different names.
|
|
||
| control C() { | ||
| action RW() { | ||
| Local<bit<32>>() local; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
fruffy
added
the
p4-spec
This shows an example that would allow the effect of Direct registers in P4.