New checksum APIs in v1model; better alias analysis in side-effects; … (#858)

Author: Mihai Budiu

backends/bmv2/backend.cpp

@@ -106,7 +106,6 @@ 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,248 +136,6 @@ 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,24 +49,6 @@ 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,6 +100,10 @@ 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;
@@ -247,7 +251,9 @@ 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.random.name, v1model.algorithm.identity.name,
+            v1model.algorithm.csum16.name, v1model.algorithm.xor16.name
+        };
 
         if (mc->arguments->size() != 5) {
             modelError("Expected 5 arguments for %1%", mc);
@@ -600,47 +606,38 @@ 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 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;
+        } 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;
                     }
+                    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");
         }
-        BUG("%1%: not handled yet", stat);
+        ::error("%1%: Only calls to %2% allowed", stat, v1model.update_checksum);
+        return;
     }
 }
 

frontends/p4/fromv1.0/converters.cpp

@@ -970,6 +970,8 @@ 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)