Skip to content
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

Fix slice generation for table keys in P4_14->16 conversion #405

Merged
merged 2 commits into from
Mar 29, 2017

Conversation

ChrisDodd
Copy link
Contributor

This fixes the P4_14->P4_16 converted to not generate completely invalid code for this case, but it still causes problems in the typechecker when the mask is disjoint.

Trying to split such disjoint masks into multiple key elements would be possible, but its not clear how the name/API should be managed for the result. Easiest would be if P4_16 supported mask expressions in keys like P4_14 -- see issue #404

value = value - range.value;
auto range = Util::findOnes(value);
if (value != range.value) {
/* FIXME -- can't represent the mask as a single slice -- no way to express this
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The key can be expressed as a concatenation of slices.
Alternatively, the key could be a slice from the first 1 to the last 1 bit.
The actual representation depends on how the information is used in the back-end and in the control-plane API. Until we have a specification on how that is done we cannot really write this code.

@ChrisDodd
Copy link
Contributor Author

I've pushed a further change that allows the masked keys to get through the frontend (not issing a type error, and adding a policy that allows them), and generate the json for the bmv2 backend which makes this test case work.

The p14_to_16 translation fails because on reparsing the dumped p4_16 code, it chokes on the mask in the key. Adding support for that would be a trivial parser addition.

@ChrisDodd ChrisDodd force-pushed the cdodd branch 2 times, most recently from 581ba03 to 8a53679 Compare March 28, 2017 21:38
@mihaibudiu
Copy link
Contributor

We have two solutions:

  • we change the P4-16 spec and define masks for keys; then we can parse this, and we add support everywhere for it. In particular, the typechecker has to check that the mask and the key have the same type too.
  • we don't change the spec, but we support P4-14 on BMv2; for this purpose the solution is to use an annotation called @Mask on the key to carry the mask through the front-end. The back-end will have to do the typechecking during code generation. This should be relatively easy since the mask is supposed to be a constant anyway.

Neither of these is too much work. I would propose using the second solution currently - at least until the spec is modified. Your PR would be easy to fix to use the second solution, and it should not need to touch any of the intermediate passes.

@ChrisDodd
Copy link
Contributor Author

Actually, now I think about it more, since P4_16 allow arbitrary expressions in tables keys, we don't really need masks as such -- just use a & (bitwise and) expression with a constant. The result will be an expression with the ignored bits zeroed out, which is more or less what BMV2 wants.

@mihaibudiu
Copy link
Contributor

You are right.
If the control-plane does not need to know the mask this is probably all you need.
There is still the generation of the @name annotation for the key - which is done in a latter pass.
You may need to do it right when you create the key.

@ChrisDodd
Copy link
Contributor Author

So I've updated this change to use IR::BAnd expressions when converting mask keys that can't be converted into simple slices. I've also extended tableKeyNames to generate a name for the BAnd when one operand is a constant, so this now all works correctly.

@ChrisDodd ChrisDodd merged commit dd43fce into p4lang:master Mar 29, 2017
hanw pushed a commit to hanw/p4c that referenced this pull request Apr 4, 2017
* Fix slice generation for table keys in P4_14->16 conversion
* Allow masked table keys in frontend and bmv2 backend
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants