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 for issue #638 #640

Closed
wants to merge 1 commit into from
Closed

Fix for issue #638 #640

wants to merge 1 commit into from

Conversation

mihaibudiu
Copy link
Contributor

The spec allows tables without default_actions; in this case we insert a default_action of NoAction very early in the compilation process.

The default_only property was misspelled in the code; fixing this causes lots of reference outputs to change.

@ChrisDodd
Copy link
Contributor

It seems to me that this is the wrong approach -- the point of the spec change was to simplify things, so we should be removing things that are no longer necessary, not addiing additional stuff:

  • remove the code in fromV1 that creates/add NoAction default actions, as it is no longer needed
  • remove NoAction from v1model
  • remove the checks for not having default actions

That should be all that is needed. We could think about having a midend pass that added NoAction as default actions, but bmv2 does not need it.

@mihaibudiu
Copy link
Contributor Author

We can probably remove these from the v1 path, since this path does the same thing. However, this path is necessary, because this is the semantics of tables, they must have some default action, it is in the spec. Moreover, many analyses rely on the fact that all actions are explicitly present in the actions list, it is much simpler to add NoAction there than to special-case the analyses. See also the discussion about this issue with the spec.

@ChrisDodd
Copy link
Contributor

The spec has been changed so that they no longer need a default action.

@mihaibudiu
Copy link
Contributor Author

They don't need it in the user program, but the semantics of the table application always requires the existence of default actions, and that is why the compiler synthesises them here. It's simpler for the programmer, but the same for us.

@mihaibudiu
Copy link
Contributor Author

I have removed the insertion of default action from the v1->p4-16 path.

This pass implements section 12.2 from the spec, the following paragraph:

The compiler may set the default_action to NoAction (and also insert it into the list of actions) for tables that do not define the default_action property. This is consistent with the semantics given in Section 12.2.1.3.

@mihaibudiu
Copy link
Contributor Author

Ooops, I have pushed this change to the wrong branch...

@mihaibudiu
Copy link
Contributor Author

This should have been in branch issue648, which was in a PR which has already been merged; I will submit another PR. I will also try to fix this one...

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