Updates to RemoveParameters

Signed-off-by: Mathew Suresh Zachariah <mazachar@pensando.io>

Author: Mathew Suresh Zachariah

backends/tofino/bf-p4c/midend/remove_action_params.h

@@ -35,8 +35,8 @@ namespace BFN {
  */
 class DoRemoveActionParametersTofino : public P4::DoRemoveActionParameters {
  public:
-    explicit DoRemoveActionParametersTofino(P4::ActionInvocation *inv)
-        : P4::DoRemoveActionParameters(inv) {}
+    explicit DoRemoveActionParametersTofino(P4::ActionInvocation *inv, P4::TypeMap *typeMap)
+        : P4::DoRemoveActionParameters(inv, typeMap) {}
 
     /**
      * Check if there is a mark_to_drop action and don't replace parameters.
@@ -187,7 +187,7 @@ class RemoveActionParameters : public PassManager {
         if (!tc) tc = new P4::TypeChecking(refMap, typeMap);
         passes.emplace_back(tc);
         passes.emplace_back(new P4::FindActionParameters(typeMap, ai));
-        passes.emplace_back(new DoRemoveActionParametersTofino(ai));
+        passes.emplace_back(new DoRemoveActionParametersTofino(ai, typeMap));
         passes.emplace_back(new P4::ClearTypeMap(typeMap));
     }
 };

frontends/p4/functionsInlining.cpp

@@ -155,9 +155,7 @@ const IR::Node *FunctionsInliner::preorder(IR::MethodCallExpression *expr) {
     auto it = pendingReplacements.find(orig);
     if (it != pendingReplacements.end()) {
         LOG2("Performing pending replacement of " << dbp(expr) << " with " << dbp(it->second));
-        auto result = it->second;
-        pendingReplacements.erase(it);
-        return result;
+        return it->second;
     }
 
     return expr;

frontends/p4/functionsInlining.h

@@ -76,7 +76,7 @@ class FunctionsInliner : public AbstractInliner<FunctionsInlineList, FunctionsIn
     class isLocalExpression;  // functor to test actual arguments scope use
 
  public:
-    FunctionsInliner() { visitDagOnce = false; }
+    FunctionsInliner() = default;
     Visitor::profile_t init_apply(const IR::Node *node) override;
     void end_apply(const IR::Node *node) override;
     const IR::Node *preorder(IR::Function *function) override;

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<IR::Declaration> &tempDecls;
+    IR::IndexedVector<IR::StatOrDecl> &tempAssigns;
+
+ public:
+    ExtractArrayIndices(TypeMap *typeMap, MinimalNameGenerator &nameGen,
+                        IR::IndexedVector<IR::Declaration> &tempDecls,
+                        IR::IndexedVector<IR::StatOrDecl> &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
@@ -117,6 +152,8 @@ const IR::Node *DoRemoveActionParameters::postorder(IR::P4Action *action) {
     ParameterSubstitution substitution;
     substitution.populate(action->parameters, args);
 
+    MinimalNameGenerator nameGen(action);
+
     bool removeAll = invocations->removeAllParameters(getOriginal<IR::P4Action>());
     for (auto p : action->parameters->parameters) {
         if (p->direction == IR::Direction::None && !removeAll) {
@@ -133,16 +170,35 @@ 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<IR::Declaration> tempDecls;
+                IR::IndexedVector<IR::StatOrDecl> tempAssigns;
+                ExtractArrayIndices eai(typeMap, nameGen, tempDecls, tempAssigns);
+                eai.setCalledBy(this);
+                auto argExpr = arg->expression->apply(eai)->to<IR::Expression>();
+                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, arg->expression);
+                auto assign = new IR::AssignmentStatement(arg->srcInfo, left, argExpr);
                 body.push_back(assign);
-            }
-            if (p->direction == IR::Direction::Out || p->direction == IR::Direction::InOut) {
+
                 auto right = new IR::PathExpression(p->name);
-                auto assign = new IR::AssignmentStatement(arg->srcInfo, arg->expression, right);
-                postamble.push_back(assign);
+                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) {
+                    auto right = new IR::PathExpression(p->name);
+                    auto assign = new IR::AssignmentStatement(arg->srcInfo, arg->expression, right);
+                    postamble.push_back(assign);
+                }
             }
         }
     }
@@ -198,7 +254,7 @@ RemoveActionParameters::RemoveActionParameters(TypeMap *typeMap, TypeChecking *t
     if (!typeChecking) typeChecking = new TypeChecking(nullptr, typeMap);
     passes.emplace_back(typeChecking);
     passes.emplace_back(new FindActionParameters(typeMap, ai));
-    passes.emplace_back(new DoRemoveActionParameters(ai));
+    passes.emplace_back(new DoRemoveActionParameters(ai, typeMap));
     passes.emplace_back(new ClearTypeMap(typeMap));
 }
 

frontends/p4/removeParameters.h

@@ -114,10 +114,13 @@ class FindActionParameters : public Inspector, public ResolutionContext {
  */
 class DoRemoveActionParameters : public Transform {
     ActionInvocation *invocations;
+    TypeMap *typeMap;
 
  public:
-    explicit DoRemoveActionParameters(ActionInvocation *invocations) : invocations(invocations) {
+    DoRemoveActionParameters(ActionInvocation *invocations, TypeMap *typeMap)
+        : invocations(invocations), typeMap(typeMap) {
         CHECK_NULL(invocations);
+        CHECK_NULL(typeMap);
         setName("DoRemoveActionParameters");
     }
 

testdata/p4_16_samples/inline-function-to-array-index-chained.p4

@@ -0,0 +1,38 @@
+#include <core.p4>
+
+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;

testdata/p4_16_samples_outputs/inline-function-to-array-index-chained-first.p4

@@ -0,0 +1,41 @@
+#include <core.p4>
+
+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;

testdata/p4_16_samples_outputs/inline-function-to-array-index-chained-frontend.p4

@@ -0,0 +1,74 @@
+#include <core.p4>
+
+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;

testdata/p4_16_samples_outputs/inline-function-to-array-index-chained-midend.p4

@@ -0,0 +1,40 @@
+#include <core.p4>
+
+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;