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

Infer types correctly for P4-14 action parameters and clone primitives #339

Conversation

sethfowler
Copy link
Contributor

We infer different types for action parameters than p4c-bm on a number of our internal test cases. I investigated, and it turns out that there are a few different issues in the P4-14 type inference algorithm that contribute to this problem. This PR fixes those issues.

ef71fd6 adds type inference information for the clone_spec operand of the various clone primitives. Previously, this operand was defaulting to Type_Unknown.

This didn't fix most of my test cases, and the reason turned out to be that the P4-14 type inference passes are Modifiers. This implies that, by default, they only visit each node once. However, TypeCheck::Pass1, we replace all references to an action argument with the ActionArg node they refer to. This means that only the first reference the type inference passes encounter gets considered for type inference.

There is an escape hatch for this: you can set visitDagOnce = false on a Modifier instance to allow it to visit the same node more than once. However, it clones the node anew each time, which prevents you from accumulating information from each visit to the node. We need to store that information off to the side instead, and we need to avoid invalidating the data structure we use until we have all the information we need.

This suggests that we should collect the necessary information in Inspector passes and then apply it in a Modifier pass. 905bb61 separates TypeCheck::Pass2 (the bottom-up pass) and TypeCheck::Pass3 (the top-down pass) into two bottom-up passes and two top-down passes. One pair of passes is for expressions; these passes continue to perform in-place modifications to the tree. The second pair is for action arguments; these passes are simply Inspectors that accumulate type information for each ActionArg. A final Modifier pass then applies the type information accumulated by the Inspectors to each ActionArg in the tree. Having names like Pass1 and Pass2 doesn't really scale to this many passes, so I renamed each pass to clarify what's going on.

This change makes us gather the correct information, but it introduces a number of type errors in existing test cases, because considering the different uses of each ActionArg leads us to infer different types for it. In particular we run into a number of cases where we infer bit<> types with several different widths. 5afa055 permits widening when combining these types, which fixes the type errors.

With these changes, we infer the same types that p4c-bm infers. The existing tests actually have pretty good coverage for all of these issues; all I needed to do was update the expected results, which is taken care of by 71e8592.

@sethfowler sethfowler self-assigned this Feb 28, 2017
@sethfowler sethfowler force-pushed the seth/infer-p4-14-action-parameter-types-correctly branch from 71e8592 to eb32807 Compare March 1, 2017 21:34
@sethfowler
Copy link
Contributor Author

Tests failed due to a cpplint issue. I've fixed it and rebased.

@sethfowler sethfowler merged commit 0dcd807 into p4lang:master Mar 1, 2017
@sethfowler sethfowler deleted the seth/infer-p4-14-action-parameter-types-correctly branch March 1, 2017 22:23
@sethfowler
Copy link
Contributor Author

FWIW: with this change, type checking is taking a long time to converge on some very large sample programs we have. I'm investigating. If it's not a quick fix, I'll revert until we can fix it.

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