Inlining pass renames objects

[antoninbas]

Hi,

I have the following P4_14 program:

parser start { return ingress; }

action a(p) { modify_field(standard_metadata.egress_spec, p); }

table t { actions { a; } }

control c1 { apply(t); }

control ingress { c1(); }

After the Inline pass in the midend, the name for my action becomes c1.a. Is there a way to keep the name as a for the control plane's sake.
I don't think such an issue can arise with P4_16...

[mihaibudiu]

This happens with P4-16 programs too. The P4-16 spec talks about this http://p4.org/wp-content/uploads/2016/12/P4_16-prerelease-Dec_16.html#sec-cp-names

In general this is not possible, because a control may be instantiated multiple times, and thus we have to make up names for the actions.
The solution proposed in the spec is to allow the control plane to use for names any unambiguous suffix of an actual name. So if there is only one action "a" in your program, then you can name it "a" no matter where it resides.

[antoninbas]

The problem is more if the action is used in multiple controls.
In the control plane, if I want to build some action input, I am only interested in the action definition, and in my original P4 program there is a single definition for the action, with an unambiguous name of my choosing, "a". Is there really no way for me to access that original definition name?
@sethfowler maybe this is a dumb suggestion but what if the P4Runtime serializer ran before the inlining pass (https://github.com/p4lang/p4c/blob/master/backends/bmv2/bmv2.cpp#L79)? Would that be possible or would that introduce other issues?

[mihaibudiu]

The original name is always a suffix.
The API generator could try to use just the suffix when handling a P4-14 program.

[antoninbas]

@mbudiu-vmw If the action is defined inside a control to start with, then we do want the control name in the full action name.

[mihaibudiu]

We could also have a rule that for global actions the inliner does not change the @name annotations.

[sethfowler]

Yeah, I think maintaining @name for global actions is the right approach. At this point I think it's safest to ensure that p4RuntimeSerializer always sees the same version of the code that the BMV2 JSON is generated from.

[sethfowler]

NoAction is a global action, right? I'm surprised that it doesn't also end up with a prefixed name.

[antoninbas]

Maybe that's because I added an explicit annotation in core.p4: https://github.com/p4lang/p4c/blob/master/p4include/core.p4#L54?
Yes it would be great if the name annotation could be preserved for global actions.

[mihaibudiu]

One way to do this is to save the @name for global objects as prefixed with a dot, and not change it afterwards if it starts with a dot.

[sethfowler]

@mbudiu-vmw, great minds think alike. =) I've been wanting to make that change for a while.

@ChrisDodd, it seemed like you felt positive about the idea earlier today. Should we just go ahead and implement this? I'm happy to take care of it next week once I get some of the high priority stuff on my plate knocked out.

[mihaibudiu]

I could implement it too, but I see a big pile of bug reports growing, so I am not sure when I will get to this one. The inliner change should be easy. I don't remember whether there is any pass that attaches name annotations to global objects; this may need to be done very early in the pipeline, possibly even before converting from V1.

[sethfowler]

Sounds good. If you get a chance first and are able to take care of it, I'd be very appreciative. =)

[mihaibudiu]

I will submit a PR which makes this change: P4-14 actions have a name annotation prefixed with a dot. The inliner does not change the name annotation if it starts with a dot.

This change may need support from the PI generator.

Also, it may need to handle other objects besides actions, and it may need to handle P4-16 actions. If this change solves @antoninbas's problem, then we'll proceed with the additional ones.

[antoninbas]

If you open a PR, I can run it on several inputs (P4_14 & P4_16) and make sure it works for me.

[mihaibudiu]

As soon as I finish testing.
But @sethfowler may need to adjust the API generator too.

[mihaibudiu]

It also looks like we'll have to strip the dot in the JSON generated.

[sethfowler]

Removing the initial dot from the generated API is straightforward.

@mbudiu-vmw, does your patch change @name's behavior in all cases, or does it only apply to actions right now?

[mihaibudiu]

For now we have only done actions.

[mihaibudiu]

I think that this issue is solved by using names prefixed with a dot for global actions.
It is unclear whether we need to do anything for other global objects.