Correctly handle don't care named arguments (p4lang#3278)

* Correctly handle don't care named arguments * def-use analysis requires all statements (including Empty) not to be shared the IR tree Signed-off-by: Mihai Budiu <[email protected]>
github-sajan · May 26, 2022 · 627ca2f · 627ca2f
1 parent 1c03d50
commit 627ca2f
Show file tree

Hide file tree

Showing 42 changed files with 489 additions and 66 deletions.
diff --git a/frontends/p4/def_use.h b/frontends/p4/def_use.h
@@ -428,7 +428,7 @@ class AllDefinitions : public IHasDbPrint {
             if (it != atPoint.end()) {
                 LOG2("Overwriting definitions at " << point << ": " <<
                      it->second << " with " << defs);
-                BUG_CHECK(false, "Overwriting definitions");
+                BUG_CHECK(false, "Overwriting definitions at %1%", point);
             }
         }
         atPoint[point] = defs;
@@ -489,6 +489,9 @@ class ComputeWriteSet : public Inspector, public IHasDbPrint {
     }
     void expressionWrites(const IR::Expression* expression, const LocationSet* loc) {
         CHECK_NULL(expression); CHECK_NULL(loc);
+        LOG3(expression << dbp(expression) << " writes " << loc);
+        BUG_CHECK(writes.find(expression) == writes.end() || expression->is<IR::Literal>(),
+                  "Expression %1% write set already set", expression);
         writes.emplace(expression, loc);
     }
     void dbprint(std::ostream& out) const override {

diff --git a/frontends/p4/dontcareArgs.h b/frontends/p4/dontcareArgs.h
@@ -28,10 +28,20 @@ class DontcareArgs : public Transform {
     ReferenceMap* refMap;
     TypeMap* typeMap;
     IR::IndexedVector<IR::Declaration> toAdd;
+
  public:
     DontcareArgs(ReferenceMap* refMap, TypeMap* typeMap): refMap(refMap), typeMap(typeMap)
     { CHECK_NULL(refMap); CHECK_NULL(typeMap); setName("DontcareArgs"); }
     const IR::Node* postorder(IR::MethodCallExpression* expression) override;
+    const IR::Node* postorder(IR::Function* function) override {
+        IR::IndexedVector<IR::StatOrDecl> body;
+        for (auto d : toAdd)
+            body.push_back(d);
+        body.append(function->body->components);
+        function->body = new IR::BlockStatement(function->body->srcInfo, body);
+        toAdd.clear();
+        return function;
+    }
     const IR::Node* postorder(IR::P4Parser* parser) override {
         toAdd.append(parser->parserLocals);
         parser->parserLocals = toAdd;

diff --git a/frontends/p4/sideEffects.h b/frontends/p4/sideEffects.h
@@ -113,18 +113,8 @@ class SideEffects : public Inspector {
                               TypeMap* typeMap) {
         // mce does not produce a side effect in few cases:
         //  * isValid()
-        //  * function with all in parameters
-        //  * extern function with noSideEffectsAnnotation
-        //  * extern method with noSideEffectsAnnotation
+        //  * function, extern function, or extern method with noSideEffects annotation
         auto mi = MethodInstance::resolve(mce, refMap, typeMap);
-        if (mi->is<FunctionCall>()) {
-            for (auto p : *mi->substitution.getParametersInArgumentOrder()) {
-                if (p->hasOut()) {
-                    return true;
-                }
-            }
-            return false;
-        }
         if (auto em = mi->to<P4::ExternMethod>()) {
             if (em->method->getAnnotation(IR::Annotation::noSideEffectsAnnotation)) {
                 return false;
@@ -137,6 +127,12 @@ class SideEffects : public Inspector {
             }
             return true;
         }
+        if (auto ef = mi->to<P4::FunctionCall>()) {
+            if (ef->function->getAnnotation(IR::Annotation::noSideEffectsAnnotation)) {
+                return false;
+            }
+            return true;
+        }
         if (auto bim = mi->to<BuiltInMethod>()) {
             if (bim->name.name == IR::Type_Header::isValid) {
                 return false;

diff --git a/frontends/p4/simplifyDefUse.cpp b/frontends/p4/simplifyDefUse.cpp
@@ -470,7 +470,8 @@ class FindUninitialized : public Inspector {
     void checkOutParameters(const IR::IDeclaration* block,
                             const IR::ParameterList* parameters,
                             Definitions* defs) {
-        LOG2("Checking output parameters; definitions are " << IndentCtl::endl << defs);
+        LOG2("Checking output parameters of " << block <<
+             "; definitions are " << IndentCtl::endl << defs);
         for (auto p : parameters->parameters) {
             if (p->direction == IR::Direction::Out || p->direction == IR::Direction::InOut) {
                 auto storage = definitions->storageMap->getStorage(p);
@@ -1463,7 +1464,7 @@ class RemoveUnused : public Transform {
                 return new IR::MethodCallStatement(statement->srcInfo, mce);
             }
             // removing
-            return new IR::EmptyStatement();
+            return new IR::EmptyStatement(statement->srcInfo);
         }
         return statement;
     }
@@ -1473,7 +1474,8 @@ class RemoveUnused : public Transform {
                 return mcs;
             }
             // removing
-            return new IR::EmptyStatement();
+            LOG3("Removing statement " << getOriginal() << IndentCtl::indent);
+            return new IR::EmptyStatement(mcs->srcInfo);
         }
         return mcs;
     }

diff --git a/frontends/p4/simplifyDefUse.h b/frontends/p4/simplifyDefUse.h
@@ -19,6 +19,7 @@ limitations under the License.
 
 #include "ir/ir.h"
 #include "frontends/p4/typeChecking/typeChecker.h"
+#include "frontends/p4/cloner.h"
 
 namespace P4 {
 
@@ -47,15 +48,36 @@ class DoSimplifyDefUse : public Transform {
     { return process(control); }
 };
 
-class SimplifyDefUse : public PassRepeated {
+class SimplifyDefUse : public PassManager {
+    class Cloner : public ClonePathExpressions {
+     public:
+        Cloner() { setName("Cloner"); }
+        const IR::Node* postorder(IR::EmptyStatement* stat) override {
+            // You cannot clone an empty statement, since
+            // the visitor claims it's equal to the original one.
+            // So we cheat and make an empty block.
+            return new IR::BlockStatement(stat->srcInfo);
+        }
+    };
+
+
  public:
     SimplifyDefUse(ReferenceMap* refMap, TypeMap* typeMap,
-             TypeChecking* typeChecking = nullptr) : PassRepeated({}) {
+             TypeChecking* typeChecking = nullptr) {
         CHECK_NULL(refMap); CHECK_NULL(typeMap);
+
+        // SimplifyDefUse needs the expression tree *not* to be a DAG,
+        // because it keeps state in hash-maps indexed with PathExpressions.
+        // This is achieved by Cloner.
+        passes.push_back(new Cloner());
         if (!typeChecking)
             typeChecking = new TypeChecking(refMap, typeMap);
-        passes.push_back(typeChecking);
-        passes.push_back(new DoSimplifyDefUse(refMap, typeMap));
+
+        auto repeated = new PassRepeated({
+                typeChecking,
+                new DoSimplifyDefUse(refMap, typeMap)
+            });
+        passes.push_back(repeated);
         setName("SimplifyDefUse");
     }
 };

diff --git a/frontends/p4/simplifySwitch.cpp b/frontends/p4/simplifySwitch.cpp
@@ -53,7 +53,7 @@ const IR::Node* DoSimplifySwitch::postorder(IR::SwitchStatement* stat) {
     }
     // If none of the labels matches none of the cases will be
     // executed.
-    return new IR::EmptyStatement();
+    return new IR::EmptyStatement(stat->srcInfo);
 }
 
 }  // namespace P4
diff --git a/frontends/parsers/p4/p4parser.ypp b/frontends/parsers/p4/p4parser.ypp
@@ -1369,6 +1369,7 @@ argument
     : expression                         { $$ = new IR::Argument(@1, $1); }
     | name "=" expression                { $$ = new IR::Argument(@1, *$1, $3); }
     | "_"                                { $$ = new IR::Argument(@1, new IR::DefaultExpression(@1)); }
+    | name "=" "_"                       { $$ = new IR::Argument(@1, *$1, new IR::DefaultExpression(@3)); }
     ;
 
 expressionList

diff --git a/midend/global_copyprop.cpp b/midend/global_copyprop.cpp
@@ -322,7 +322,7 @@ const IR::Node *DoGlobalCopyPropagation::preorder(IR::AssignmentStatement *stat)
         if (stat->left->is<IR::Slice>())
             return stat;
         if ((*vars)[lvalue_name(stat->left)] && lit->equiv(*((*vars)[lvalue_name(stat->left)])))
-            return new IR::EmptyStatement();
+            return new IR::EmptyStatement(stat->srcInfo);
         (*vars)[lvalue_name(stat->left)] = lit;
         LOG5("  Setting value: " << lit << ", for: " << stat->left);
     }

diff --git a/midend/local_copyprop.cpp b/midend/local_copyprop.cpp
@@ -97,7 +97,7 @@ class DoLocalCopyPropagation::ElimDead : public Transform {
                 if (!self.hasSideEffects(s->condition)) {
                     return nullptr;
                 } else {
-                    s->ifTrue = new IR::EmptyStatement();
+                    s->ifTrue = new IR::EmptyStatement(s->srcInfo);
                     return s;
                 }
             }

diff --git a/testdata/p4_14_samples_outputs/modify_conditionally-frontend.p4 b/testdata/p4_14_samples_outputs/modify_conditionally-frontend.p4
@@ -36,10 +36,17 @@ control egress(inout headers hdr, inout metadata meta, inout standard_metadata_t
 }
 
 control ingress(inout headers hdr, inout metadata meta, inout standard_metadata_t standard_metadata) {
+    @name("ingress.tmp") bit<8> tmp;
     @noWarn("unused") @name(".NoAction") action NoAction_1() {
     }
     @name(".table0_actionlist") action table0_actionlist(@name("do_goto_table") bit<1> do_goto_table_1, @name("goto_table_id") bit<8> goto_table_id_1) {
         meta.metadata_global.do_goto_table = do_goto_table_1;
+        if (do_goto_table_1 != 1w0) {
+            tmp = goto_table_id_1;
+        } else {
+            tmp = meta.metadata_global.goto_table_id;
+        }
+        meta.metadata_global.goto_table_id = tmp;
     }
     @name(".table0") table table0_0 {
         actions = {

diff --git a/testdata/p4_14_samples_outputs/modify_conditionally-midend.p4 b/testdata/p4_14_samples_outputs/modify_conditionally-midend.p4
@@ -40,6 +40,7 @@ control ingress(inout headers hdr, inout metadata meta, inout standard_metadata_
     }
     @name(".table0_actionlist") action table0_actionlist(@name("do_goto_table") bit<1> do_goto_table_1, @name("goto_table_id") bit<8> goto_table_id_1) {
         meta._metadata_global_do_goto_table0 = do_goto_table_1;
+        meta._metadata_global_goto_table_id1 = (do_goto_table_1 != 1w0 ? goto_table_id_1 : meta._metadata_global_goto_table_id1);
     }
     @name(".table0") table table0_0 {
         actions = {

diff --git a/testdata/p4_16_samples/issue3274-1.p4 b/testdata/p4_16_samples/issue3274-1.p4
@@ -0,0 +1,12 @@
+void x() {}
+
+control c() {
+    apply {
+        x();
+    }
+}
+
+control _c();
+package top(_c _c);
+
+top(c()) main;
diff --git a/testdata/p4_16_samples/issue3274-2.p4 b/testdata/p4_16_samples/issue3274-2.p4
@@ -0,0 +1,16 @@
+extern bit<32> f(in bit<32> x, out bit<16> y);
+
+void x() {
+    f(x = 1, y = _);
+}
+
+control c() {
+    apply {
+        x();
+    }
+}
+
+control _c();
+package top(_c _c);
+
+top(c()) main;
diff --git a/testdata/p4_16_samples/issue3274.p4 b/testdata/p4_16_samples/issue3274.p4
@@ -0,0 +1,16 @@
+extern bit<32> f(in bit<32> x, out bit<16> y);
+
+bit<32> x() {
+    return f(x = 1, y = _);
+}
+
+control c(out bit<32> r) {
+    apply {
+        r = x();
+    }
+}
+
+control _c(out bit<32> r);
+package top(_c _c);
+
+top(c()) main;
diff --git a/testdata/p4_16_samples_outputs/issue1781-bmv2-frontend.p4 b/testdata/p4_16_samples_outputs/issue1781-bmv2-frontend.p4
@@ -19,6 +19,8 @@ control IngressImpl(inout headers hdr, inout metadata meta, inout standard_metad
     @name("IngressImpl.value") bit<32> value_3;
     @name("IngressImpl.hasReturned") bool hasReturned;
     @name("IngressImpl.retval") bit<32> retval;
+    @name("IngressImpl.hasReturned") bool hasReturned_1;
+    @name("IngressImpl.retval") bit<32> retval_1;
     @name("IngressImpl.update_value") action update_value() {
         hasReturned = false;
         hasReturned = true;
@@ -27,6 +29,9 @@ control IngressImpl(inout headers hdr, inout metadata meta, inout standard_metad
         value_1 = value_3;
     }
     apply {
+        hasReturned_1 = false;
+        hasReturned_1 = true;
+        retval_1 = 32w1;
         update_value();
     }
 }

diff --git a/testdata/p4_16_samples_outputs/issue2148-frontend.p4 b/testdata/p4_16_samples_outputs/issue2148-frontend.p4
@@ -18,9 +18,11 @@ control ingress(inout Headers h, inout Meta m, inout standard_metadata_t sm) {
     @name("ingress.retval") bit<16> retval;
     @name("ingress.not_initialized") H not_initialized_0;
     @name("ingress.new_val") bit<32> new_val_0;
+    @name("ingress.hasReturned") bool hasReturned_1;
+    @name("ingress.retval") bit<16> retval_1;
+    @name("ingress.not_initialized") H not_initialized_1;
+    @name("ingress.new_val") bit<32> new_val_1;
     @name("ingress.do_thing_action") action do_thing_action() {
-    }
-    apply {
         hasReturned = false;
         not_initialized_0.setInvalid();
         new_val_0 = 32w1;
@@ -31,7 +33,19 @@ control ingress(inout Headers h, inout Meta m, inout standard_metadata_t sm) {
         }
         hasReturned = true;
         retval = (bit<16>)new_val_0;
-        h.h.a = retval;
+    }
+    apply {
+        hasReturned_1 = false;
+        not_initialized_1.setInvalid();
+        new_val_1 = 32w1;
+        if (not_initialized_1.a < 16w6) {
+            ;
+        } else {
+            new_val_1 = 32w232;
+        }
+        hasReturned_1 = true;
+        retval_1 = (bit<16>)new_val_1;
+        h.h.a = retval_1;
         do_thing_action();
     }
 }

diff --git a/testdata/p4_16_samples_outputs/issue2148-midend.p4 b/testdata/p4_16_samples_outputs/issue2148-midend.p4
@@ -15,18 +15,20 @@ struct Meta {
 
 control ingress(inout Headers h, inout Meta m, inout standard_metadata_t sm) {
     @name("ingress.not_initialized") H not_initialized_0;
-    @name("ingress.new_val") bit<32> new_val_0;
+    @name("ingress.not_initialized") H not_initialized_1;
+    @name("ingress.new_val") bit<32> new_val_1;
     @name("ingress.do_thing_action") action do_thing_action() {
+        not_initialized_0.setInvalid();
     }
     @hidden action issue2148l21() {
-        new_val_0 = 32w232;
+        new_val_1 = 32w232;
     }
     @hidden action issue2148l17() {
-        not_initialized_0.setInvalid();
-        new_val_0 = 32w1;
+        not_initialized_1.setInvalid();
+        new_val_1 = 32w1;
     }
     @hidden action issue2148l30() {
-        h.h.a = (bit<16>)new_val_0;
+        h.h.a = (bit<16>)new_val_1;
     }
     @hidden table tbl_issue2148l17 {
         actions = {
@@ -54,7 +56,7 @@ control ingress(inout Headers h, inout Meta m, inout standard_metadata_t sm) {
     }
     apply {
         tbl_issue2148l17.apply();
-        if (not_initialized_0.a < 16w6) {
+        if (not_initialized_1.a < 16w6) {
             ;
         } else {
             tbl_issue2148l21.apply();

diff --git a/testdata/p4_16_samples_outputs/issue2260-2-frontend.p4 b/testdata/p4_16_samples_outputs/issue2260-2-frontend.p4
@@ -1,7 +1,12 @@
 control C();
 package S(C c);
 control MyC() {
+    @name("MyC.hasReturned") bool hasReturned;
+    @name("MyC.retval") bit<8> retval;
     apply {
+        hasReturned = false;
+        hasReturned = true;
+        retval = 8w255;
     }
 }
 

diff --git a/testdata/p4_16_samples_outputs/issue2289-frontend.p4 b/testdata/p4_16_samples_outputs/issue2289-frontend.p4
@@ -31,11 +31,35 @@ parser p(packet_in pkt, out Headers hdr, inout Meta m, inout standard_metadata_t
 }
 
 control ingress(inout Headers h, inout Meta m, inout standard_metadata_t sm) {
+    @name("ingress.tmp_0") bit<16> tmp;
     @name("ingress.byaA") bit<16> byaA_0;
+    @name("ingress.hasReturned") bool hasReturned;
+    @name("ingress.retval") bit<16> retval;
+    @name("ingress.hasReturned_0") bool hasReturned_0;
+    @name("ingress.retval_0") bit<16> retval_0;
+    @name("ingress.tmp") bit<16> tmp_0;
+    @name("ingress.hasReturned") bool hasReturned_3;
+    @name("ingress.retval") bit<16> retval_3;
+    @name("ingress.hasReturned") bool hasReturned_4;
+    @name("ingress.retval") bit<16> retval_4;
     @name("ingress.simple_action") action simple_action() {
+        hasReturned = false;
+        hasReturned = true;
+        retval = 16w1;
+        tmp = retval;
+        hasReturned_0 = false;
+        hasReturned_3 = false;
+        hasReturned_3 = true;
+        retval_3 = 16w1;
+        tmp_0 = retval_3;
+        hasReturned_0 = true;
+        retval_0 = tmp_0;
         h.eth_hdr.eth_type = byaA_0;
     }
     apply {
+        hasReturned_4 = false;
+        hasReturned_4 = true;
+        retval_4 = 16w1;
         simple_action();
     }
 }