-
Notifications
You must be signed in to change notification settings - Fork 445
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.
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
adds table entries support #397
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The biggest bug is using ActionListElement in an Entry; ActionListElement is only meant for the list of action names that show up in the "actions" list. You should use an Expression. Then some of the special cases you wrote will go away.
backends/bmv2/jsonconverter.cpp
Outdated
*/ | ||
void JsonConverter::convertTableEntries(const IR::P4Table *table, | ||
Util::JsonObject *jsonTable) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that Chris likes the brace on the previous line. This applies everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the cpplint commit addresses all the styling issues.
backends/bmv2/jsonconverter.cpp
Outdated
auto matchType = getKeyMatchType(tableKey); | ||
key->emplace("match_type", matchType); | ||
if (matchType == "valid") { | ||
if(k->is<IR::BoolLiteral>()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need a space after if. This happens many times; I won't flag all of them.
backends/bmv2/jsonconverter.cpp
Outdated
key->emplace("key", stringRepr(k->to<IR::Constant>()->value, ROUNDUP(keyWidth, 8))); | ||
unsigned long mask = (1ULL << keyWidth)-1; | ||
key->emplace("mask", stringRepr(mask, ROUNDUP(keyWidth, 8))); | ||
} else if (k->is<IR::DefaultExpression>()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this one to work correctly you will need to have a pass that converts default expressions to lists, similar to SimplifySelectList.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why. These are the keyset elements that get set as default, not the whole list as default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look at the following example ../testdata/p4_16_samples/select_struct.p4 and check the output from the compiler.
backends/bmv2/jsonconverter.cpp
Outdated
key->emplace("mask", stringRepr(k->to<IR::Mask>()->right->to<IR::Constant>()->value, ROUNDUP(keyWidth, 8))); | ||
} else if (k->is<IR::Constant>()) { | ||
key->emplace("key", stringRepr(k->to<IR::Constant>()->value, ROUNDUP(keyWidth, 8))); | ||
unsigned long mask = (1ULL << keyWidth)-1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should use mpz; keyWidth can be bigger than 64.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
even though width_bits() is defined as:
// This is well-defined only for types with fixed width
virtual int width_bits() const { return 0; }
in base.def:21?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider the case of an ipv6 key field.
backends/bmv2/jsonconverter.cpp
Outdated
if (entriesList == nullptr) return; | ||
|
||
auto entries = mkArrayField(jsonTable, "entries"); | ||
int entryPriority = 1; // priority is defined by index position |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is misleading; it should say "default priority"; you are overwriting it below from an annotation.
/// Pre-defined entry in a table | ||
class Entry : IAnnotated { | ||
Annotations annotations; /// annotations are optional (supported annotations: @priority(value)) | ||
ListExpression keys; /// must be a tuple expression |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default should be a legal key; I suggest having this be an Expression, exactly like a SelectCase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see the above comment on how I'm handling default.
ir/ir.def
Outdated
class Entry : IAnnotated { | ||
Annotations annotations; /// annotations are optional (supported annotations: @priority(value)) | ||
ListExpression keys; /// must be a tuple expression | ||
ActionListElement action; /// action must be defined in action list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also be an expression; in general it will be a MethodCallExpression.
Don't use ActionListElement, there control-plane arguments are not bound, so the type-checking is different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will this solve my other type checker issues?
midend/removeParameters.cpp
Outdated
@@ -76,6 +76,12 @@ void FindActionParameters::postorder(const IR::MethodCallExpression* expression) | |||
} | |||
} | |||
|
|||
const IR::Node* DoRemoveActionParameters::preorder(IR::EntriesList *list) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You MUST remove action parameters - this takes care of the data-plane bound parameters.
This function should be removed; it may work correctly if you make the change above.
entries = { | ||
0x01 : a_with_control_params(1); | ||
0x02 : a_with_control_params(2); | ||
#ifdef ENABLE_NEGATIVE_TESTS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should make two tests out of this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I'll work on handling the negative testing separately.
@@ -0,0 +1,81 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should also write some tests where you use "default" to match a tuple, and also where actions have data-plane arguments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are default entry tests in almost all the test cases. Again, the way I expect defaults for tuples is (_, _)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not symmetric with the select, and it is also less consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you set the version number to 2.7 in the generated JSON?
The typechecker will probably need an extension similar to SelectExpression and matchCase to typecheck the entries. |
45b033c
to
d8782ca
Compare
@mbudiu-vmw I replaced the ActionListElement with a MethodCallExpression and was able to successfully remove two of the hacks in typeChecker (the ugly ones), as well as having the default behavior for removeParameters. There is only the remaining issue with the typeChecker thinking I mutated the program.
|
eecc82f
to
8f13347
Compare
@mbudiu-vmw: I revised the code according to your guidance. Still need to add the negative tests (I have them) but need to figure out a way of handling multiple types of errors in a single test. I will open a separate issue for that so that it doesn't slip. There may be a few other things that can be optimized, however, I would like to merge this PR so that folks developing the tutorial materials for the developer day (in particular @robertsoule and @vgurevich) can use this feature. |
For negative tests it's easiest to write 1 file/test. |
backends/bmv2/jsonconverter.cpp
Outdated
auto match_type = getKeyMatchType(ke); | ||
if (match_type == v1model.selectorMatchType.name) | ||
continue; | ||
// Decreasing order of precedence: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These rules should really be part of the v1model spec - which does not exist.
They were reverse-engineered from the p4-14 compiler.
Maybe this information should be in this comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated comment. It is actually what BMv2 expects (which might be the same as v1model spec).
frontends/p4/p4-parse.ypp
Outdated
@@ -776,6 +783,13 @@ tableProperty | |||
auto id = IR::ID(@1, "actions"); | |||
$$ = new IR::Property( | |||
@1 + @5, id, IR::Annotations::empty, v, false); } | |||
/* \todo: force CONST entries? conflicts with next rule */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be fixed too.
Would it work to have two rules:
optAnnotations ENTRIES ...
optAnnotations const ENTRIES ...
Maybe @ChrisDodd can help suggest a way to remove this ambiguity. It is important to have the const
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'll ask Chris for help
frontends/p4/p4-parse.ypp
Outdated
@@ -813,6 +827,34 @@ actionRef | |||
$$ = new IR::ActionListElement(@2, $1, mce); } | |||
; | |||
|
|||
entry | |||
: optAnnotations keysetExpression ':' actionInvocation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest replacing this with
: optAnnotations keysetExpression : initializer ';'
The code becomes much simpler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, initializer is defined just an expression. I really think this should point to an invocation expression, i.e., we should at least have an action reference with (). As opposed to default_action which can also take actions without (). AFAIK, at least in P4_14, we distinguish between the two types (one is const and one is not). I think this is resolved in P4_16 by using the const keyword for default_action, but we still do not require the () for an action without parameters. I would prefer to have the () enforced in the grammar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Actions cannot be generic, so you don't need the actionInvocation with
<>
. - bison gives bad error messages; we can give a much better error message in the type-checker. Even if the grammar accepts without (), we can enforce this later.
- whatever this syntax is for actionInvocation, it should accept the same kinds of expressions as the default_action does. They are both accepting actions with all parameters bound. There is no reason that one of them accepts without () and the other one doesn't.
- By using initializer we can remove two productions from the grammar. This is not the right metric to optimize, but it's still useful to have a small grammar.
Please note that this is not really an invocation, so the name of the production is unsuitable. It is just binding the arguments; the invocation happens at runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mbudiu-vmw: there is no definition for default_action, it falls on the generic rule
| optAnnotations optCONST IDENTIFIER '=' initializer ';'
and initializer is in fact an expression.
In fact reviewing the grammar to look for an action invocation, the only pertinent rule I found is assignmentOrMethodCallStatement
, and I borrowed the rules for actionInvocation from there. I think that a more clean implementation should split that rule into the assignment and actionInvocation and in that case there are no additional rules, and the intent is much more obvious.
frontends/p4/toP4/toP4.cpp
Outdated
@@ -1176,6 +1176,36 @@ bool ToP4::preorder(const IR::TableProperties* t) { | |||
return false; | |||
} | |||
|
|||
bool ToP4::preorder(const IR::EntriesList *l) { | |||
dump(1); | |||
// if (l->isConstant) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the grammar can be fixed this should be added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed.
LOG2("Setting key type to " << dbp(keyTuple)); | ||
setType(key, keyTuple); | ||
// installing also for the original because we cannot tell which one will survive in the ir | ||
// \TODO: figure out the rules of survivorship |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment applies to almost all the methods in all visitors. So it's not very useful here.
This is a problem with the current visitor design.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point. The larger issue is to understand both why there are additional copies and their lifetime. @cole-barefoot, @jnfoster, and I looked over the visitor code on Fri and we have a number of questions that we'll discuss with @ChrisDodd on Mon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem stems from the fact that the visitor always passes you a clone of the node, both in preorder and postorder. But you have no idea whether this copy differs from the original because you are changing it or because someone else changed one of its children.
One can design a visitor were there are never any automatic clones. I proposed to add one such visitor implementation, where all mutations must be made only by the user. This would be easier to debug, but would require users writing more code in some circumstances. Today all transforms use at least O(n) space because of this feature, even if they change no nodes!
Alternatively, we could endow the visitor with an upcall when a node is substituted; this could enable us to incrementally update some data structures such as typemaps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually O(log n) space (bounded by the depth of the tree, not the size of the tree). I'm not sure how to design a visitor pattern that does no automatic clones, since you need to clone any node that has any children changed (recursively through grandchildren) even if you don't otherwise want to change the node. When I tried it, you end up needing to manually define preorder and postorder functions for almost every possible IR type, which seems impractical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that if you really want to know whether this node has changed in a postorder function, and only insert into the typemap in that case, you can use:
if (*key != *getOriginal())
setType(key, keyTuple);
This doesn't work in a preorder function as it only tells you if the ndoe has changed yet, and not if it might be changed in the future from traversing its children or in the postorder function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should change the setType helper function to work this way.
Or add another helper.
@@ -1647,6 +1760,7 @@ const IR::Node* TypeInference::typeSet(const IR::Operation_Binary* expression) { | |||
// set after unification | |||
auto r = expression->right->clone(); | |||
auto e = expression->clone(); | |||
if (isCompileTimeConstant(expression->right)) setCompileTimeConstant(r); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's easier to debug if you have only one statement per line.
if (!isv1 && da == nullptr) | ||
::warning("Table %1% does not have an `%2%' property", | ||
t->name, IR::TableProperties::defaultActionPropertyName); | ||
|
||
auto entriesList = t->getEntries(); | ||
if (entriesList != nullptr) { // list of entries is optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this check is completely subsumed by the type checker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. Actually this is a check that I forgot to remove and did not yet add to the type checker.
auto keyset = e->getKeys(); | ||
if (keyset == nullptr || !keyset->is<IR::ListExpression>()) | ||
::error("%1%: key expression must be tuple", keyset); | ||
else if (keyset->to<IR::ListExpression>()->components->size() < |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also wrong if you allow DefaultExpression.
@@ -0,0 +1,81 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not symmetric with the select, and it is also less consistent.
action a() { standard_meta.egress_spec = 0; } | ||
action a_with_control_params(bit<9> x) { standard_meta.egress_spec = x; } | ||
|
||
table t_exact { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to use a standard indentation for P4 programs too.
ae3d515
to
d513bfb
Compare
frontends/p4/p4-parse.ypp
Outdated
actionInvocation | ||
: lvalue '(' argumentList ')' ';' { $$ = new IR::MethodCallExpression(@1 + @4, $1, | ||
new IR::Vector<IR::Type>(), $3); } | ||
| lvalue '<' typeArgumentList '>' '(' argumentList ')' ';' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This rule should be removed; actions cannot have type arguments.
frontends/p4/p4-parse.ypp
Outdated
} | ||
; | ||
|
||
actionInvocation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actionBinding or actionConstruction is a more suitable name. The action is not being invoked.
} | ||
|
||
// the key must have been typechecked already. | ||
if (!typeMap->contains(key)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is too early: in preorder the type of the key has not been computed yet.
You need a negative test for this case too - the entries coming before the key.
|
||
table t_ternary { | ||
|
||
#if ENABLE_NEGATIVE_TESTS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this ever used?
If not it should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is defined in line 20 and used here.
0x02 : a_with_control_params(2); | ||
#ifdef ENABLE_NEGATIVE_TESTS | ||
// test default entries | ||
_ : a_with_control_params(3); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this a negative test? This is legal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the default for an exact key? In BMv2, I support default only for ternary, lpm, and range.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a tricky one; I am not sure it should be a negative test.
I can write a compiler pass which would cause this program to work. You could claim that it reflects a shortcoming in the implementation.
A negative test should be an illegal program, which will never be accepted.
This program is somewhere in a gray area.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For any program (and backend for that matter) the keyset has to resolve to an actual value that can be installed as a match. We can do that for some other types of matches but not for exact matches. It is in a gray area because you're thinking about these keyset expressions as select expressions. I argue that they are not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is an example:
bit x;
table t {
key = { x: exact; }
...
const entries = { default: a(); }
}
This can be easily translated to:
const entries = { 1w0: a(); 1w0: a(); }
These two programs are semantically equivalent.
Also, note that nothing in the spec says that the compiler cannot change the key type if necessary; perhaps the compiler can convert a table with exact
keys into a table with ternary
keys. We should not rule out these possibilities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not agree that these programs are semantically equivalent unless you explicitly say that 0 is a default value in an exact match table (which I don't recall being in the spec). A key value of 0 for either exact or ternary is different from a 0 mask for a ternary key (which is what the default for a ternary key is -- matching any). So even if you have a semantically preserving transformation from exact to ternary keys, these two programs are not equivalent.
@mbudiu-vmw you've given me a whole lot of good feedback on this PR and I am grateful for your careful reviews. But on this one, we should just agree to disagree and move on. Please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, the example should have been 1w0: a(); 1w1: a()
.
Anyway, this is not very important - it is just a test case.
_ : a_with_control_params(23); | ||
#ifdef ENABLE_NEGATIVE_TESTS | ||
// negative tests: | ||
(0x18, 0xF) : a_with_control_params(24); // not a range |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want to declare this being a negative test.
A mask may be convertible to a set of ranges.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't think we should support such implicit conversions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not up to the front-end to decide what conversions are supported.
The back-end should reject such programs, not the type-checker.
@@ -0,0 +1,86 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you name all these files ending with -bmv2, then the p4c-bm2-ss compiler will be used, and the output json will be validated by bmv2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should they be checked by bmv2 or just p4test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p4test runs all of them.
p4c-bm2-ss runs all the ones that end with bmv2.p4. And the output json is validated by the simulator. So you want to use it, because otherwise the json generation code is not exercised.
../testdata/p4_16_errors/table-entries-decl-order.p4(53): error: Table initializer entries must occur after key definitions (table t_ternary) | ||
table t_ternary { | ||
^^^^^^^^^ | ||
../testdata/p4_16_errors/table-entries-decl-order.p4(66): error: Could not find type of Key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want the same error message 3 times?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, i don't want repeated messages, but these are different entries, and each of them gets checked. Are you suggesting to prune the rest of the children if an error is encountered? I thought we were leaning toward reporting as many errors as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They may be different entries, but they all generate the same error message.
We want to show as many informative errors as possible, but the second one does not bring anything new.
@@ -0,0 +1,4 @@ | |||
../testdata/p4_16_errors/table-entries-exact-ternary.p4(74):syntax error, unexpected ')', expecting ',' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This actually looks like a legitimate key expression. Why is this an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs a pair to match the key type of the table
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be a syntax error, perhaps a type error.
(0x02, 0x1181 ) : a_with_control_params(2); | ||
(0x03, 0x1111 &&& 0xF000) : a_with_control_params(3); | ||
// test default entries | ||
(0x04, _ ) : a_with_control_params(4); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also please add a case
default : a_with_control_params(6);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the default for an exact key in bmv2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The actual implementation for default is up to the back-end.
But the front-end should typecheck this program.
You do allocate O(n), but you may reclaim all of it if there are no changes. |
94ea36e
to
d26dfb7
Compare
frontends/p4/p4-parse.ypp
Outdated
new IR::Vector<IR::Type>(), $3); } | ||
| lvalue '<' typeArgumentList '>' '(' argumentList ')' ';' | ||
| lvalue '<' typeArgumentList '>' '(' argumentList ')' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is useless, since actions cannot have type arguments.
frontends/p4/p4-parse.ypp
Outdated
@@ -828,26 +828,26 @@ actionRef | |||
; | |||
|
|||
entry | |||
: optAnnotations keysetExpression ':' actionInvocation | |||
: keysetExpression ':' actionBinding optAnnotations ';' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is somewhat unfortunate, but I don't have a counter-proposal.
void ValidateParsedProgram::postorder(const IR::EntriesList* l) { | ||
auto table = findContext<IR::P4Table>(); | ||
if (table == nullptr) | ||
::error("%1%: table initialziers must belong to a table", l); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo in error message
Adds two IR nodes (IR::Entry and IR::EntriesList) as table properties Adds the serialization of predefined entries to JSON (requires JSON v2.7 or later) and a series of tests for table entries. As part of the serialization, the match key type detection was refactored to be common for generating table match keys and determining the type of keysets in entries. The typechecker needed convincing to accept action 'calls' in the entry definition. Action call parameters were being removed by the removeParameters pass, which should not happen -- action data needs to be serialized into the bmv2 json. When disabling for IR::EntriesList, the type inference complains ... not sure why. So there are a couple of instances in the type inference visitors where we check whether or not we are in an EntriesList context and elide the type parameter checking. There should be a cleaner way. CONST ENTRIES grammar rule conflicts with CONST IDENTIFIER rule. For now the only way this works is removing the CONST. We need to bring it back.
* proper type checking for table initializers that removes the hacky implementation of shortcuts Refactored the type checking for table initializers in checking the entire list for matching table structures, and then entries for checking legality of keys, key types, constants, and actions. Uses (and )typeSet to set expressions to compile time constants even when creating new IR::Constant expressions for left and right operands, as well as set the set itself to be a compile time constant if its members are both constants. * proper methods for annotation handling and updates min JSON version * use MethodCallExpression instead of ActionListElement for actions in table entry initializer * use mpz_class for mask to represent masks larger than 64 bits * adds sample outputs for p4test
* Moves EntriesList checking to pre-order * Moves checks from validateParsedProgram to type checker * Fixes returns on errors
…ry rule Changes the grammar to rename actionInvocation to actionBiding and move annotations for entries at the end of the statement, such that we avoid a shift/reduce conflict in the grammar. Also tried splitting assignmentOrMethodCallStatement into assignmentStatement and methodCallStatement that is just transforming an actionBinding (MethodCallExpression) into a Statement, but that creates more issues in the grammar. Fiddled with the tests to move annotations at the end ... so happened that I had to pick the test with annotations to test all error conditions :(
d26dfb7
to
63442c0
Compare
Adds two IR nodes (IR::Entry and IR::EntriesList) as table properties
Adds the serialization of predefined entries to JSON (requires JSON
v2.7 or later) and a series of tests for table entries.
As part of the serialization, the match key type detection was
refactored to be common for generating table match keys and
determining the type of keysets in entries.
The typechecker needed convincing to accept action 'calls' in the
entry definition.
Action call parameters were being removed by the removeParameters
pass, which should not happen -- action data needs to be serialized
into the bmv2 json.
When disabling for IR::EntriesList, the type inference complains
... not sure why. So there are a couple of instances in the type
inference visitors where we check whether or not we are in an
EntriesList context and elide the type parameter checking. There
should be a cleaner way.
CONST ENTRIES grammar rule conflicts with CONST IDENTIFIER rule. For
now the only way this works is removing the CONST. We need to bring it
back.