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

Three minor changes #370

Merged
merged 5 commits into from
Mar 23, 2017
Merged

Three minor changes #370

merged 5 commits into from
Mar 23, 2017

Conversation

ChrisDodd
Copy link
Contributor

These relate to #367, in that they are minor cleanups that help with it, plus a test case that is problematic.

The ability to disable the SideEffectOrdering pass is a bit of a hack -- it is problematic in that it introduces too many unneeded copies that are too hard to get rid of, and isn't really necessary for most targets. It should probably be a midend pass rather than a frontend pass in the first place.

@mihaibudiu
Copy link
Contributor

This pass does many important things, including making sure that the evaluation order is the one specified by the language. It takes care of many corner cases. Many subsequent passes assume that these corner cases do not happen; for example, arguments to calls never alias after this pass, so the inliner can be much simpler.

The SideEffectOrdering pass is the only pass which was specific to P4 v1. The PR #367 makes this pass independent on the source language.

Previously we could skip this pass for P4 v1 because we knew these corner cases cannot be expressed in P4-14. In general it should not be disabled for P4-16 programs - that's why it is in the front-end.

@mihaibudiu
Copy link
Contributor

Please note that #367 causes the side-effect ordering to introduce many fewer copies. In fact, it will never introduce copies for P4-14 programs.

@ChrisDodd
Copy link
Contributor Author

#367 introduces more copies for P4_14 programs, which is the problem. Particularly it introduces extra copies for parser lookahead.

@mihaibudiu
Copy link
Contributor

In general the copies are necessary to correctly generate code for bmv2. The language supports expressions which are too complicated for bmv2 to handle directly in select expressions, e.g.:
select(a.b, p.lookahead<H>().x)

In general, the back-end should be prepared to handle lookahead that reads into temporaries.
I think that the right approach is to teach the copy propagation pass about lookahead.

There are also some subtle issues related to exceptions and how much you readahead.

Copy link
Contributor

@mihaibudiu mihaibudiu left a comment

Choose a reason for hiding this comment

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

I approve all changes, but I expressed my opinion with respect to the sideEffects.

It was a lot of work to make the bmv2 back-end work with the sideEffects changes, so I expect it will take some work with other back-ends. But the BMv2 back-end didn't really support the language in its full generality; the back-end is now much more powerful. That's why I could actually delete some code from that back-end, which was handling lookahead in a special way.

For example, users can write expressions involving lookahead, and the json generated will be correct. We are not relying on lookahead being only invoked within a select.

@ChrisDodd ChrisDodd requested a review from cole-barefoot March 22, 2017 19:51
Copy link
Contributor

@cc10512 cc10512 left a comment

Choose a reason for hiding this comment

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

please try to resolve the PI submodule changes before merging.

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