Better alias analysis in side-effects pass (#922)

Author: Mihai Budiu

frontends/p4/sideEffects.cpp

@@ -310,40 +310,205 @@ class DismantleExpression : public Transform {
     const IR::Node* preorder(IR::LAnd* expression) override { return shortCircuit(expression); }
     const IR::Node* preorder(IR::LOr* expression) override { return shortCircuit(expression); }
 
-    // We don't want to compute the full read/write set here so we
-    // overapproximate it as follows: all declarations that occur in
-    // an expression.
-    // TODO: this could be made more precise, perhaps using LocationSets.
-    class ReadsWrites : public Inspector {
+    /// This class represents the path to a location.
+    /// Given a struct S { bit a; bit b; } and a variable S x;
+    /// a path can be x.a, or just x.  An array index is represented as a
+    /// number (encoded as a string) or as "*", denoting an unknown index.
+    struct LocationPath : public IHasDbPrint {
+        const IR::IDeclaration* root;
+        std::vector<cstring> path;
+
+        explicit LocationPath(const IR::IDeclaration* root): root(root) { CHECK_NULL(root); }
+
+        const LocationPath* append(cstring suffix) const {
+            auto result = new LocationPath(root);
+            result->path = path;
+            result->path.push_back(suffix);
+            return result;
+        }
+
+        /// True if this path is a prefix of other or the other way around
+        bool isPrefix(const LocationPath* other) const {
+            // Due to the structure of the P4 language, two distinct
+            // declarations can never alias.
+            if (root != other->root)
+                return false;
+            size_t len = std::min(path.size(), other->path.size());
+            for (size_t i = 0; i < len; i++) {
+                if (path.at(i) == "*" || other->path.at(i) == "*")
+                    continue;
+                if (path.at(i) != other->path.at(i))
+                    return false;
+            }
+            return true;
+        }
+
+        void dbprint(std::ostream& out) const override {
+            out << root->getName();
+            for (auto p : path)
+                out << "." << p;
+        }
+    };
+
+    /// We represent a set of location set as a set of LocationPath
+    /// objects.
+    class SetOfLocations : public IHasDbPrint {
      public:
-        std::set<const IR::IDeclaration*> decls;
-        ReferenceMap* refMap;
+        std::set<const LocationPath*> paths;
+
+        SetOfLocations() = default;
+        explicit SetOfLocations(const LocationPath* path) {
+            add(path);
+        }
+        explicit SetOfLocations(const SetOfLocations* set): paths(set->paths) {}
+
+        void add(const LocationPath* path) { paths.emplace(path); }
+        bool overlaps(const SetOfLocations* other) const {
+            // Normally one of these sets has only one element, because
+            // one of the two is a left-value, so this should be fast.
+            for (auto s : paths) {
+                for (auto so : other->paths) {
+                    if (s->isPrefix(so))
+                        return true;
+                }
+            }
+            return false;
+        }
+
+        const SetOfLocations* join(const SetOfLocations* other) const {
+            auto result = new SetOfLocations(this);
+            for (auto p : other->paths)
+                result->add(p);
+            return result;
+        }
+
+        /// Append suffix to each location in the set
+        const SetOfLocations* append(cstring suffix) const {
+            auto result = new SetOfLocations();
+            for (auto p : paths) {
+                auto append = p->append(suffix);
+                result->add(append);
+            }
+            return result;
+        }
 
-        explicit ReadsWrites(ReferenceMap* refMap) : refMap(refMap)
+        void dbprint(std::ostream& out) const override {
+            for (auto p : paths)
+                out << p << std::endl;
+        }
+    };
+
+    /// Computes the SetOfLocations read and written by an expression.
+    /// This is invoked only for expressions that appear as arguments
+    /// to method calls.
+    class ReadsWrites : public Inspector {
+        const ReferenceMap* refMap;
+        std::map<const IR::Expression*, const SetOfLocations*> rw;
+
+     public:
+        explicit ReadsWrites(const ReferenceMap* refMap) : refMap(refMap)
         { setName("ReadsWrites"); }
 
+        void postorder(const IR::Operation_Binary* expression) override {
+            auto left = ::get(rw, expression->left);
+            auto right = ::get(rw, expression->right);
+            rw.emplace(expression, left->join(right));
+        }
+
         void postorder(const IR::PathExpression* expression) override {
             auto decl = refMap->getDeclaration(expression->path);
-            decls.emplace(decl);
+            auto path = new LocationPath(decl);
+            auto locs = new SetOfLocations(path);
+            rw.emplace(expression, locs);
+        }
+
+        void postorder(const IR::Operation_Unary* expression) override {
+            auto e = ::get(rw, expression->expr);
+            rw.emplace(expression, e);
+        }
+
+        void postorder(const IR::Member* expression) override {
+            auto e = ::get(rw, expression->expr);
+            auto result = e->append(expression->member);
+            rw.emplace(expression, result);
+        }
+
+        void postorder(const IR::ArrayIndex* expression) override {
+            auto e = ::get(rw, expression->left);
+            const SetOfLocations* result;
+            if (expression->right->is<IR::Constant>()) {
+                int index = expression->right->to<IR::Constant>()->asInt();
+                result = e->append(Util::toString(index));
+            } else {
+                result = e->append("*");
+            }
+            rw.emplace(expression, result);
+        }
+
+        void postorder(const IR::Literal* expression) override {
+            rw.emplace(expression, new SetOfLocations());
+        }
+
+        void postorder(const IR::TypeNameExpression* expression) override {
+            rw.emplace(expression, new SetOfLocations());
+        }
+
+        void postorder(const IR::Operation_Ternary* expression) override {
+            auto e0 = ::get(rw, expression->e0);
+            auto e1 = ::get(rw, expression->e1);
+            auto e2 = ::get(rw, expression->e2);
+            rw.emplace(expression, e0->join(e1)->join(e2));
+        }
+
+        void postorder(const IR::MethodCallExpression* expression) override {
+            // The only expression that can appear here is h.isValid();
+            // The ReadsWrites analysis is not called for other methods that
+            // have side-effects -- these are always copied into temporaries.
+            BUG_CHECK(expression->method->is<IR::Member>(),
+                      "%1%: expected isValid()", expression);
+            auto member = expression->method->to<IR::Member>();
+            BUG_CHECK(member->member == "isValid", "%1%: expected isValid()", expression);
+            auto obj = member->expr;
+            auto e = ::get(rw, obj);
+            rw.emplace(expression, e->append("$valid"));
+        }
+
+        void postorder(const IR::ConstructorCallExpression* expression) override {
+            const SetOfLocations* result = new SetOfLocations();
+            for (auto e : *expression->arguments) {
+                auto s = ::get(rw, e);
+                result = result->join(s);
+            }
+            rw.emplace(expression, result);
+        }
+
+        void postorder(const IR::ListExpression* expression) override {
+            const SetOfLocations* result = new SetOfLocations();
+            for (auto e : expression->components) {
+                auto s = ::get(rw, e);
+                result = result->join(s);
+            }
+            rw.emplace(expression, result);
+        }
+
+        const SetOfLocations* get(const IR::Expression* expression) {
+            expression->apply(*this);
+            auto result = ::get(rw, expression);
+            CHECK_NULL(result);
+            LOG3("SetOfLocations(" << expression << ")=" << result);
+            return result;
         }
     };
 
     // Conservative alias analysis.  We implement this here because this pass
     // runs early in the front end, before enough information is present (eg.
     // def-use information) to do a precise alias analysis.
     bool mayAlias(const IR::Expression* left, const IR::Expression* right) const {
-        ReadsWrites rwleft(refMap);
-        (void)left->apply(rwleft);
-        ReadsWrites rwright(refMap);
-        (void)right->apply(rwright);
-
-        for (auto d : rwleft.decls) {
-            if (rwright.decls.count(d) > 0) {
-                LOG3(dbp(d) << " accessed by both " << dbp(left) << " and " << dbp(right));
-                return true;
-            }
-        }
-        return false;
+        ReadsWrites rw(refMap);
+        auto llocs = rw.get(left);
+        auto rlocs = rw.get(right);
+        LOG3("Checking overlap between " << llocs << " and " << rlocs);
+        return llocs->overlaps(rlocs);
     }
 
     /// Returns true if type is a header or a struct containing a header.
@@ -430,6 +595,10 @@ class DismantleExpression : public Transform {
                         break;
                     if (!p1->hasOut() && !p2->hasOut())
                         continue;
+                    if (useTemporary.find(p1) != useTemporary.end())
+                        continue;
+                    if (useTemporary.find(p2) != useTemporary.end())
+                        continue;
                     auto arg2 = desc.substitution.lookup(p2);
                     if (mayAlias(arg1, arg2)) {
                         LOG3("Using temporary for " << dbp(mce) <<

midend/actionSynthesis.cpp

@@ -108,6 +108,8 @@ bool DoSynthesizeActions::mustMove(const IR::AssignmentStatement *assign) {
         if (!mi->is<ExternMethod>())
             return true;
         auto em = mi->to<ExternMethod>();
+        // TODO: this is wrong, there is no way a mid-end pass can depend on
+        // v1model.
         auto &v1model = P4V1::V1Model::instance;
         if (em->originalExternType->name.name == v1model.ck16.name)
             return false;

testdata/p4_14_samples_outputs/action_inline2-frontend.p4

@@ -27,7 +27,6 @@ parser ParserImpl(packet_in packet, out headers hdr, inout metadata meta, inout
 }
 
 control ingress(inout headers hdr, inout metadata meta, inout standard_metadata_t standard_metadata) {
-    bit<8> tmp;
     @name(".copy2") action copy2_0(inout bit<8> dest_0, bit<8> val_0) {
         dest_0 = val_0;
     }
@@ -38,9 +37,7 @@ control ingress(inout headers hdr, inout metadata meta, inout standard_metadata_
         copy_0(dest_2, val_2);
     }
     @name(".setb1") action setb1_0(bit<9> port) {
-        tmp = hdr.data.b1;
-        setbyte_0(tmp, hdr.data.b2);
-        hdr.data.b1 = tmp;
+        setbyte_0(hdr.data.b1, hdr.data.b2);
         standard_metadata.egress_spec = port;
     }
     @name(".noop") action noop_0() {

testdata/p4_14_samples_outputs/flowlet_switching-frontend.p4

@@ -117,21 +117,13 @@ 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) {
-    bit<14> tmp;
-    tuple<bit<32>, bit<32>, bit<8>, bit<16>, bit<16>, bit<16>> tmp_0;
-    bit<16> tmp_1;
-    bit<32> tmp_2;
-    bit<32> tmp_3;
-    bit<32> tmp_4;
     @name(".flowlet_id") register<bit<16>>(32w8192) flowlet_id_0;
     @name(".flowlet_lasttime") register<bit<32>>(32w8192) flowlet_lasttime_0;
     @name("._drop") action _drop_1() {
         mark_to_drop();
     }
     @name(".set_ecmp_select") action set_ecmp_select_0(bit<8> ecmp_base, bit<8> ecmp_count) {
-        tmp_0 = { hdr.ipv4.srcAddr, hdr.ipv4.dstAddr, hdr.ipv4.protocol, hdr.tcp.srcPort, hdr.tcp.dstPort, meta.ingress_metadata.flowlet_id };
-        hash<bit<14>, bit<10>, tuple<bit<32>, bit<32>, bit<8>, bit<16>, bit<16>, bit<16>>, bit<20>>(tmp, HashAlgorithm.crc16, (bit<10>)ecmp_base, tmp_0, (bit<20>)ecmp_count);
-        meta.ingress_metadata.ecmp_offset = tmp;
+        hash<bit<14>, bit<10>, tuple<bit<32>, bit<32>, bit<8>, bit<16>, bit<16>, bit<16>>, bit<20>>(meta.ingress_metadata.ecmp_offset, HashAlgorithm.crc16, (bit<10>)ecmp_base, { hdr.ipv4.srcAddr, hdr.ipv4.dstAddr, hdr.ipv4.protocol, hdr.tcp.srcPort, hdr.tcp.dstPort, meta.ingress_metadata.flowlet_id }, (bit<20>)ecmp_count);
     }
     @name(".set_nhop") action set_nhop_0(bit<32> nhop_ipv4, bit<9> port) {
         meta.ingress_metadata.nhop_ipv4 = nhop_ipv4;
@@ -140,13 +132,9 @@ control ingress(inout headers hdr, inout metadata meta, inout standard_metadata_
     }
     @name(".lookup_flowlet_map") action lookup_flowlet_map_0() {
         hash<bit<13>, bit<13>, tuple<bit<32>, bit<32>, bit<8>, bit<16>, bit<16>>, bit<26>>(meta.ingress_metadata.flowlet_map_index, HashAlgorithm.crc16, 13w0, { hdr.ipv4.srcAddr, hdr.ipv4.dstAddr, hdr.ipv4.protocol, hdr.tcp.srcPort, hdr.tcp.dstPort }, 26w13);
-        tmp_2 = (bit<32>)meta.ingress_metadata.flowlet_map_index;
-        flowlet_id_0.read(tmp_1, tmp_2);
-        meta.ingress_metadata.flowlet_id = tmp_1;
+        flowlet_id_0.read(meta.ingress_metadata.flowlet_id, (bit<32>)meta.ingress_metadata.flowlet_map_index);
         meta.ingress_metadata.flow_ipg = (bit<32>)meta.intrinsic_metadata.ingress_global_timestamp;
-        tmp_4 = (bit<32>)meta.ingress_metadata.flowlet_map_index;
-        flowlet_lasttime_0.read(tmp_3, tmp_4);
-        meta.ingress_metadata.flowlet_lasttime = tmp_3;
+        flowlet_lasttime_0.read(meta.ingress_metadata.flowlet_lasttime, (bit<32>)meta.ingress_metadata.flowlet_map_index);
         meta.ingress_metadata.flow_ipg = meta.ingress_metadata.flow_ipg - meta.ingress_metadata.flowlet_lasttime;
         flowlet_lasttime_0.write((bit<32>)meta.ingress_metadata.flowlet_map_index, (bit<32>)meta.intrinsic_metadata.ingress_global_timestamp);
     }
@@ -226,23 +214,23 @@ control DeparserImpl(packet_out packet, in headers hdr) {
 }
 
 control verifyChecksum(in headers hdr, inout metadata meta) {
-    bit<16> tmp_5;
-    bool tmp_6;
+    bit<16> tmp;
+    bool tmp_0;
     @name("ipv4_checksum") Checksum16() ipv4_checksum_0;
     apply {
-        tmp_5 = ipv4_checksum_0.get<tuple<bit<4>, bit<4>, bit<8>, bit<16>, bit<16>, bit<3>, bit<13>, bit<8>, bit<8>, bit<32>, bit<32>>>({ hdr.ipv4.version, hdr.ipv4.ihl, hdr.ipv4.diffserv, hdr.ipv4.totalLen, hdr.ipv4.identification, hdr.ipv4.flags, hdr.ipv4.fragOffset, hdr.ipv4.ttl, hdr.ipv4.protocol, hdr.ipv4.srcAddr, hdr.ipv4.dstAddr });
-        tmp_6 = hdr.ipv4.hdrChecksum == tmp_5;
-        if (tmp_6) 
+        tmp = ipv4_checksum_0.get<tuple<bit<4>, bit<4>, bit<8>, bit<16>, bit<16>, bit<3>, bit<13>, bit<8>, bit<8>, bit<32>, bit<32>>>({ hdr.ipv4.version, hdr.ipv4.ihl, hdr.ipv4.diffserv, hdr.ipv4.totalLen, hdr.ipv4.identification, hdr.ipv4.flags, hdr.ipv4.fragOffset, hdr.ipv4.ttl, hdr.ipv4.protocol, hdr.ipv4.srcAddr, hdr.ipv4.dstAddr });
+        tmp_0 = hdr.ipv4.hdrChecksum == tmp;
+        if (tmp_0) 
             mark_to_drop();
     }
 }
 
 control computeChecksum(inout headers hdr, inout metadata meta) {
-    bit<16> tmp_7;
+    bit<16> tmp_1;
     @name("ipv4_checksum") Checksum16() ipv4_checksum_1;
     apply {
-        tmp_7 = ipv4_checksum_1.get<tuple<bit<4>, bit<4>, bit<8>, bit<16>, bit<16>, bit<3>, bit<13>, bit<8>, bit<8>, bit<32>, bit<32>>>({ hdr.ipv4.version, hdr.ipv4.ihl, hdr.ipv4.diffserv, hdr.ipv4.totalLen, hdr.ipv4.identification, hdr.ipv4.flags, hdr.ipv4.fragOffset, hdr.ipv4.ttl, hdr.ipv4.protocol, hdr.ipv4.srcAddr, hdr.ipv4.dstAddr });
-        hdr.ipv4.hdrChecksum = tmp_7;
+        tmp_1 = ipv4_checksum_1.get<tuple<bit<4>, bit<4>, bit<8>, bit<16>, bit<16>, bit<3>, bit<13>, bit<8>, bit<8>, bit<32>, bit<32>>>({ hdr.ipv4.version, hdr.ipv4.ihl, hdr.ipv4.diffserv, hdr.ipv4.totalLen, hdr.ipv4.identification, hdr.ipv4.flags, hdr.ipv4.fragOffset, hdr.ipv4.ttl, hdr.ipv4.protocol, hdr.ipv4.srcAddr, hdr.ipv4.dstAddr });
+        hdr.ipv4.hdrChecksum = tmp_1;
     }
 }