Revert "New checksum APIs in v1model; better alias analysis in side-e…

…ffects; … (p4lang#858)"

This reverts commit f09a95d.

This commit breaks all existing programs using checksums. This is a
change that needs to be staged more carefully, announced on the
mailing list and deprecate the old way in a more graceful manner,
giving people the chance of transitioning their programs.

backends/bmv2/backend.cpp

@@ -106,6 +106,7 @@ Backend::process(const IR::ToplevelBlock* tlb, BMV2Options& options) {
         new P4::ConstantFolding(refMap, typeMap, false),
         new P4::TypeChecking(refMap, typeMap),
         new RemoveComplexExpressions(refMap, typeMap, new ProcessControls(&pipeline_controls)),
+        new FixupChecksum(&update_checksum_controls),
         new P4::SimplifyControlFlow(refMap, typeMap),
         new P4::RemoveAllUnusedDeclarations(refMap),
         new DiscoverStructure(&structure),

backends/bmv2/lower.cpp

@@ -136,6 +136,248 @@ const IR::Node* LowerExpressions::postorder(IR::Concat* expression) {
 
 namespace {
 
+/**
+A list of assignments that all write to a "variable".
+This really only handles scalar variables.
+It is customized for the needs of FixupChecksum.
+*/
+struct VariableWriters {
+    std::set<const IR::AssignmentStatement*> writers;
+    VariableWriters() = default;
+    explicit VariableWriters(const IR::AssignmentStatement* writer)
+    { writers.emplace(writer); }
+    VariableWriters* join(const VariableWriters* other) const {
+        auto result = new VariableWriters();
+        result->writers = writers;
+        for (auto e : other->writers)
+            result->writers.emplace(e);
+        return result;
+    }
+    /**
+       This function returns a non-null value only if there is exaclty one writer statement.
+       In that case it returns the RHS of the assignment
+    */
+    const IR::Expression* substitution() const {
+        if (writers.size() != 1)
+            return nullptr;
+        auto first = *writers.begin();
+        return first->right;
+    }
+};
+
+/**
+Maintains a map from variable names to VariableWriters
+It is customized for the needs of FixupChecksum.
+*/
+struct VariableDefinitions {
+    std::map<cstring, const VariableWriters*> writers;
+    VariableDefinitions(const VariableDefinitions& other) = default;
+    VariableDefinitions() = default;
+    VariableDefinitions* clone() const {
+        return new VariableDefinitions(*this);
+    }
+    VariableDefinitions* join(const VariableDefinitions* other) const {
+        auto result = clone();
+        for (auto e : other->writers) {
+            auto &prev = result->writers[e.first];
+            prev = prev ? prev->join(e.second) : e.second;
+        }
+        return result;
+    }
+    void declare(const IR::Declaration_Variable* decl) {
+        writers.emplace(decl->getName().name, new VariableWriters());
+    }
+    const VariableWriters* getWriters(const IR::Path* path) {
+        if (path->absolute)
+            return nullptr;
+        return ::get(writers, path->name.name);
+    }
+    VariableDefinitions* setDefinition(const IR::Path* path,
+                                       const IR::AssignmentStatement* statement) {
+        auto w = getWriters(path);
+        if (w == nullptr)
+            // Path does not represent a variable
+            return this;
+        auto result = clone();
+        result->writers[path->name.name] = new VariableWriters(statement);
+        return result;
+    }
+};
+
+/**
+Maintain def-use information.
+It is customized for the needs of FixupChecksum.
+*/
+struct PathSubstitutions {
+    std::map<const IR::PathExpression*, const IR::Expression*> definitions;
+    std::set<const IR::AssignmentStatement*> haveUses;
+    PathSubstitutions() = default;
+    void add(const IR::PathExpression* path, const IR::Expression* expression) {
+        definitions.emplace(path, expression);
+        LOG3("Will substitute " << dbp(path) << " with " << expression);
+    }
+    const IR::Expression* get(const IR::PathExpression* path) const {
+        return ::get(definitions, path);
+    }
+    void foundUses(const VariableWriters* writers) {
+        for (auto w : writers->writers)
+            haveUses.emplace(w);
+    }
+    void foundUses(const IR::AssignmentStatement* statement) {
+        haveUses.emplace(statement);
+    }
+    bool hasUses(const IR::AssignmentStatement* statement) const {
+        return haveUses.find(statement) != haveUses.end();
+    }
+};
+
+/**
+See the SimpleCopyProp pass below for the context in which this
+analysis is run.  We take advantage that some more complex code
+patterns have already been eliminated.
+*/
+class Accesses : public Inspector {
+    PathSubstitutions* substitutions;
+    VariableDefinitions* currentDefinitions;
+
+    bool notSupported(const IR::Node* node) {
+        ::error("%1%: not supported in checksum update control", node);
+        return false;
+    }
+
+ public:
+    explicit Accesses(PathSubstitutions* substitutions): substitutions(substitutions) {
+        CHECK_NULL(substitutions); setName("Accesses");
+        currentDefinitions = new VariableDefinitions();
+    }
+
+    bool preorder(const IR::Declaration_Variable* decl) override {
+        // we assume all variable declarations are at the beginning
+        currentDefinitions->declare(decl);
+        return false;
+    }
+
+    // This is only invoked for read expressions
+    bool preorder(const IR::PathExpression* expression) override {
+        auto writers = currentDefinitions->getWriters(expression->path);
+        if (writers != nullptr) {
+            if (auto s = writers->substitution())
+                substitutions->add(expression, s);
+            else
+                substitutions->foundUses(writers);
+        }
+        return false;
+    }
+
+    bool preorder(const IR::AssignmentStatement* statement) override {
+        visit(statement->right);
+        if (statement->left->is<IR::PathExpression>()) {
+            auto pe = statement->left->to<IR::PathExpression>();
+            currentDefinitions = currentDefinitions->setDefinition(pe->path, statement);
+        } else {
+            substitutions->foundUses(statement);
+        }
+        return false;
+    }
+
+    bool preorder(const IR::IfStatement* statement) override {
+        visit(statement->condition);
+        auto defs = currentDefinitions->clone();
+        visit(statement->ifTrue);
+        auto afterTrue = currentDefinitions;
+        if (statement->ifFalse != nullptr) {
+            currentDefinitions = defs;
+            visit(statement->ifFalse);
+            currentDefinitions = afterTrue->join(currentDefinitions);
+        } else {
+            currentDefinitions = defs->join(afterTrue);
+        }
+        return false;
+    }
+
+    bool preorder(const IR::SwitchStatement* statement) override
+    { return notSupported(statement); }
+
+    bool preorder(const IR::P4Action* action) override
+    { return notSupported(action); }
+
+    bool preorder(const IR::P4Table* table) override
+    { return notSupported(table); }
+
+    bool preorder(const IR::ReturnStatement* statement) override
+    { return notSupported(statement); }
+
+    bool preorder(const IR::ExitStatement* statement) override
+    { return notSupported(statement); }
+};
+
+class Replace : public Transform {
+    const PathSubstitutions* substitutions;
+ public:
+    explicit Replace(const PathSubstitutions* substitutions): substitutions(substitutions) {
+        CHECK_NULL(substitutions); setName("Replace"); }
+
+    const IR::Node* postorder(IR::AssignmentStatement* statement) override {
+        if (!substitutions->hasUses(getOriginal<IR::AssignmentStatement>()))
+            return new IR::EmptyStatement();
+        return statement;
+    }
+
+    const IR::Node* postorder(IR::PathExpression* expression) override {
+        auto repl = substitutions->get(getOriginal<IR::PathExpression>());
+        if (repl != nullptr) {
+            Replace rpl(substitutions);
+            auto recurse = repl->apply(rpl);
+            return recurse;
+        }
+        return expression;
+    }
+};
+
+/**
+This analysis is only executed on the control which performs
+checksum update computations.
+
+This is a simpler variant of copy propagation; it just finds
+patterns of the form:
+tmp = X;
+...
+out = tmp;
+
+then it substitutes the definition into the use.
+The LocalCopyPropagation pass does not do this, because
+it won't consider replacing definitions where the RHS has side-effects.
+Since the only method call we accept in the checksum update block
+is a checksum unit "get" method (this is not checked here, but
+in the json code generator), we know that this method has no side-effects,
+so we can safely reorder calls to methods.
+Also, this is run after eliminating struct and tuple operations,
+so we know that all assignments operate on scalar values.
+*/
+class SimpleCopyProp : public PassManager {
+    PathSubstitutions substitutions;
+ public:
+    SimpleCopyProp() {
+        setName("SimpleCopyProp");
+        passes.push_back(new Accesses(&substitutions));
+        passes.push_back(new Replace(&substitutions));
+    }
+};
+
+}  // namespace
+
+const IR::Node* FixupChecksum::preorder(IR::P4Control* control) {
+    if (updateChecksumBlocks->find(control->name) != updateChecksumBlocks->end()) {
+        SimpleCopyProp scp;
+        return control->apply(scp);
+    }
+    return control;
+}
+
+////////////////////////////////////////////////////////////////////////////////////
+
+namespace {
+
 /**
 Detect whether a Select expression is too complicated for BMv2.
 Also used to detect complex expressions that are arguments

backends/bmv2/lower.h

@@ -49,6 +49,24 @@ class LowerExpressions : public Transform {
     { prune(); return table; }  // don't simplify expressions in table
 };
 
+/**
+This pass is a hack to work around current BMv2 limitations:
+checksum computations must be expressed in a restricted way, since
+the JSON code generator uses simple pattern-matching.
+
+The real solution to this problem is to have the BMv2 simulator use a
+real extern for computing and verifying checksums.  Then this hack
+would not be necessary anymore.
+*/
+class FixupChecksum : public Transform {
+    const std::set<cstring>* updateChecksumBlocks;
+ public:
+    explicit FixupChecksum(const std::set<cstring>* updateChecksumBlocks) :
+            updateChecksumBlocks(updateChecksumBlocks)
+    { setName("FixupChecksum"); }
+    const IR::Node* preorder(IR::P4Control* control) override;
+};
+
 /**
 Policy which selects the control blocks where remove
 complex expression is applied.

backends/bmv2/simpleSwitch.cpp

@@ -100,10 +100,6 @@ SimpleSwitch::convertHashAlgorithm(cstring algorithm) {
         result = "random";
     else if (algorithm == v1model.algorithm.identity.name)
         result = "identity";
-    else if (algorithm == v1model.algorithm.csum16.name)
-        result = "csum16";
-    else if (algorithm == v1model.algorithm.xor16.name)
-        result = "xor16";
     else
         ::error("%1%: unexpected algorithm", algorithm);
     return result;
@@ -251,9 +247,7 @@ SimpleSwitch::convertExternFunctions(Util::JsonArray *result,
         static std::set<cstring> supportedHashAlgorithms = {
             v1model.algorithm.crc32.name, v1model.algorithm.crc32_custom.name,
             v1model.algorithm.crc16.name, v1model.algorithm.crc16_custom.name,
-            v1model.algorithm.random.name, v1model.algorithm.identity.name,
-            v1model.algorithm.csum16.name, v1model.algorithm.xor16.name
-        };
+            v1model.algorithm.random.name, v1model.algorithm.identity.name };
 
         if (mc->arguments->size() != 5) {
             modelError("Expected 5 arguments for %1%", mc);
@@ -606,38 +600,47 @@ SimpleSwitch::generateUpdate(const IR::BlockStatement *block,
     auto typeMap = backend->getTypeMap();
     auto refMap = backend->getRefMap();
     auto conv = backend->getExpressionConverter();
+    // Currently this is very hacky to target the very limited support
+    // for checksums in BMv2 This will work much better when BMv2
+    // offers a checksum extern.
     for (auto stat : block->components) {
+        if (stat->is<IR::IfStatement>()) {
+            // The way checksums work in Json, they always ignore the condition!
+            stat = stat->to<IR::IfStatement>()->ifTrue;
+        }
         if (auto blk = stat->to<IR::BlockStatement>()) {
             generateUpdate(blk, checksums, calculations);
             continue;
-        } else if (auto mc = stat->to<IR::MethodCallStatement>()) {
-            auto mi = P4::MethodInstance::resolve(mc->methodCall, refMap, typeMap);
-            if (auto em = mi->to<P4::ExternFunction>()) {
-                if (em->method->name.name == v1model.update_checksum.name) {
-                    BUG_CHECK(mi->expr->arguments->size() == 4, "%1%: Expected 4 arguments", mc);
-                    auto cksum = new Util::JsonObject();
-                    auto ei = P4::EnumInstance::resolve(mi->expr->arguments->at(3), typeMap);
-                    if (ei->name != "csum16") {
-                        ::error("%1%: the only supported algorithm is csum16",
-                                mi->expr->arguments->at(3));
-                        return;
+        } else if (auto assign = stat->to<IR::AssignmentStatement>()) {
+            if (auto mc = assign->right->to<IR::MethodCallExpression>()) {
+                auto mi = P4::MethodInstance::resolve(mc, refMap, typeMap);
+                if (auto em = mi->to<P4::ExternMethod>()) {
+                    if (em->method->name.name == v1model.ck16.get.name &&
+                        em->originalExternType->name.name == v1model.ck16.name) {
+                        if (mi->expr->arguments->size() != 1) {
+                            modelError("%1%: Expected 1 argument", assign->right);
+                            return;
+                        }
+                        auto cksum = new Util::JsonObject();
+                        cstring calcName = createCalculation("csum16", mi->expr->arguments->at(0),
+                                                             calculations, mc);
+                        cksum->emplace("name", refMap->newName("cksum_"));
+                        cksum->emplace("id", nextId("checksums"));
+                        // TODO(jafingerhut) - add line/col here?
+                        auto jleft = conv->convert(assign->left);
+                        cksum->emplace("target", jleft->to<Util::JsonObject>()->get("value"));
+                        cksum->emplace("type", "generic");
+                        cksum->emplace("calculation", calcName);
+                        checksums->append(cksum);
+                        continue;
                     }
-                    cstring calcName = createCalculation(ei->name, mi->expr->arguments->at(1),
-                                                         calculations, mc);
-                    cksum->emplace("name", refMap->newName("cksum_"));
-                    cksum->emplace("id", nextId("checksums"));
-                    // TODO(jafingerhut) - add line/col here?
-                    auto jleft = conv->convert(mi->expr->arguments->at(2));
-                    cksum->emplace("target", jleft->to<Util::JsonObject>()->get("value"));
-                    cksum->emplace("type", "generic");
-                    cksum->emplace("calculation", calcName);
-                    checksums->append(cksum);
-                    continue;
                 }
             }
+        } else if (auto mc = stat->to<IR::MethodCallStatement>()) {
+            auto mi = P4::MethodInstance::resolve(mc->methodCall, refMap, typeMap, true);
+            BUG_CHECK(mi && mi->isApply(), "Call of something other than an apply method");
         }
-        ::error("%1%: Only calls to %2% allowed", stat, v1model.update_checksum);
-        return;
+        BUG("%1%: not handled yet", stat);
     }
 }
 

frontends/p4/fromv1.0/converters.cpp

@@ -970,8 +970,6 @@ class FixExtracts final : public Transform {
   do much better than this.
 */
 class AdjustLengths : public Transform {
- public:
-    AdjustLengths() { setName("AdjustLengths"); }
     const IR::Node* postorder(IR::PathExpression* expression) override {
         auto anno = findContext<IR::Annotation>();
         if (anno == nullptr)