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

Remove predication pass from p4test #3916

Merged
merged 3 commits into from
Mar 23, 2023
Merged

Conversation

mihaibudiu
Copy link
Contributor

The main problem with the predication pass is that it either converts all ifs in actions to conditionals, or it emits a hard error.
The ideal solution would have the predication pass leave expressions it cannot handle untouched in the hope a subsequent pass can fix them.
A less ideal solution would be to have the predication pass leave the program unchanged if it fails to convert everything.
For now I have disabled completely the predication pass in p4test.
The downside is that the tests for predication which relied on p4test no longer do anything.
But hopefully other backends exercise predication enough.
The upside is that we remove many xfails.

Mihai Budiu added 2 commits March 4, 2023 16:21
Signed-off-by: Mihai Budiu <mbudiu@vmware.com>
Signed-off-by: Mihai Budiu <mbudiu@vmware.com>
@mihaibudiu mihaibudiu requested a review from fruffy March 5, 2023 02:20
@fruffy
Copy link
Collaborator

fruffy commented Mar 5, 2023

Any particular reason the pass can not handle the conditionals in actions? Is that just because of BMv2? Or is the analysis too complex? Otherwise we could just parametrize.

@mihaibudiu
Copy link
Contributor Author

Predication cannot handle all forms of conditionals; in particular it cannot convert something like if (condition) function();
When it cannot convert a statement, it will report an error and stop.

@jafingerhut
Copy link
Contributor

I checked the first 10 to 15 of the updated -midend.p4 expected output files in p4_16_samples_outputs, and they looked correct to me, but there are a lot more of them than that.

Does gauntlet run on the contents of all PR's?

If not, when does gauntlet run?

@fruffy
Copy link
Collaborator

fruffy commented Mar 6, 2023

Does gauntlet run on the contents of all PR's?

If not, when does gauntlet run?

It does run on all the programs in the P4-16 testdata folder for every PR. However, it only runs on the output of p4test.

Copy link
Contributor

@jafingerhut jafingerhut left a comment

Choose a reason for hiding this comment

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

LGTM. I checked about the first 10 to 15 of the updated midend expected output files by hand, and found no errors. Fabian says that Gauntlet is run on all of the p4test expected output files, so hopefully it should catch any errors in any others that I did not check by hand.

@fruffy
Copy link
Collaborator

fruffy commented Mar 6, 2023

LGTM. I checked about the first 10 to 15 of the updated midend expected output files by hand, and found no errors. Fabian says that Gauntlet is run on all of the p4test expected output files, so hopefully it should catch any errors in any others that I did not check by hand.

Predication was actually a major source of mistranslations in the compiler. It would make sense that Gauntlet has no issues with it being removed.

Signed-off-by: Mihai Budiu <mbudiu@vmware.com>
@mihaibudiu mihaibudiu merged commit ae32631 into p4lang:main Mar 23, 2023
@mihaibudiu mihaibudiu deleted the nopredication branch March 23, 2023 21:15
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