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

Fixing bug for ParserUnroll application to a p4 program with a header union #3121

Conversation

VolodymyrPeschanenkoIntel
Copy link
Contributor

These small changes allow to use ParserUnroll class for the programs with header unions.

if (element->is<IR::ArrayIndex>()) {
const IR::Expression* left = element->to<IR::ArrayIndex>()->left;
if (left->type->is<IR::Type_Stack>())
return left->type->to<IR::Type_Stack>()->elementType;
}
return typeMap->getType(element, true);
auto* currentType = typeMap->getType(element);
Copy link
Collaborator

Choose a reason for hiding this comment

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

How can it happen that type information is missing here? Is this a problem with type inference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem with right part of the assignment. If element is hdr.h[0].i1.index then typeMap->getType(element) returns nullptr.

@mihaibudiu
Copy link
Contributor

The right solution is to fix the evaluation of expressions with type headerunion in the symbolic interpreter.
I think that #3063 attempts to do this, but it is not yet ready.

@VolodymyrPeschanenkoIntel
Copy link
Contributor Author

The right solution is to fix the evaluation of expressions with type header union in the symbolic interpreter. I think that #3063 attempts to do this, but it is not yet ready.

The getTypeArray function is not a part of the interpreter and It doesn't take part in a calculation process. #3063 contains changes for support header union in a evaluation process. I think that is a some particular case for support of header union.

@fruffy
Copy link
Collaborator

fruffy commented May 10, 2022

Now that #3063 has been merged what is the status of this pull request?

@VolodymyrPeschanenkoIntel
Copy link
Contributor Author

Now that #3063 has been merged what is the status of this pull request?

These changes still actual, I wait for review. #3063 contains update for interpreter. This PR contains changes inside parserUnroll.

@fruffy
Copy link
Collaborator

fruffy commented May 27, 2022

@mbudiu-vmw Do you have any comments on this pull request?

@fruffy fruffy requested a review from mihaibudiu May 27, 2022 20:33
@mihaibudiu
Copy link
Contributor

Yes, I have requested changes which have not been carried out.
There are a couple of comments in the code which have not been addressed.

@VolodymyrPeschanenkoIntel
Copy link
Contributor Author

Yes, I have requested changes which have not been carried out. There are a couple of comments in the code which have not been addressed.

Could, you, please, specify them?

@fruffy
Copy link
Collaborator

fruffy commented Sep 28, 2022

What is the status of this PR?

@VolodymyrPeschanenkoLitSoft
Copy link
Contributor

As far as I understood we are waiting for solving the issue with the description of header union in p4lang here. It was opened two years ago. It looks like we are stuck here.

@mihaibudiu
Copy link
Contributor

The compiler should usually be ahead of the spec - this helps us make sure that the spec is right.
I think that the intent was always to support stacks of header unions.

@VolodymyrPeschanenkoIntel
Copy link
Contributor Author

@mbudiu-vmw, Could you, please, make a review again?

midend/parserUnroll.cpp Outdated Show resolved Hide resolved
midend/parserUnroll.cpp Outdated Show resolved Hide resolved
[no-jira]
[no-jira]
[no-jira]
@mihaibudiu
Copy link
Contributor

This PR contains only test cases, is this intended?

@VolodymyrPeschanenkoLitSoft
Copy link
Contributor

This PR contains only test cases, is this intended?

I would like to add an additional test for header union stacks.

@VolodymyrPeschanenkoLitSoft VolodymyrPeschanenkoLitSoft merged commit ff35a94 into p4lang:main Oct 7, 2022
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.

4 participants