Open kfcripps opened 5 months ago
Similarly, the following P4 program:
header h_t {
bit<8> f;
}
control C() {
action bar() {
}
table t {
actions = { bar; }
default_action = bar;
}
table t2 {
actions = { bar; }
default_action = bar;
}
apply {
t.apply();
t2.apply();
}
}
control proto();
package top(proto p);
top(C()) main;
gets transformed into:
header h_t {
bit<8> f;
}
control C() {
@name("C.bar") action bar() {
}
@name("C.bar") action bar_1() {
}
@name("C.t") table t_0 {
actions = {
bar();
}
default_action = bar();
}
@name("C.t2") table t2_0 {
actions = {
bar_1();
}
default_action = bar_1();
}
apply {
t_0.apply();
t2_0.apply();
}
}
control proto();
package top(proto p);
top(C()) main;
so we end up with two actions defined in the same scope that have the same local name (C.bar
). Is this a bug per the spec?
@jphurl @oleg-ran-amd @rsmith-pensando
@kfcripps This behavior has existed in p4c for quite some time. I will send links to some other p4c issues that I am aware of from years back with some discussion on the topic, but if I am recalling correctly, the reason that p4c internally takes one action and makes multiple copies of it, one for each table it is used within, has its motivation in allowing each of those copies to be optimized independently of each other, based upon the context in which the table invoking it is applied.
Here is the oldest issue I could find with the most discussion on it:
Here are some later issues, where either other people rediscovered this, or a similar thing, or in some case I probably forgot about the old one and filed another. As one of my comments in the first issue linked below says, I would love it if we could get all the stakeholders involved into a discussion with the goal of coming up with an answer to this.
There is also a PR that started with one potential change, and I believe the discussion on it led to a proposal for a different approach, one which would be a small p4c change, but I haven't thought about the details in a while to have a good evaluation of either of those proposed changes at this moment:
In particular, this comment: https://github.com/p4lang/p4c/pull/3228#issuecomment-1111635352 mentions a specific approach that might be a good one to try. I have copied the text of that comment below for convenience:
dmatousek I spoke with some folks at Intel including Han Wang about the underlying issue here, and he had a suggestion for an experiment to try: put the @name
conflict checking somewhere in the front end before the LocalizeAllActions pass, which is one that we know intentionally creates copies of actions with identical @name
annotations. If there is a @name
conflict before that pass, it should be one that the P4 developer themselves wrote.
The only disadvantage I can think of to such an approach is that now P4_16 source code produced as the output of passes after LocalizeAllActions could cause errors when used as input to p4c. I do not know how serious that is, but perhaps having a command line option that skips the new @name
conflict check would help with that.
@kfcripps This PR #4951 is intended to address this issue. Feel free to take a look at it, and review it if you are interested. It only issues error messages if you invoke a p4c command with the command line options to generate one or more P4Runtime API files. Without such options, there are no errors given for such programs, just as there are no errors for similar programs with two tables in the same control with the same @name
annotation.
@oleg-ran-amd Please take a look at #4951. This is related to issue # 4025 in our backend.
@kfcripps @oleg-ran-amd Sorry for any confusion, but I would recommend we abandon PR #4951, and instead focus on #4975, which includes all of the changes of #4951 plus one more fix.
If/when you do review PR #4975, this slide deck might help provide useful background on what is changing, what is not changing, and why: https://docs.google.com/presentation/d/1JqQciG8hWdd2UlbXbDENw36yfGGRk2LugjzRrbv6Sf4/edit?usp=sharing
That link is in the first comment on PR #4975, but it is a big comment, so wanted to call it out to you.
For example:
Per the P4-16 spec, shouldn't this result in an error message?
...
...