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.

Sign up for GitHub

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

Jump to bottom

Make StateVariable an IR class that accepts IR::Member. Use ICastable for some nodes. #3741

Merged
merged 7 commits into from
Apr 17, 2023
Merged

Make StateVariable an IR class that accepts IR::Member. Use ICastable for some nodes. #3741

merged 7 commits into from
Apr 17, 2023

Conversation

fruffy
Copy link
Collaborator

@fruffy fruffy commented Nov 29, 2022 •
edited
Loading

Some preparation for #3519. Remove implicit casting from StateVariable and explicitly require IR::Member as initializer argument.

This required a small change to ir-generator.ypp. Those operators were not part of the syntax.

@fruffy fruffy force-pushed the fruffy/state_variable branch 6 times, most recently from b3a31c6 to a3ea011 Compare November 30, 2022 14:11
@fruffy fruffy marked this pull request as ready for review December 1, 2022 15:23
@fruffy fruffy changed the title Make StateVariable only accept IR::Member. Use ICastable for some nodes. Make StateVariable and IR class that accepts IR::Member. Use ICastable for some nodes. Dec 1, 2022
@fruffy fruffy changed the title Make StateVariable and IR class that accepts IR::Member. Use ICastable for some nodes. Make StateVariable an IR class that accepts IR::Member. Use ICastable for some nodes. Dec 1, 2022
@fruffy fruffy added the p4tools Topics related to the P4Tools back end label Dec 5, 2022
@fruffy fruffy force-pushed the fruffy/state_variable branch 2 times, most recently from 30dccf6 to 82d7547 Compare April 13, 2023 21:23
pkotikal
pkotikal reviewed Apr 13, 2023
View reviewed changes
backends/p4tools/modules/testgen/lib/concolic.h Outdated
@@ -28,7 +27,7 @@ namespace P4Tools::P4Testgen {
/// are present, but not expressions. The reason expressions need to be keys is that sometimes
/// entire expressions are mapped to a particular constant.
using ConcolicVariableMap =
ordered_map<std::variant<const StateVariable, const IR::Expression *>, const IR::Expression *>;
std::map<std::variant<const IR::StateVariable, const IR::Expression *>, const IR::Expression *>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do the ordering of any of these maps and sets matter - do we need them for deterministic behavior when passing the same seed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope, that was a rebase mistake, good catch!

pkotikal
pkotikal reviewed Apr 13, 2023
View reviewed changes
backends/p4tools/modules/testgen/lib/test_backend.cpp Outdated
@@ -64,8 +63,9 @@ const Model *TestBackEnd::computeConcolicVariables(const ExecutionState *executi
const auto *concolicAssignment = resolvedConcolicVariable.second;
const IR::Expression *pathConstraint = nullptr;
// We need to differentiate between state variables and expressions here.
if (std::holds_alternative<const StateVariable>(concolicVariable)) {
pathConstraint = new IR::Equ(std::get<const StateVariable>(concolicVariable),
// We need to differentiate between state variables and expressions here.
Copy link
Contributor

Choose a reason for hiding this comment

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

Repeated comment.

pkotikal
pkotikal reviewed Apr 13, 2023
View reviewed changes
backends/p4tools/modules/testgen/test/z3-solver/expressions.cpp Outdated
@@ -125,9 +125,9 @@ void test(const IR::Expression *expression, const IR::AssignmentStatement *varia
Model model = *solver.getModel();
ASSERT_EQ(model.size(), 2u);

ASSERT_EQ(model.count(variableValue->left), 1u);
ASSERT_EQ(model.count(variableValue->left->checkedTo<IR::Member>()), 1u);
Copy link
Contributor

Choose a reason for hiding this comment

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

1U

pkotikal
pkotikal approved these changes Apr 13, 2023
View reviewed changes
Copy link
Contributor

@pkotikal pkotikal left a comment

Choose a reason for hiding this comment

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

LGTM.

@fruffy
Copy link
Collaborator Author

fruffy commented Apr 13, 2023

@mbudiu-vmw This adds new operators to the syntax in the ir-generator yacc file. Any potential issues that could arise here?

@mihaibudiu
Copy link
Contributor

@mbudiu-vmw This adds new operators to the syntax in the ir-generator yacc file. Any potential issues that could arise here?

You probably mean that you have a new .def file. That should always work.

@fruffy
Copy link
Collaborator Author

fruffy commented Apr 13, 2023

@mbudiu-vmw This adds new operators to the syntax in the ir-generator yacc file. Any potential issues that could arise here?

You probably mean that you have a new .def file. That should always work.

I was referring to the edits here:
https://github.com/p4lang/p4c/pull/3741/files#diff-0ee13e28f9c23c0a103c9e59759d5e112d41813c3dc330b66ac922c596c77177R288

The syntax was missing -> and * which need to be overridden for this particular IR construct.

@mihaibudiu
Copy link
Contributor

@mbudiu-vmw This adds new operators to the syntax in the ir-generator yacc file. Any potential issues that could arise here?

You probably mean that you have a new .def file. That should always work.

I was referring to the edits here: https://github.com/p4lang/p4c/pull/3741/files#diff-0ee13e28f9c23c0a103c9e59759d5e112d41813c3dc330b66ac922c596c77177R288

The file in question has been written entirely by @ChrisDodd

@fruffy fruffy requested a review from ChrisDodd April 14, 2023 18:05
@fruffy fruffy merged commit 1335eb6 into main Apr 17, 2023
@fruffy fruffy deleted the fruffy/state_variable branch April 17, 2023 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pkotikal pkotikal pkotikal approved these changes

@vhavel vhavel Awaiting requested review from vhavel

@jnfoster jnfoster Awaiting requested review from jnfoster

@mihaibudiu mihaibudiu Awaiting requested review from mihaibudiu

@ChrisDodd ChrisDodd Awaiting requested review from ChrisDodd

Assignees
No one assigned
Labels
p4tools Topics related to the P4Tools back end
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fruffy @mihaibudiu @pkotikal