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

Support emits of structs; test parsing of varbit fields #634

Merged
merged 5 commits into from
May 24, 2017

Conversation

mihaibudiu
Copy link
Contributor

This does not solve issue #447 completely, only partially.
More fixes will be necessary.
It should solve issue #630 completely, though.

auto vec = new IR::Vector<IR::StatOrDecl>();
auto it = expansionTypes.begin();
for (auto e : expansion) {
auto method = statement->methodCall->method->clone();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why clone the method if you're not changing it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These must be different IR nodes, otherwise all analyses which compute types, def-use, etc. will produce wrong results - the results are usually indexed in a hash-table by IR node.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A bit worried we may actually need a deeper clone.

Copy link
Contributor

Choose a reason for hiding this comment

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

This probably indicates that we should fix those analyses to work properly in the presence of dags -- just storing pointers to IR nodes is not adequate if it also depends on the context of those nodes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is really easiest to have nodes be distinct rather than look at the context.

The question is whether these analyses should establish this invariant by themselves. Is setting visitDagOnce = false enough for an analysis to duplicate all nodes?

@mihaibudiu mihaibudiu merged commit 6d1a821 into p4lang:master May 24, 2017
@mihaibudiu mihaibudiu deleted the issue447 branch May 26, 2017 15:03
mihaibudiu pushed a commit that referenced this pull request May 30, 2017
* Fix for issue #638

* IRGen fixes

- support for autogenerating methods in nested classes
- support for friend functions
- misc bug fixes

* cpplint fixes

* Update comment

* Minor change in bmv2 backend: set action_const (#652)

In the bmv2 JSON, a match-table can indicate that its default action is
constant (using 'action_const') and that the default action entry
(action + action parameter values) is constant (using
'action_entry_const'). `action_entry_const == true => action_const ==
true`. The bmv2 backend was setting 'action_entry_const' correctly, but
'action_const' was always set to false. Note that P4_16 does not make a
difference between the 2, the presence of `const` in the P4_16 source
implies that both attributes should be true.

* Fix for issue #629 (#636)

* Support emits of structs; test parsing of varbit fields (#634)

* Support emits of structs; test parsing of varbit fields

* Fixes issue #447 and #630

* Updated reference outputs

* Additional tests and bug-fixes for varbits

* Fix for issue #638
* Allow empty actions list in tables
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