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

Fixes for issues #355 and #356 #367

Merged
merged 12 commits into from
Mar 24, 2017
Merged

Fixes for issues #355 and #356 #367

merged 12 commits into from
Mar 24, 2017

Conversation

mihaibudiu
Copy link
Contributor

@mihaibudiu mihaibudiu commented Mar 18, 2017

This started as a normal attempt to fix some reported bugs; in the process it has ballooned to a complicated PR, because while testing more bugs were found and fixed. Unfortunately it is not easy to break down this PR into multiple smaller ones, because it would be quite difficult to make the tests run correctly in the intermediate stages; you will notice that there are not that many code changes, but there are lots of reference output changes.

Here is a list of the actual modifications made in this PR:

  • translation of P4-14 current() to lookahead was incorrect; implementation of lookahead in BMv2 was incorrect both were confusing the bit order, and were cancelling each other.
  • Fix for issue Lookahead does not work on bmv2 #355 - convert lookahead<T>() into lookahead<sizeof(T)> followed by a sequence of Slices; this is pass ExpandLookahead
  • Fix for issue Incorrect @name annotation for key #366 - BMv2 does not support expressions in select expressions, so these are lifted into temporaries (pass RemoveExpressionsFromSelect)
  • Lower nested list expressions in select expressions; pass SimplifySelectList
  • removed special handling of P4-14 programs from the SideEffects pass
  • in the SideEffect pass do not create temporaries for expressions that do not have side effects
  • removed special cases for lookahead code generation from BMv2 backend
  • enable loading all v1model json programs to be loaded into simple_switch by using an empty STF file if none is present
  • fixed bug in BMv2 back-end where incorrect type name was used (external name should be used)
  • support for setValid and setInvalid in BMv2 back-end
  • converted some LOG1 to LOG3
  • initial support for boolean values in select expressions (addressing issue Boolean labels are not supported for select expressions on BMv2 back-end #361; not yet fully tested)
  • renamed class SimplifySelect into SimplifySelectCases
  • separated a new pass TableHit pass from the SideEffects pass; TableHit converts expressions involving table.apply().hit into if statements
  • removed table parameters from the grammar and all supporting code; adapted all tests removing parameter list from tables; removed the pass that eliminated table parameters
  • fixed bug in ToP4 pass which used the wrong list terminators within Select expressions
  • dissalow expressions typed as headers in select expressions

@mihaibudiu
Copy link
Contributor Author

This change could become somewhat less intrusive if we still allowed parentheses after table declarations.
This change disallows such table declarations:

table t() { ... }

If there can never be table arguments, why have the parentheses?
On the other hand, table apply invocations still require parentheses: t.apply(), so perhaps we should allow them in table declarations.

@ChrisDodd
Copy link
Contributor

I see that most of our existing examples use () on tables with no parameters, even though they were not required...

@mihaibudiu
Copy link
Contributor Author

Yes, and that's why there are so many changed files.

@ChrisDodd
Copy link
Contributor

Overall looks fine, though it causes some issues with overeager copy introduction in our backend, so it may be a bit before I can merge it.

@mihaibudiu
Copy link
Contributor Author

There are potentially more copied introduced for P4-14 programs, but there should be fewer copies introduced for P4-16 programs. The only change in copy introductions is in sideEffects, where I was never introducing any copies for P4-14 programs, and now I am doing a relatively crude alias analysis. If the crude alias analysis is the problem we can enhance that.

Can you produce a test case that exhibits the undesired behavior?

Unfortunately I have not used a branch, so I am having now difficulties to create other PR which do not depend on this one. I have one queued for issue #364...

passes.push_back(new TypeChecking(refMap, typeMap));
passes.push_back(new DoSimplifyExpressions(refMap, typeMap));
SideEffectOrdering(ReferenceMap* refMap, TypeMap* typeMap, bool skipSideEffectOrdering) {
if (!skipSideEffectOrdering) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have done it this way to preserve the pass numbering, used by the testing scripts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary, as p4test always runs with this flag false, and changing it would change the outputs anyways.

passes.push_back(new TypeChecking(refMap, typeMap));
passes.push_back(new DoSimplifyExpressions(refMap, typeMap));
SideEffectOrdering(ReferenceMap* refMap, TypeMap* typeMap, bool skipSideEffectOrdering) {
if (!skipSideEffectOrdering) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary, as p4test always runs with this flag false, and changing it would change the outputs anyways.

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.

2 participants