Enable sharing an action_selector between tables #354

antoninbas · 2017-03-09T02:02:17Z

In the bmv2 backend. We basically added a pass to the backend which
ensures that when several match tables share the same action_selector,
the selector input is the same across all tables.

mihaibudiu

I see no problem merging this PR; the main issue I have is that I don't know exactly what the meaning of shared action selectors is, so I am unable to judge whether this produces the correct result. I can see that the result is probably the intended one for the attached tests, but I don't know how more complicated programs should be handled.

This is really a problem with the v1model - what we are missing is a documentation that describes how v1model works. The implication is that the v1model description is really the P4-14 spec, but I am not sure that shared action selectors are described there properly either.

mihaibudiu · 2017-03-09T17:24:15Z

testdata/p4_16_samples_outputs/action_selector_shared-bmv2.p4-stderr

+    action_selector(HashAlgorithm algorithm, bit<32> size, bit<32> outputWidth);
+    ^^^^^^^^^^^^^^^
+/home/antonin/Documents/p4lang/p4c/build/p4include/v1model.p4(102)
+extern action_selector {


This warning will go away when PR #352 is merged.

mihaibudiu · 2017-03-09T17:25:42Z

backends/bmv2/jsonconverter.cpp

+    { CHECK_NULL(converter); }
+
+    const Input &get_selector_input(const IR::Declaration_Instance* selector) const {
+      return selector_input_map.at(selector);


I think that the style guide requires 4 spaces.

mihaibudiu · 2017-03-09T17:33:04Z

backends/bmv2/jsonconverter.cpp

+                auto cmp_e = [e1](const IR::Expression *e2) {
+                    // not the best solution but the best available one given that there is no "depp
+                    // comparison" of expressions for now.
+                    return e1->toString() == e2->toString();


This looks dangerous.
What is really the meaning of the action selector? Can you describe how it is supposed to work?
I would have done this transformation myself when I did the action profiles, but I didn't know exactly what the meaning of action selectors is.

The problem here is that the same expression may evaluate to a different result.
Consider this case:

bit<32> x; table t1() { key = { a[x].f: exact; } ... } table t2() { key = { a[x].f: exact; } ... } apply { x = 0; t1.apply(); x = 1; t2.apply(); }

Although both key expressions are identical, they evaluate to different results.
The converse problem may happen too, where different expressions evaluate to the same result, e.g., because they use different temporary names.

The desired behavior is for the expressions to be the same, not the result of the evaluation. Which is why ideally the selector input would be a property of the action_selector, not the table. Having the selector input has part of the match key, even with a special match type and even if it does contribute to choosing an action entry, seems "wrong", as logically it is really a 2-step selection process: first you choose a group using a regular match, then you choose a member in the group by computing a hash over the selector input.
I tried coming up with an alternative extern definition for action_selector, but couldn't come up with anything within the boundaries of the current syntax (basically externs cannot "store" references to other P objects like fields), so this work-around works for me in the meantime.

When is the selector input evaluated?

logically that would be after the match key (obviously omitting the fields with match type selector) is evaluated and the match was performed.

The match key is evaluated twice, once for each table that uses the selector.
So the selector input is evaluated twice as well?

yes although note that most of the time tables sharing an action_selector will be mutually exclusive or only one of them will result in a match and therefore in a evaluation of the selector input.

mihaibudiu · 2017-03-09T17:39:56Z

backends/bmv2/jsonconverter.cpp

-                    auto sz = eb->getParameterValue(v1model.action_profile.sizeParam.name);
-                    BUG_CHECK(sz->is<IR::Constant>(), "%1%: expected a constant", sz);
-                    action_profile->emplace("max_size", sz->to<IR::Constant>()->value);
+


It looks like much of this code is duplicated for the case where a ConstructorCallExpression is used.
It would have been nice to share this code, but it is not high priority.

antoninbas · 2017-03-09T18:06:59Z

@mbudiu-vmw I can address all of your style comments before merging. However ideally you would fix the regression for #352 first, so it can be merged and I can update the test output for my new test.

mihaibudiu · 2017-03-09T18:12:38Z

I don't know why the build of #352 fails; it includes a fix from @ChrisDodd which should have prevented the failure. I will take a closer look.

mihaibudiu · 2017-03-09T18:45:24Z

I will send an update to PR #352 to fix the failure.

antoninbas · 2017-03-09T19:48:48Z

Please don't merge yet, I am integrating some review feedback

In the bmv2 backend. We basically added a pass to the backend which ensures that when several match tables share the same action_selector, the selector input is the same across all tables.

mihaibudiu · 2017-03-10T17:10:17Z

backends/bmv2/jsonconverter.cpp

+                auto cmp_e = [e1](const IR::Expression *e2) {
+                    // not the best solution but the best available one given that there is no "deep
+                    // comparison" of expressions for now.
+                    return e1->toString() == e2->toString();


This won't work; for example, toString() does not emit anything for array indexes. You will need to create a recursive function helper to detect whether two expressions are syntactically identical.

Since we only support simple expressions, would something like your checkSame method work: https://github.com/p4lang/p4c/blob/master/backends/bmv2/jsonconverter.cpp#L213

You may need to add the case for ArrayIndex to checkSame.
Indeed, there is a pass which simplifies the keys run beforehand.

antoninbas requested review from ChrisDodd and mihaibudiu March 9, 2017 02:02

mihaibudiu approved these changes Mar 9, 2017

View reviewed changes

ChrisDodd approved these changes Mar 9, 2017

View reviewed changes

Enable sharing an action_selector between tables

c4403e1

In the bmv2 backend. We basically added a pass to the backend which ensures that when several match tables share the same action_selector, the selector input is the same across all tables.

antoninbas force-pushed the action-selector-sharing branch from 2f377bd to c4403e1 Compare March 9, 2017 21:00

antoninbas merged commit 28c620d into master Mar 9, 2017

antoninbas deleted the action-selector-sharing branch March 9, 2017 22:08

mihaibudiu reviewed Mar 10, 2017

View reviewed changes

fefer mentioned this pull request Aug 7, 2018

"make" get's 100% without error, but "make check" fails #1433

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable sharing an action_selector between tables #354

Enable sharing an action_selector between tables #354

antoninbas commented Mar 9, 2017

mihaibudiu left a comment

mihaibudiu Mar 9, 2017

mihaibudiu Mar 9, 2017

mihaibudiu Mar 9, 2017

antoninbas Mar 9, 2017

mihaibudiu Mar 9, 2017

antoninbas Mar 9, 2017

mihaibudiu Mar 9, 2017

antoninbas Mar 9, 2017

mihaibudiu Mar 9, 2017

antoninbas commented Mar 9, 2017

mihaibudiu commented Mar 9, 2017

mihaibudiu commented Mar 9, 2017

antoninbas commented Mar 9, 2017

mihaibudiu Mar 10, 2017

antoninbas Mar 10, 2017

mihaibudiu Mar 10, 2017

Enable sharing an action_selector between tables #354

Enable sharing an action_selector between tables #354

Conversation

antoninbas commented Mar 9, 2017

mihaibudiu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoninbas commented Mar 9, 2017

mihaibudiu commented Mar 9, 2017

mihaibudiu commented Mar 9, 2017

antoninbas commented Mar 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment