Fix loop unrolling bugs related to issue4739 (p4lang#4783)

* Fix loop unrolling bugs related to issue4739 - fix FindUninitialized in simplifyDefUse.cpp to correctly get live state for for loop bodies, so as to not dead-code eliminate things unused after the loop exit - fix midend/def_use.cpp ComputeDefUse to visit assignments right then left to get correct info for assignments that read the old value of the lhs - fix loopUnrolling.cpp UnrollLoops to not unroll loops where the index is modified in the loop body in addition to the loop increment. - more logging in ComputeDefUse to see help debugging Signed-off-by: Chris Dodd <[email protected]> * XFAILS for testgen for new test, as it barfs on loops Signed-off-by: Chris Dodd <[email protected]> * Rearrange and simplify the code for determining if a loop is unrollable - just need to look at def-use chains for the index var, so actually will unroll a few more cases. Signed-off-by: Chris Dodd <[email protected]> --------- Signed-off-by: Chris Dodd <[email protected]>
komaljai · Jul 7, 2024 · 5fda7d2 · 5fda7d2
1 parent a305732
commit 5fda7d2
Show file tree

Hide file tree

Showing 17 changed files with 515 additions and 70 deletions.
diff --git a/backends/p4tools/modules/testgen/targets/bmv2/test/BMV2MetadataXfail.cmake b/backends/p4tools/modules/testgen/targets/bmv2/test/BMV2MetadataXfail.cmake
@@ -57,6 +57,12 @@ p4tools_add_xfail_reason(
   # and more specifically when member is passed to getTypeArray
 )
 
+p4tools_add_xfail_reason(
+  "testgen-p4c-bmv2-metadata"
+  "Unhandled node type in Bmv2V1ModelCmdStepper: ForStatement"
+  issue4739.p4
+)
+
 ####################################################################################################
 # 3. WONTFIX
 ####################################################################################################

diff --git a/backends/p4tools/modules/testgen/targets/bmv2/test/BMV2PTFXfail.cmake b/backends/p4tools/modules/testgen/targets/bmv2/test/BMV2PTFXfail.cmake
@@ -101,6 +101,12 @@ p4tools_add_xfail_reason(
   issue907-bmv2.p4
 )
 
+p4tools_add_xfail_reason(
+  "testgen-p4c-bmv2-ptf"
+  "Unhandled node type in Bmv2V1ModelCmdStepper: ForStatement"
+  issue4739.p4
+)
+
 ####################################################################################################
 # 3. WONTFIX
 # These are failures that can not be solved by changing P4Testgen

diff --git a/backends/p4tools/modules/testgen/targets/bmv2/test/BMV2ProtobufIrXfail.cmake b/backends/p4tools/modules/testgen/targets/bmv2/test/BMV2ProtobufIrXfail.cmake
@@ -74,6 +74,12 @@ p4tools_add_xfail_reason(
   parser-unroll-test10.p4
 )
 
+p4tools_add_xfail_reason(
+  "testgen-p4c-bmv2-protobuf-ir"
+  "Unhandled node type in Bmv2V1ModelCmdStepper: ForStatement"
+  issue4739.p4
+)
+
 ####################################################################################################
 # 3. WONTFIX
 ####################################################################################################

diff --git a/backends/p4tools/modules/testgen/targets/bmv2/test/BMV2STFXfail.cmake b/backends/p4tools/modules/testgen/targets/bmv2/test/BMV2STFXfail.cmake
@@ -101,6 +101,12 @@ p4tools_add_xfail_reason(
   issue907-bmv2.p4
 )
 
+p4tools_add_xfail_reason(
+  "testgen-p4c-bmv2-stf"
+  "Unhandled node type in Bmv2V1ModelCmdStepper: ForStatement"
+  issue4739.p4
+)
+
 ####################################################################################################
 # 3. WONTFIX
 # These are failures that can not be solved by changing P4Testgen

diff --git a/frontends/p4/simplifyDefUse.cpp b/frontends/p4/simplifyDefUse.cpp
@@ -948,6 +948,22 @@ class FindUninitialized : public Inspector {
         return setCurrent(statement);
     }
 
+    bool preorder(const IR::ForStatement *statement) override {
+        Log::TempIndent indent;
+        LOG3("FU Visiting " << dbp(statement) << " " << statement << indent);
+        if (!unreachable) {
+            visit(statement->init, "init");
+            // use the live state from the end of the loop, as that jumps to the condition
+            setCurrent(statement);
+            visit(statement->condition, "condition");
+            visit(statement->body, "body");
+            visit(statement->updates, "updates");
+        } else {
+            LOG3("Unreachable");
+        }
+        return setCurrent(statement);
+    }
+
     bool preorder(const IR::ForInStatement *statement) override {
         Log::TempIndent indent;
         LOG3("FU Visiting " << dbp(statement) << " " << statement << indent);

diff --git a/midend/def_use.cpp b/midend/def_use.cpp
@@ -18,19 +18,36 @@ limitations under the License.
 
 #include "frontends/p4/methodInstance.h"
 
+struct LogAbbrev {
+    const Util::SourceInfo &si;
+    explicit LogAbbrev(const Util::SourceInfo &si) : si(si) {}
+
+    friend std::ostream &operator<<(std::ostream &out, const LogAbbrev &la) {
+        if (la.si) {
+            unsigned line, col;
+            out << '(' << la.si.toSourcePositionData(&line, &col);
+            out << ':' << line << ':' << (col + 1) << ')';
+        }
+        return out;
+    }
+};
+
 namespace P4 {
 
 using namespace literals;
 
+int ComputeDefUse::uid_ctr = 0;
 const ordered_set<const ComputeDefUse::loc_t *> ComputeDefUse::empty = {};
 
 void ComputeDefUse::flow_merge(Visitor &a_) {
     ComputeDefUse &a = dynamic_cast<ComputeDefUse &>(a_);
+    LOG8("ComputeDefUse::flow_merge(" << a.uid << ") -> " << uid);
     unreachable &= a.unreachable;
     for (auto &di : a.def_info) def_info[di.first].flow_merge(di.second);
 }
 void ComputeDefUse::flow_copy(ControlFlowVisitor &a_) {
     ComputeDefUse &a = dynamic_cast<ComputeDefUse &>(a_);
+    LOG8("ComputeDefUse::flow_copy(" << a.uid << ") -> " << uid);
     BUG_CHECK(state == a.state, "inconsistent state in ComputeDefUse::flow_copy");
     unreachable = a.unreachable;
     def_info = a.def_info;
@@ -264,7 +281,7 @@ void ComputeDefUse::def_info_t::split_slice(le_bitrange range) {
 bool ComputeDefUse::preorder(const IR::P4Control *c) {
     BUG_CHECK(state == SKIPPING, "Nested %s not supported in ComputeDefUse", c);
     IndentCtl::TempIndent indent;
-    LOG5("ComputeDefUse(P4Control " << c->name << ")" << indent);
+    LOG5("ComputeDefUse" << uid << "(P4Control " << c->name << ")" << indent);
     bool is_type_declaration = !c->getTypeParameters()->empty();
     for (auto *p : c->getApplyParameters()->parameters)
         if (p->direction == IR::Direction::In || p->direction == IR::Direction::InOut) {
@@ -293,7 +310,7 @@ bool ComputeDefUse::preorder(const IR::P4Control *c) {
 bool ComputeDefUse::preorder(const IR::P4Table *tbl) {
     if (state == SKIPPING) return false;
     IndentCtl::TempIndent indent;
-    LOG5("ComputeDefUse(P4Table " << tbl->name << ")" << indent);
+    LOG5("ComputeDefUse" << uid << "(P4Table " << tbl->name << ")" << indent);
     if (auto key = tbl->getKey()) visit(key, "key");
     if (auto actions = tbl->getActionList()) {
         parallel_visit(actions->actionList, "actions");
@@ -307,15 +324,15 @@ bool ComputeDefUse::preorder(const IR::P4Action *act) {
     if (state == SKIPPING) return false;
     for (auto *p : *act->parameters) def_info[p].defs.insert(getLoc(p));
     IndentCtl::TempIndent indent;
-    LOG5("ComputeDefUse(P4Action " << act->name << ")" << indent);
+    LOG5("ComputeDefUse" << uid << "(P4Action " << act->name << ")" << indent);
     visit(act->body, "body");
     return false;
 }
 
 bool ComputeDefUse::preorder(const IR::P4Parser *p) {
     BUG_CHECK(state == SKIPPING, "Nested %s not supported in ComputeDefUse", p);
     IndentCtl::TempIndent indent;
-    LOG5("ComputeDefUse(P4Parser " << p->name << ")" << indent);
+    LOG5("ComputeDefUse" << uid << "(P4Parser " << p->name << ")" << indent);
     for (auto *a : p->getApplyParameters()->parameters)
         if (a->direction == IR::Direction::In || a->direction == IR::Direction::InOut)
             def_info[a].defs.insert(getLoc(a));
@@ -333,7 +350,7 @@ bool ComputeDefUse::preorder(const IR::P4Parser *p) {
     return false;
 }
 bool ComputeDefUse::preorder(const IR::ParserState *p) {
-    LOG5("ComputeDefUse(ParserState " << p->name << ")" << Log::indent);
+    LOG5("ComputeDefUse" << uid << "(ParserState " << p->name << ")" << Log::indent);
     return true;
 }
 void ComputeDefUse::revisit(const IR::ParserState *p) { LOG5("  * revisit " << p->name); }
@@ -345,6 +362,13 @@ bool ComputeDefUse::preorder(const IR::KeyElement *ke) {
     return false;
 }
 
+bool ComputeDefUse::preorder(const IR::AssignmentStatement *as) {
+    // visit RHS of assignment before LHS
+    visit(as->right, "right", 1);
+    visit(as->left, "left", 0);
+    return false;
+}
+
 // Add all definitions in the given def_info_t (whole and partial) as defs that reach
 // a use at the specified location
 void ComputeDefUse::add_uses(const loc_t *loc, def_info_t &di) {
@@ -396,6 +420,7 @@ static const IR::Expression *isValid(const IR::Member *m, const Visitor::Context
 
 const IR::Expression *ComputeDefUse::do_read(def_info_t &di, const IR::Expression *e,
                                              const Context *ctxt) {
+    LOG7("do_read(" << *e << "<" << e->id << ">" << LogAbbrev(e->srcInfo) << ")");
     if (!ctxt) {
     } else if (auto *m = ctxt->node->to<IR::Member>()) {
         if (auto *t = isValid(m, ctxt->parent)) {
@@ -471,6 +496,7 @@ static int constIntMethodArg(const Visitor::Context *ctxt) {
 
 const IR::Expression *ComputeDefUse::do_write(def_info_t &di, const IR::Expression *e,
                                               const Context *ctxt) {
+    LOG7("do_write(" << *e << "<" << e->id << ">" << LogAbbrev(e->srcInfo) << ")");
     if (!ctxt) {
     } else if (auto *m = ctxt->node->to<IR::Member>()) {
         if (auto *t = isValid(m, ctxt->parent)) {
@@ -566,7 +592,9 @@ const IR::Expression *ComputeDefUse::do_write(def_info_t &di, const IR::Expressi
 }
 
 bool ComputeDefUse::preorder(const IR::PathExpression *pe) {
-    LOG7("ComputeDefUse(PathExpression " << *pe << ")");
+    Log::TempIndent indent;
+    LOG6("ComputeDefUse" << uid << "(PathExpression " << *pe << '<' << pe->id << '>'
+                         << LogAbbrev(pe->srcInfo) << ")" << indent);
     if (pe->type->is<IR::Type_State>()) {
         auto *d = resolveUnique(pe->path->name, P4::ResolutionType::Any);
         BUG_CHECK(d, "failed to resolve %s", pe);
@@ -628,12 +656,18 @@ void ComputeDefUse::end_apply() { LOG5(defuse); }
 
 // Debugging
 std::ostream &operator<<(std::ostream &out, const ComputeDefUse::loc_t &loc) {
-    out << '<' << loc.node->id << '>';
-    if (loc.node->srcInfo) {
-        unsigned line, col;
-        out << '(' << loc.node->srcInfo.toSourcePositionData(&line, &col);
-        out << ':' << line << ':' << (col + 1) << ')';
+    out << '<' << loc.node->id << '>' << LogAbbrev(loc.node->srcInfo);
+    return out;
+}
+
+std::ostream &operator<<(std::ostream &out, const ordered_set<const ComputeDefUse::loc_t *> &s) {
+    out << ": {";
+    const char *sep = " ";
+    for (auto *l : s) {
+        out << sep << *l;
+        sep = ", ";
     }
+    out << (sep + 1) << "}";
     return out;
 }
 
@@ -649,13 +683,7 @@ std::ostream &operator<<(
         out << '(' << p.first->srcInfo.toSourcePositionData(&line, &col);
         out << ':' << line << ':' << (col + 1) << ')';
     }
-    out << ": {";
-    const char *sep = " ";
-    for (auto *l : p.second) {
-        out << sep << *l;
-        sep = ", ";
-    }
-    out << (sep + 1) << "}";
+    out << p.second;
     return out;
 }
 
@@ -669,3 +697,8 @@ std::ostream &operator<<(std::ostream &out, const ComputeDefUse::defuse_t &du) {
 }
 
 }  // namespace P4
+
+void dump(const P4::ComputeDefUse::loc_t &p) { std::cout << p << std::endl; }
+void dump(const ordered_set<const P4::ComputeDefUse::loc_t *> &p) { std::cout << p << std::endl; }
+void dump(const P4::ComputeDefUse &du) { std::cout << du << std::endl; }
+void dump(const P4::ComputeDefUse *du) { std::cout << *du << std::endl; }
diff --git a/midend/def_use.h b/midend/def_use.h
@@ -43,7 +43,14 @@ class ComputeDefUse : public Inspector,
                       public ControlFlowVisitor,
                       public P4WriteContext,
                       public P4::ResolutionContext {
-    ComputeDefUse *clone() const override { return new ComputeDefUse(*this); }
+    static int uid_ctr;
+    int uid = 0;
+    ComputeDefUse *clone() const override {
+        auto *rv = new ComputeDefUse(*this);
+        rv->uid = ++uid_ctr;
+        LOG8("ComputeDefUse::clone " << rv->uid);
+        return rv;
+    }
     void flow_merge(Visitor &) override;
     void flow_copy(ControlFlowVisitor &) override;
     bool operator==(const ControlFlowVisitor &) const override;
@@ -70,6 +77,7 @@ class ComputeDefUse : public Inspector,
             return nullptr;
         }
     };
+    typedef ordered_set<const loc_t *> locset_t;
 
  private:
     std::set<loc_t> &cached_locs;
@@ -83,13 +91,13 @@ class ComputeDefUse : public Inspector,
         // definitions of a symbol (or part of a symbol) visible at this point in the
         // program.  `defs` will be empty if `live` is; if not those defs are visible only
         // for those bits/elements/fields where live is set.
-        ordered_set<const loc_t *> defs;
+        locset_t defs;
         bitvec live;
         // FIXME -- this parent field is never used and is not set consistently, so
         // appears to be useless?
         def_info_t *parent = nullptr;
         // track valid bit access for headers separate from the rest of the header
-        ordered_set<const loc_t *> valid_bit_defs;
+        locset_t valid_bit_defs;
         // one of these maps will always be empty.
         std::map<cstring, def_info_t> fields;
         std::map<le_bitrange, def_info_t> slices;  // also used for arrays
@@ -114,14 +122,15 @@ class ComputeDefUse : public Inspector,
         // defs maps from all uses to their definitions
         // uses maps from all definitions to their uses
         // uses/defs are lvalue expressions, or param declarations.
-        ordered_map<const IR::Node *, ordered_set<const loc_t *>> defs;
-        ordered_map<const IR::Node *, ordered_set<const loc_t *>> uses;
+        ordered_map<const IR::Node *, locset_t> defs;
+        ordered_map<const IR::Node *, locset_t> uses;
     } & defuse;
-    static const ordered_set<const loc_t *> empty;
+    static const locset_t empty;
 
     profile_t init_apply(const IR::Node *root) override {
         auto rv = Inspector::init_apply(root);
         LOG3("## Midend ComputeDefUse");
+        uid_ctr = 0;
         state = SKIPPING;
         clear();
         return rv;
@@ -137,6 +146,7 @@ class ComputeDefUse : public Inspector,
     bool preorder(const IR::Type *) override { return false; }
     bool preorder(const IR::Annotations *) override { return false; }
     bool preorder(const IR::KeyElement *) override;
+    bool preorder(const IR::AssignmentStatement *) override;
     const IR::Expression *do_read(def_info_t &, const IR::Expression *, const Context *);
     const IR::Expression *do_write(def_info_t &, const IR::Expression *, const Context *);
     bool preorder(const IR::PathExpression *) override;
@@ -161,11 +171,11 @@ class ComputeDefUse : public Inspector,
         defuse.uses.clear();
     }
 
-    const ordered_set<const loc_t *> &getDefs(const IR::Node *n) const {
+    const locset_t &getDefs(const IR::Node *n) const {
         auto it = defuse.defs.find(n);
         return it == defuse.defs.end() ? empty : it->second;
     }
-    const ordered_set<const loc_t *> &getUses(const IR::Node *n) const {
+    const locset_t &getUses(const IR::Node *n) const {
         auto it = defuse.uses.find(n);
         return it == defuse.uses.end() ? empty : it->second;
     }