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

Handle correctly general switch statements when simplifying #3624

Merged
merged 2 commits into from
Nov 2, 2022
Merged

Handle correctly general switch statements when simplifying #3624

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b5980b6
Handle correctly general switch statements when simplifying
Oct 26, 2022
75bd26b
Properly handle side-effects when removing switch statements
Oct 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Handle correctly general switch statements when simplifying 
Signed-off-by: Mihai Budiu <mbudiu@vmware.com>
  • Loading branch information
Mihai Budiu committed Oct 26, 2022
commit b5980b643c4bb9537dddc19b88e781fb56b853d4
20 changes: 10 additions & 10 deletions frontends/p4/simplify.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,16 +92,16 @@ const IR::Node* DoSimplifyControlFlow::postorder(IR::EmptyStatement* statement)
const IR::Node* DoSimplifyControlFlow::postorder(IR::SwitchStatement* statement) {
LOG3("Visiting " << dbp(getOriginal()));
if (statement->cases.empty()) {
// The P4_16 spec prohibits expressions other than table application as
// switch conditions. The parser should have rejected programs for
// which this is not the case.
BUG_CHECK(statement->expression->is<IR::Member>(),
"%1%: expected a Member", statement->expression);
auto expr = statement->expression->to<IR::Member>();
BUG_CHECK(expr->expr->is<IR::MethodCallExpression>(),
"%1%: expected a table invocation", expr->expr);
auto mce = expr->expr->to<IR::MethodCallExpression>();
return new IR::MethodCallStatement(mce->srcInfo, mce);
// If this is a table application remove the switch altogether but keep
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have a test for this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also another test for a function call with side-effects could be useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

issue3619-1.p4 is testing the side-effect.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But that function call is side-effect free. Also this does not include a table.apply(), right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This pass does not know that the function has no side effects. All tests with table applications would qualify for the second case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

if (SideEffects::check(statement->expression, this, refMap, typeMap))
    // This can happen if this pass is run before SideEffectOrdering.
    return statement;

I am trying to understand this segment. If the function call has side effects, ideally only the method call statement should remain, right? I do not think we have a test for this.

Similarly,
if (TableApplySolver::isActionRun(statement->expression, refMap, typeMap)) {
should check whether there is an action_run call and then only leave the table apply statement.

I assume we have tests already for this case? Since this was the default behavior.

Copy link
Contributor Author

@mihaibudiu mihaibudiu Nov 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

If the argument of the switch may have side-effects then we leave the whole switch unchaged. This pass is invoked again later, after SideEffectOrdering has removed calls with side effects from switch statements (all except table.apply().action_run), and that case will never occur again.

// the table application. Otherwise remove the switch altogether.
if (auto mem = statement->expression->to<IR::Member>()) {
if (auto mce = mem->expr->to<IR::MethodCallExpression>()) {
LOG2("Removing switch statement " << statement << " keeping " << mce);
return new IR::MethodCallStatement(mce->srcInfo, mce);
}
}
LOG2("Removing switch statement " << statement);
return nullptr;
}
auto last = statement->cases.back();
if (last->statement == nullptr) {
Expand Down
7 changes: 7 additions & 0 deletions testdata/p4_16_samples/issue3619.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
control c(in bit<4> v) {
apply {
switch(v) {
default:
}
}
}
5 changes: 5 additions & 0 deletions testdata/p4_16_samples_outputs/issue3619-first.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
control c(in bit<4> v) {
apply {
}
}

Empty file added testdata/p4_16_samples_outputs/issue3619-frontend.p4
View file
Open in desktop
Empty file.
8 changes: 8 additions & 0 deletions testdata/p4_16_samples_outputs/issue3619.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
control c(in bit<4> v) {
apply {
switch (v) {
default:
}
}
}

4 changes: 4 additions & 0 deletions testdata/p4_16_samples_outputs/issue3619.p4-stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
issue3619.p4(4): [--Wwarn=missing] warning: SwitchCase: fallthrough with no statement
default:
^^^^^^^
[--Wwarn=missing] warning: Program does not contain a `main' module