diff --git a/backends/p4tools/modules/smith/scripts/compilation-test.sh b/backends/p4tools/modules/smith/scripts/compilation-test.sh index ab226243b46..0d5cca47091 100755 --- a/backends/p4tools/modules/smith/scripts/compilation-test.sh +++ b/backends/p4tools/modules/smith/scripts/compilation-test.sh @@ -12,7 +12,6 @@ KNOWN_BUGS=( "Cannot evaluate initializer for constant" "Null expr" "error: retval_1: declaration not found" # V1model failure. - "Compiler Bug: Cannot find declaration for inlinedRetval" # See #5549 "At this point in the compilation typechecking should not infer new types anymore" # See #5560 ) diff --git a/backends/tofino/bf-p4c/midend/remove_action_params.h b/backends/tofino/bf-p4c/midend/remove_action_params.h index 7675d64a6a5..c3d6c28093c 100644 --- a/backends/tofino/bf-p4c/midend/remove_action_params.h +++ b/backends/tofino/bf-p4c/midend/remove_action_params.h @@ -35,8 +35,9 @@ namespace BFN { */ class DoRemoveActionParametersTofino : public P4::DoRemoveActionParameters { public: - explicit DoRemoveActionParametersTofino(P4::ActionInvocation *inv) - : P4::DoRemoveActionParameters(inv) {} + DoRemoveActionParametersTofino(P4::ActionInvocation *inv, P4::TypeMap *typeMap, + MinimalNameGenerator *nameGen) + : P4::DoRemoveActionParameters(inv, typeMap, nameGen) {} /** * Check if there is a mark_to_drop action and don't replace parameters. @@ -186,8 +187,10 @@ class RemoveActionParameters : public PassManager { passes.emplace_back(new P4::MoveDeclarations()); if (!tc) tc = new P4::TypeChecking(refMap, typeMap); passes.emplace_back(tc); + auto nameGen = new MinimalNameGenerator(); passes.emplace_back(new P4::FindActionParameters(typeMap, ai)); - passes.emplace_back(new DoRemoveActionParametersTofino(ai)); + passes.emplace_back(nameGen); + passes.emplace_back(new DoRemoveActionParametersTofino(ai, typeMap, nameGen)); passes.emplace_back(new P4::ClearTypeMap(typeMap)); } }; diff --git a/frontends/p4/removeParameters.cpp b/frontends/p4/removeParameters.cpp index 733bea9004a..8dfaa0ffdfa 100644 --- a/frontends/p4/removeParameters.cpp +++ b/frontends/p4/removeParameters.cpp @@ -19,12 +19,47 @@ limitations under the License. #include "frontends/common/resolveReferences/resolveReferences.h" #include "frontends/p4/methodInstance.h" #include "frontends/p4/moveDeclarations.h" +#include "frontends/p4/sideEffects.h" #include "frontends/p4/tableApply.h" #include "frontends/p4/typeChecking/typeChecker.h" namespace P4 { namespace { + +// Extract side-effecting array index expressions into temporaries +// to ensure they are evaluated exactly once for inout copy-in/copy-out. +class ExtractArrayIndices : public Transform { + TypeMap *typeMap; + MinimalNameGenerator &nameGen; + IR::IndexedVector &tempDecls; + IR::IndexedVector &tempAssigns; + + public: + ExtractArrayIndices(TypeMap *typeMap, MinimalNameGenerator &nameGen, + IR::IndexedVector &tempDecls, + IR::IndexedVector &tempAssigns) + : typeMap(typeMap), nameGen(nameGen), tempDecls(tempDecls), tempAssigns(tempAssigns) { + setName("ExtractArrayIndices"); + } + const IR::Node *preorder(IR::ArrayIndex *aindex) override { + visit(aindex->left); + if (SideEffects::check(aindex->right, called_by, typeMap)) { + if (auto indexType = typeMap->getType(aindex->right, true)) { + auto tmp = nameGen.newName("tmp_idx"); + auto decl = new IR::Declaration_Variable(aindex->right->srcInfo, tmp, indexType); + tempDecls.push_back(decl); + auto assign = new IR::AssignmentStatement( + aindex->right->srcInfo, new IR::PathExpression(tmp), aindex->right); + tempAssigns.push_back(assign); + aindex->right = new IR::PathExpression(aindex->right->srcInfo, new IR::Path(tmp)); + } + } + prune(); + return aindex; + } +}; + // Remove arguments from any embedded MethodCallExpression class RemoveMethodCallArguments : public Transform { int argumentsToRemove; // -1 => all @@ -133,16 +168,34 @@ const IR::Node *DoRemoveActionParameters::postorder(IR::P4Action *action) { continue; } - if (p->direction == IR::Direction::In || p->direction == IR::Direction::InOut || - p->direction == IR::Direction::None) { + if (p->direction == IR::Direction::InOut) { + // For inout parameters, extract side-effecting array indices + // into temporaries to evaluate them once and avoid DAG sharing. + IR::IndexedVector tempDecls; + IR::IndexedVector tempAssigns; + ExtractArrayIndices eai(typeMap, *nameGen, tempDecls, tempAssigns); + eai.setCalledBy(this); + auto argExpr = arg->expression->apply(eai)->to(); + for (auto *d : tempDecls) result->push_back(d); + body.append(tempAssigns); + + auto left = new IR::PathExpression(p->name); + auto assign = new IR::AssignmentStatement(arg->srcInfo, left, argExpr); + body.push_back(assign); + + auto right = new IR::PathExpression(p->name); + auto assign2 = new IR::AssignmentStatement(arg->srcInfo, argExpr->clone(), right); + postamble.push_back(assign2); + } else if (p->direction == IR::Direction::In || p->direction == IR::Direction::None) { auto left = new IR::PathExpression(p->name); auto assign = new IR::AssignmentStatement(arg->srcInfo, left, arg->expression); body.push_back(assign); - } - if (p->direction == IR::Direction::Out || p->direction == IR::Direction::InOut) { + } else if (p->direction == IR::Direction::Out) { auto right = new IR::PathExpression(p->name); auto assign = new IR::AssignmentStatement(arg->srcInfo, arg->expression, right); postamble.push_back(assign); + } else { + BUG("unhandled direction %1%", directionToString(p->direction)); } } } @@ -197,8 +250,10 @@ RemoveActionParameters::RemoveActionParameters(TypeMap *typeMap, TypeChecking *t passes.emplace_back(new MoveDeclarations()); if (!typeChecking) typeChecking = new TypeChecking(nullptr, typeMap); passes.emplace_back(typeChecking); + auto nameGen = new MinimalNameGenerator(); passes.emplace_back(new FindActionParameters(typeMap, ai)); - passes.emplace_back(new DoRemoveActionParameters(ai)); + passes.emplace_back(nameGen); + passes.emplace_back(new DoRemoveActionParameters(ai, typeMap, nameGen)); passes.emplace_back(new ClearTypeMap(typeMap)); } diff --git a/frontends/p4/removeParameters.h b/frontends/p4/removeParameters.h index f3b781147fb..dc8310eba8f 100644 --- a/frontends/p4/removeParameters.h +++ b/frontends/p4/removeParameters.h @@ -114,10 +114,16 @@ class FindActionParameters : public Inspector, public ResolutionContext { */ class DoRemoveActionParameters : public Transform { ActionInvocation *invocations; + TypeMap *typeMap; + MinimalNameGenerator *nameGen; public: - explicit DoRemoveActionParameters(ActionInvocation *invocations) : invocations(invocations) { + DoRemoveActionParameters(ActionInvocation *invocations, TypeMap *typeMap, + MinimalNameGenerator *nameGen) + : invocations(invocations), typeMap(typeMap), nameGen(nameGen) { CHECK_NULL(invocations); + CHECK_NULL(typeMap); + CHECK_NULL(nameGen); setName("DoRemoveActionParameters"); } diff --git a/testdata/p4_16_samples/inline-function-to-array-index-chained.p4 b/testdata/p4_16_samples/inline-function-to-array-index-chained.p4 new file mode 100644 index 00000000000..a752d943850 --- /dev/null +++ b/testdata/p4_16_samples/inline-function-to-array-index-chained.p4 @@ -0,0 +1,38 @@ +#include + +bit<3> max(in bit<3> val, in bit<3> bound) { + return val < bound ? val : bound; +} + +header inner_t { + bit<8> f2; +} + +header outer_t { + bit<8> f1; +} + +struct mid_t { + inner_t[1] inner_arr; +} + +struct Headers { + outer_t[1] outer_arr; + mid_t[1] mid_arr; +} + +control proto(inout Headers h); +package top(proto _c); + +// Test: expr[i1].f1[i2].f2 — chained array indices with function calls and field accesses +control ingress(inout Headers h) { + action a(inout bit<8> val) { + val = 8w1; + } + table t { + actions = { a(h.mid_arr[max(3w0, 3w0)].inner_arr[max(3w0, 3w0)].f2); } + } + apply { t.apply(); } +} + +top(ingress()) main; diff --git a/testdata/p4_16_samples/inline-function-to-array-index.p4 b/testdata/p4_16_samples/inline-function-to-array-index.p4 new file mode 100644 index 00000000000..1edb08b613d --- /dev/null +++ b/testdata/p4_16_samples/inline-function-to-array-index.p4 @@ -0,0 +1,24 @@ +#include + +bit<3> max(in bit<3> val, in bit<3> bound) { + return val < bound ? val : bound; +} + +header h_t {} + +struct Headers { + h_t[1] hdr_arr; +} + +control proto(inout Headers h); +package top(proto _c); + +control ingress(inout Headers h) { + action a(inout h_t hdr) {} + table t { + actions = { a(h.hdr_arr[max(3w0, 3w0)]); } + } + apply { t.apply(); } +} + +top(ingress()) main; diff --git a/testdata/p4_16_samples_outputs/inline-function-to-array-index-chained-first.p4 b/testdata/p4_16_samples_outputs/inline-function-to-array-index-chained-first.p4 new file mode 100644 index 00000000000..9ec8af7bb07 --- /dev/null +++ b/testdata/p4_16_samples_outputs/inline-function-to-array-index-chained-first.p4 @@ -0,0 +1,41 @@ +#include + +bit<3> max(in bit<3> val, in bit<3> bound) { + return (val < bound ? val : bound); +} +header inner_t { + bit<8> f2; +} + +header outer_t { + bit<8> f1; +} + +struct mid_t { + inner_t[1] inner_arr; +} + +struct Headers { + outer_t[1] outer_arr; + mid_t[1] mid_arr; +} + +control proto(inout Headers h); +package top(proto _c); +control ingress(inout Headers h) { + action a(inout bit<8> val) { + val = 8w1; + } + table t { + actions = { + a(h.mid_arr[max(3w0, 3w0)].inner_arr[max(3w0, 3w0)].f2); + @defaultonly NoAction(); + } + default_action = NoAction(); + } + apply { + t.apply(); + } +} + +top(ingress()) main; diff --git a/testdata/p4_16_samples_outputs/inline-function-to-array-index-chained-frontend.p4 b/testdata/p4_16_samples_outputs/inline-function-to-array-index-chained-frontend.p4 new file mode 100644 index 00000000000..f0ad1db081c --- /dev/null +++ b/testdata/p4_16_samples_outputs/inline-function-to-array-index-chained-frontend.p4 @@ -0,0 +1,74 @@ +#include + +header inner_t { + bit<8> f2; +} + +header outer_t { + bit<8> f1; +} + +struct mid_t { + inner_t[1] inner_arr; +} + +struct Headers { + outer_t[1] outer_arr; + mid_t[1] mid_arr; +} + +control proto(inout Headers h); +package top(proto _c); +control ingress(inout Headers h) { + @name("ingress.val") bit<8> val; + @name("ingress.tmp_idx") bit<3> tmp_idx_1; + @name("ingress.tmp_idx_0") bit<3> tmp_idx_2; + @name("ingress.val_0") bit<3> val_3; + @name("ingress.bound_0") bit<3> bound; + @name("ingress.retval") bit<3> retval; + @name("ingress.tmp") bit<3> tmp; + @name("ingress.inlinedRetval") bit<3> inlinedRetval_1; + @name("ingress.val_2") bit<3> val_4; + @name("ingress.bound_1") bit<3> bound_2; + @name("ingress.retval") bit<3> retval_1; + @name("ingress.tmp") bit<3> tmp_1; + @name("ingress.inlinedRetval_0") bit<3> inlinedRetval_2; + @noWarn("unused") @name(".NoAction") action NoAction_1() { + } + @name("ingress.a") action a() { + val_3 = 3w0; + bound = 3w0; + if (val_3 < bound) { + tmp = val_3; + } else { + tmp = bound; + } + retval = tmp; + inlinedRetval_1 = retval; + tmp_idx_1 = inlinedRetval_1; + val_4 = 3w0; + bound_2 = 3w0; + if (val_4 < bound_2) { + tmp_1 = val_4; + } else { + tmp_1 = bound_2; + } + retval_1 = tmp_1; + inlinedRetval_2 = retval_1; + tmp_idx_2 = inlinedRetval_2; + val = 8w1; + h.mid_arr[tmp_idx_1].inner_arr[tmp_idx_2].f2 = val; + } + @name("ingress.t") table t_0 { + actions = { + a(); + @defaultonly NoAction_1(); + } + default_action = NoAction_1(); + } + apply { + t_0.apply(); + } +} + +top(ingress()) main; diff --git a/testdata/p4_16_samples_outputs/inline-function-to-array-index-chained-midend.p4 b/testdata/p4_16_samples_outputs/inline-function-to-array-index-chained-midend.p4 new file mode 100644 index 00000000000..2e1ef6cfecf --- /dev/null +++ b/testdata/p4_16_samples_outputs/inline-function-to-array-index-chained-midend.p4 @@ -0,0 +1,40 @@ +#include + +header inner_t { + bit<8> f2; +} + +header outer_t { + bit<8> f1; +} + +struct mid_t { + inner_t[1] inner_arr; +} + +struct Headers { + outer_t[1] outer_arr; + mid_t[1] mid_arr; +} + +control proto(inout Headers h); +package top(proto _c); +control ingress(inout Headers h) { + @noWarn("unused") @name(".NoAction") action NoAction_1() { + } + @name("ingress.a") action a() { + h.mid_arr[3w0].inner_arr[3w0].f2 = 8w1; + } + @name("ingress.t") table t_0 { + actions = { + a(); + @defaultonly NoAction_1(); + } + default_action = NoAction_1(); + } + apply { + t_0.apply(); + } +} + +top(ingress()) main; diff --git a/testdata/p4_16_samples_outputs/inline-function-to-array-index-chained.p4 b/testdata/p4_16_samples_outputs/inline-function-to-array-index-chained.p4 new file mode 100644 index 00000000000..9f577091663 --- /dev/null +++ b/testdata/p4_16_samples_outputs/inline-function-to-array-index-chained.p4 @@ -0,0 +1,39 @@ +#include + +bit<3> max(in bit<3> val, in bit<3> bound) { + return (val < bound ? val : bound); +} +header inner_t { + bit<8> f2; +} + +header outer_t { + bit<8> f1; +} + +struct mid_t { + inner_t[1] inner_arr; +} + +struct Headers { + outer_t[1] outer_arr; + mid_t[1] mid_arr; +} + +control proto(inout Headers h); +package top(proto _c); +control ingress(inout Headers h) { + action a(inout bit<8> val) { + val = 8w1; + } + table t { + actions = { + a(h.mid_arr[max(3w0, 3w0)].inner_arr[max(3w0, 3w0)].f2); + } + } + apply { + t.apply(); + } +} + +top(ingress()) main; diff --git a/testdata/p4_16_samples_outputs/inline-function-to-array-index-chained.p4-stderr b/testdata/p4_16_samples_outputs/inline-function-to-array-index-chained.p4-stderr new file mode 100644 index 00000000000..e69de29bb2d diff --git a/testdata/p4_16_samples_outputs/inline-function-to-array-index-first.p4 b/testdata/p4_16_samples_outputs/inline-function-to-array-index-first.p4 new file mode 100644 index 00000000000..2d5b392eccc --- /dev/null +++ b/testdata/p4_16_samples_outputs/inline-function-to-array-index-first.p4 @@ -0,0 +1,30 @@ +#include + +bit<3> max(in bit<3> val, in bit<3> bound) { + return (val < bound ? val : bound); +} +header h_t { +} + +struct Headers { + h_t[1] hdr_arr; +} + +control proto(inout Headers h); +package top(proto _c); +control ingress(inout Headers h) { + action a(inout h_t hdr) { + } + table t { + actions = { + a(h.hdr_arr[max(3w0, 3w0)]); + @defaultonly NoAction(); + } + default_action = NoAction(); + } + apply { + t.apply(); + } +} + +top(ingress()) main; diff --git a/testdata/p4_16_samples_outputs/inline-function-to-array-index-frontend.p4 b/testdata/p4_16_samples_outputs/inline-function-to-array-index-frontend.p4 new file mode 100644 index 00000000000..871b7e4196b --- /dev/null +++ b/testdata/p4_16_samples_outputs/inline-function-to-array-index-frontend.p4 @@ -0,0 +1,48 @@ +#include + +header h_t { +} + +struct Headers { + h_t[1] hdr_arr; +} + +control proto(inout Headers h); +package top(proto _c); +control ingress(inout Headers h) { + @name("ingress.hdr") h_t hdr_0; + @name("ingress.tmp_idx") bit<3> tmp_idx_0; + @name("ingress.val_0") bit<3> val; + @name("ingress.bound_0") bit<3> bound; + @name("ingress.retval") bit<3> retval; + @name("ingress.tmp") bit<3> tmp; + @name("ingress.inlinedRetval") bit<3> inlinedRetval_0; + @noWarn("unused") @name(".NoAction") action NoAction_1() { + } + @name("ingress.a") action a() { + val = 3w0; + bound = 3w0; + if (val < bound) { + tmp = val; + } else { + tmp = bound; + } + retval = tmp; + inlinedRetval_0 = retval; + tmp_idx_0 = inlinedRetval_0; + hdr_0 = h.hdr_arr[tmp_idx_0]; + h.hdr_arr[tmp_idx_0] = hdr_0; + } + @name("ingress.t") table t_0 { + actions = { + a(); + @defaultonly NoAction_1(); + } + default_action = NoAction_1(); + } + apply { + t_0.apply(); + } +} + +top(ingress()) main; diff --git a/testdata/p4_16_samples_outputs/inline-function-to-array-index-midend.p4 b/testdata/p4_16_samples_outputs/inline-function-to-array-index-midend.p4 new file mode 100644 index 00000000000..ed703b33fa5 --- /dev/null +++ b/testdata/p4_16_samples_outputs/inline-function-to-array-index-midend.p4 @@ -0,0 +1,29 @@ +#include + +header h_t { +} + +struct Headers { + h_t[1] hdr_arr; +} + +control proto(inout Headers h); +package top(proto _c); +control ingress(inout Headers h) { + @noWarn("unused") @name(".NoAction") action NoAction_1() { + } + @name("ingress.a") action a() { + } + @name("ingress.t") table t_0 { + actions = { + a(); + @defaultonly NoAction_1(); + } + default_action = NoAction_1(); + } + apply { + t_0.apply(); + } +} + +top(ingress()) main; diff --git a/testdata/p4_16_samples_outputs/inline-function-to-array-index.p4 b/testdata/p4_16_samples_outputs/inline-function-to-array-index.p4 new file mode 100644 index 00000000000..112e5537ab0 --- /dev/null +++ b/testdata/p4_16_samples_outputs/inline-function-to-array-index.p4 @@ -0,0 +1,28 @@ +#include + +bit<3> max(in bit<3> val, in bit<3> bound) { + return (val < bound ? val : bound); +} +header h_t { +} + +struct Headers { + h_t[1] hdr_arr; +} + +control proto(inout Headers h); +package top(proto _c); +control ingress(inout Headers h) { + action a(inout h_t hdr) { + } + table t { + actions = { + a(h.hdr_arr[max(3w0, 3w0)]); + } + } + apply { + t.apply(); + } +} + +top(ingress()) main; diff --git a/testdata/p4_16_samples_outputs/inline-function-to-array-index.p4-stderr b/testdata/p4_16_samples_outputs/inline-function-to-array-index.p4-stderr new file mode 100644 index 00000000000..f2d01357436 --- /dev/null +++ b/testdata/p4_16_samples_outputs/inline-function-to-array-index.p4-stderr @@ -0,0 +1,3 @@ +inline-function-to-array-index.p4(17): [--Wwarn=unused] warning: 'hdr' is unused + action a(inout h_t hdr) {} + ^^^