Skip to content

Commit

Permalink
[#4656] Explicitly delay constant folding of only action enum `IR::Sw…
Browse files Browse the repository at this point in the history
…itchCase` `label` expressions, instead of delaying constant folding of all `IR::Mux` expressions (#4657)

* Delay constant folding IR::Mux expressions only if we are inside of a label of an action enum SwitchStatement

* Apply IR::Mux constant folding logic to all switch case labels. We don't know if it is an action enum switch statement until after type inference has run.

* Move issue3699 test to p4_16_samples/ as we now have the proper fix for #3699

* Add IR::SwitchCase preorder function instead of visiting all IR::Mux expressions.

* appease cpplint

* try to appease cpplint

* Add test with compile-time constant expressions in labels of regular switch statement labels

* Explicitly check whether SwitchCase belongs to a SwitchStatement with action_run expression

* appease p4c-lint

* Invert logic to skip visiting action_run SwitchCase labels

* Move isActionRun() to anonymous namespace

* Add comment about static_cast
  • Loading branch information
kfcripps committed May 15, 2024
1 parent b4d8e06 commit 6343eab
Show file tree
Hide file tree
Showing 21 changed files with 406 additions and 73 deletions.
40 changes: 37 additions & 3 deletions frontends/common/constantFolding.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,43 @@ const IR::Node *DoConstantFolding::preorder(IR::ArrayIndex *e) {
return e;
}

namespace {

// Returns true if the given expression is of the form "some_table.apply().action_run."
bool isActionRun(const IR::Expression *e, const ReferenceMap *refMap) {
const auto *actionRunMem = e->to<IR::Member>();
if (!actionRunMem) return false;
if (actionRunMem->member.name != IR::Type_Table::action_run) return false;

const auto *applyMce = actionRunMem->expr->to<IR::MethodCallExpression>();
if (!applyMce) return false;
const auto *applyMceMem = applyMce->method->to<IR::Member>();
if (!applyMceMem) return false;
if (applyMceMem->member.name != IR::IApply::applyMethodName) return false;

const auto *tablePathExpr = applyMceMem->expr->to<IR::PathExpression>();
if (!tablePathExpr) return false;
const auto *tableDecl = refMap->getDeclaration(tablePathExpr->path);
if (!tableDecl) return false;

return tableDecl->is<IR::P4Table>();
}

} // namespace

const IR::Node *DoConstantFolding::preorder(IR::SwitchCase *c) {
// Action enum switch case labels must be action names.
// Do not fold the switch case's 'label' expression so that it can be inspected by the
// TypeInference pass.
// Note: static_cast is used as a SwitchCase's parent is always SwitchStatement.
const auto *parent = static_cast<const IR::SwitchStatement *>(getContext()->node);
if (isActionRun(parent->expression, refMap)) {
visit(c->statement);
prune();
}
return c;
}

const IR::Node *DoConstantFolding::postorder(IR::Cmpl *e) {
auto op = getConstant(e->expr);
if (op == nullptr) return e;
Expand Down Expand Up @@ -710,9 +747,6 @@ const IR::Node *DoConstantFolding::postorder(IR::LNot *e) {
}

const IR::Node *DoConstantFolding::postorder(IR::Mux *e) {
if (!typesKnown)
// We want the typechecker to look at the expression first
return e;
auto cond = getConstant(e->e0);
if (cond == nullptr) return e;
auto b = cond->to<IR::BoolLiteral>();
Expand Down
1 change: 1 addition & 0 deletions frontends/common/constantFolding.h
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ class DoConstantFolding : public Transform {
const IR::Node *postorder(IR::IfStatement *statement) override;
const IR::Node *preorder(IR::AssignmentStatement *statement) override;
const IR::Node *preorder(IR::ArrayIndex *e) override;
const IR::Node *preorder(IR::SwitchCase *c) override;
const IR::BlockStatement *preorder(IR::BlockStatement *bs) override {
if (bs->annotations->getSingle("disable_optimization")) prune();
return bs;
Expand Down
2 changes: 1 addition & 1 deletion frontends/p4/frontend.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ const IR::P4Program *FrontEnd::run(const CompilerOptions &options, const IR::P4P
// Type checking and type inference. Also inserts
// explicit casts where implicit casts exist.
new SetStrictStruct(&typeMap, true), // Next pass uses strict struct checking
new TypeInference(&refMap, &typeMap, false, false), // insert casts, dont' check arrays
new TypeInference(&refMap, &typeMap, false, false), // insert casts, don't check arrays
new SetStrictStruct(&typeMap, false),
new ValidateMatchAnnotations(&typeMap),
new ValidateValueSets(),
Expand Down
69 changes: 0 additions & 69 deletions testdata/p4_16_errors_outputs/issue3699.p4-stderr

This file was deleted.

File renamed without changes.
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
#include <core.p4>

extern void foo();
extern void bar();
extern bit<8> baz();
action a(){}
action b(){}
control c() {
table t {
actions = { a ; b; }
}
apply {
switch(baz()) {
1 + 2 == 3 ? 1 : 2 : { foo(); }
3 + 4 == 0 ? 3 : 4 : { bar(); }
}
t.apply();
}
}

control C();
package top(C c);

top(c()) main;
22 changes: 22 additions & 0 deletions testdata/p4_16_samples/issue4656_fold_bit_width_const_mux_expr.p4
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
header h_t {
bit<((10 == 1 ? 1 : 10 + 1) * (8+1))> f;
}

control C() {
action bar() {
}

table t {
actions = { bar; }
default_action = bar;
}

apply {
t.apply();
}
}

control proto();
package top(proto p);

top(C()) main;
27 changes: 27 additions & 0 deletions testdata/p4_16_samples_outputs/issue3699-first.p4
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
const bit<4> one = 4w1;
const bit<4> max = 4w0xf;
const bit<4> value = 4w0xf;
const bit<4> value1 = 4w0xf;
header h {
}

void f(in h[1] a) {
}
void f1(in h[1] a) {
}
void f1(in h[1] a) {
}
void f1(in h[1] a) {
}
const int<5> ones = 5s1;
const int<5> maxs = 5s0xf;
const int<5> values = 5s0xf;
const int<5> values1 = 5s0xf;
void f(in h[1] a) {
}
void f1(in h[1] a) {
}
void f1(in h[1] a) {
}
void f1(in h[1] a) {
}
3 changes: 3 additions & 0 deletions testdata/p4_16_samples_outputs/issue3699-frontend.p4
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
header h {
}

File renamed without changes.
1 change: 1 addition & 0 deletions testdata/p4_16_samples_outputs/issue3699.p4-stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
[--Wwarn=missing] warning: Program does not contain a `main' module
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
#include <core.p4>

extern void foo();
extern void bar();
extern bit<8> baz();
action a() {
}
action b() {
}
control c() {
table t {
actions = {
a();
b();
@defaultonly NoAction();
}
default_action = NoAction();
}
apply {
switch (baz()) {
8w1: {
foo();
}
8w4: {
bar();
}
}
t.apply();
}
}

control C();
package top(C c);
top(c()) main;
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
#include <core.p4>

extern void foo();
extern void bar();
extern bit<8> baz();
control c() {
@name("c.tmp") bit<8> tmp;
@name(".a") action a_0() {
}
@name(".b") action b_0() {
}
@noWarn("unused") @name(".NoAction") action NoAction_1() {
}
@name("c.t") table t_0 {
actions = {
a_0();
b_0();
@defaultonly NoAction_1();
}
default_action = NoAction_1();
}
apply {
tmp = baz();
switch (tmp) {
8w1: {
foo();
}
8w4: {
bar();
}
}
t_0.apply();
}
}

control C();
package top(C c);
top(c()) main;
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
#include <core.p4>

extern void foo();
extern void bar();
extern bit<8> baz();
control c() {
@name("c.tmp") bit<8> tmp;
@name(".a") action a_0() {
}
@name(".b") action b_0() {
}
@noWarn("unused") @name(".NoAction") action NoAction_1() {
}
@name("c.t") table t_0 {
actions = {
a_0();
b_0();
@defaultonly NoAction_1();
}
default_action = NoAction_1();
}
bit<8> switch_0_key;
@hidden action switch_0_case() {
}
@hidden action switch_0_case_0() {
}
@hidden action switch_0_case_1() {
}
@hidden table switch_0_table {
key = {
switch_0_key: exact;
}
actions = {
switch_0_case();
switch_0_case_0();
switch_0_case_1();
}
const default_action = switch_0_case_1();
const entries = {
const 8w1 : switch_0_case();
const 8w4 : switch_0_case_0();
}
}
@hidden action issue4656_const_fold_generic_switch_label_expr14() {
foo();
}
@hidden action issue4656_const_fold_generic_switch_label_expr15() {
bar();
}
@hidden action issue4656_const_fold_generic_switch_label_expr13() {
switch_0_key = tmp;
}
@hidden action act() {
tmp = baz();
}
@hidden table tbl_act {
actions = {
act();
}
const default_action = act();
}
@hidden table tbl_issue4656_const_fold_generic_switch_label_expr13 {
actions = {
issue4656_const_fold_generic_switch_label_expr13();
}
const default_action = issue4656_const_fold_generic_switch_label_expr13();
}
@hidden table tbl_issue4656_const_fold_generic_switch_label_expr14 {
actions = {
issue4656_const_fold_generic_switch_label_expr14();
}
const default_action = issue4656_const_fold_generic_switch_label_expr14();
}
@hidden table tbl_issue4656_const_fold_generic_switch_label_expr15 {
actions = {
issue4656_const_fold_generic_switch_label_expr15();
}
const default_action = issue4656_const_fold_generic_switch_label_expr15();
}
apply {
tbl_act.apply();
tbl_issue4656_const_fold_generic_switch_label_expr13.apply();
switch (switch_0_table.apply().action_run) {
switch_0_case: {
tbl_issue4656_const_fold_generic_switch_label_expr14.apply();
}
switch_0_case_0: {
tbl_issue4656_const_fold_generic_switch_label_expr15.apply();
}
switch_0_case_1: {
}
}
t_0.apply();
}
}

control C();
package top(C c);
top(c()) main;
Loading

0 comments on commit 6343eab

Please sign in to comment.