p4-spec
p4-spec copied to clipboard
p4lang
Introduce local compile-time known values
@apinski-cavium, actually, I had inverted the definitions -- local compile-time known is more restrictive. See my latest commit which fixes this and adopts your streamlining suggestions.
@apinski-cavium, actually, I had inverted the definitions -- local compile-time known is more restrictive. See my latest commit which fixes this and adopts your streamlining suggestions.
Yes I didn't even noticed that until now and yes this is now +1 from me.
Sure. Can you provide a pointer to those questions? I might be missing something but I don't see them in the conversation on this issue.
Looks like I forgot to submit the comments. Somehow github has preserved them for a long time...
@jnfoster, let me know when this is ready for me to do a review.
By a very quick test, it seems that the compiler does reject constructor parameters as well as generic-parameter-derived values in type declarations:
int<4> fn<T>(inout bit<4> data) {
int<(T.minSizeInBits())> v = 42;
v = v + data;
return v;
}
control C(inout bit<4> data)(int cnt) {
int<(cnt)> var;
apply {
}
}
# ./p4test test.p4
test.p4(2): [--Werror=expected] error: T.minSizeInBits(): expected a constant
int<(T.minSizeInBits())> v = 42;
^^^^^^^^^^^^^^^^^
test.p4(8): [--Werror=expected] error: cnt: expected a constant
int<(cnt)> var;
^^^
So at least this one case should be safe and we would not be restricting what the compiler already compiles.
However, minSizeInBits is currently not considered a constant also where it should be according to this spec:
typedef int<4> T;
int<4> fn(inout bit<4> data) {
int<(T.minSizeInBits())> v = 42;
v = v + data;
return v;
}
const int cnt = 4;
control C(inout bit<4> data) {
int<(cnt)> var;
apply {
}
}
$ ./p4test test.p4
test.p4(4): [--Werror=expected] error: T.minSizeInBits(): expected a constant
int<(T.minSizeInBits())> v = 42;
^^^^^^^^^^^^^^^^^
Here is a small example showing what p4c does.
control C(inout bit<8> x);
package P(C c);
const int f = 42;
int g(int x) { return x; }
control MyB(inout bit<8> x, inout bit<8> y)(int n) {
bit<(1 + 1)> a = 0;
bit<(f)> b = 1;
bit<(f / 2)> c = 2;
// These are all currently errors, and they should be
// bit<(g(3))> d = 3;
// bit<(n)> e = 3;
// bit<(x)> f = 4;
apply {
x = y;
}
}
control MyC(inout bit<8> x)(int o) {
MyB(o) b;
bit<8> y = 0;
apply {
b.apply(x, y);
}
}
P(MyC(9)) main;
Actually @jnfoster, can you say why you also wanted to remove the experimenting with bison file? Doesn't look obviously related to this...
Good catch. I've fixed that.
(Sorry for the force-pushes. I was trying to rebase and squash these to tidy it up, which is perhaps ill-advised on a public PR.)
@jnfoster @ChrisDodd If Nate agrees that some of Chris Dodd's latest comments/questions justify changes, it would be good to create a separate issue to track those.
@jnfoster @ChrisDodd If Nate agrees that some of Chris Dodd's latest comments/questions justify changes, it would be good to create a separate issue to track those.
The suggestions are good, but creating an issue for something that can be implemented in a few minutes seems like overkill. I created #1286. If people agree, I think it would be safe without discussing this at the LDWG -- all of the changes only increase clarity, but don't change meaning.
Of course, we can also discuss it, so we feel good about the process and about PRs being applied :-)
Made an executive decision to merge. Obvious and simple improvements aren't worth the discussion imo, but feel free to push back if you feel otherwise.