From 68fade4573d84a5baac6cfeab65467ef5baaf43f Mon Sep 17 00:00:00 2001 From: mbudiu-vmw Date: Wed, 19 Apr 2017 08:49:31 -0700 Subject: [PATCH 1/2] Attempt to fix (unsuccessful) --- backends/bmv2/jsonconverter.cpp | 25 +++++++++++++++++++++---- backends/bmv2/jsonconverter.h | 1 - 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/backends/bmv2/jsonconverter.cpp b/backends/bmv2/jsonconverter.cpp index c19c17a4a4..23b8151cd2 100644 --- a/backends/bmv2/jsonconverter.cpp +++ b/backends/bmv2/jsonconverter.cpp @@ -470,6 +470,12 @@ class ExpressionConverter : public Inspector { e->append(converter->jsonMetadataParameterName); e->append(fieldName); } else { + if (param != converter->userMetadataParameter) + // I.e., this is the headers parameter. + // Since the metadata and the headers parameters + // are flattened in the same JSON namespace, we + // use this trick to prevent name clashes. + fieldName += "@"; if (type->is()) { result->emplace("type", "header_stack"); result->emplace("value", fieldName); @@ -800,7 +806,7 @@ JsonConverter::JsonConverter(const CompilerOptions& options) : corelib(P4::P4CoreLibrary::instance), refMap(nullptr), typeMap(nullptr), toplevelBlock(nullptr), conv(new ExpressionConverter(this)), - headerParameter(nullptr), userMetadataParameter(nullptr), stdMetadataParameter(nullptr) + userMetadataParameter(nullptr), stdMetadataParameter(nullptr) {} // return calculation name @@ -2081,7 +2087,11 @@ void JsonConverter::addHeaderStacks(const IR::Type_Struct* headersStruct) { LOG3("Creating " << stack); auto json = new Util::JsonObject(); - json->emplace("name", extVisibleName(f)); + cstring fname = extVisibleName(f) + "@"; + // since the metadata and the headers parameters + // are flattened in the same JSON namespace, we + // use this trick to prevent name clashes + json->emplace("name", fname); json->emplace("id", nextId("stack")); json->emplace("size", stack->getSize()); auto type = typeMap->getTypeType(stack->elementType, true); @@ -2096,7 +2106,7 @@ void JsonConverter::addHeaderStacks(const IR::Type_Struct* headersStruct) { unsigned id = nextId("headers"); stackMembers->append(id); auto header = new Util::JsonObject(); - cstring name = extVisibleName(f) + "[" + Util::toString(i) + "]"; + cstring name = fname + "[" + Util::toString(i) + "]"; header->emplace("name", name); header->emplace("id", id); header->emplace("header_type", header_type); @@ -2451,7 +2461,13 @@ void JsonConverter::addTypesAndInstances(const IR::Type_StructLike* type, bool m auto ft = typeMap->getType(f, true); if (ft->is()) { auto json = new Util::JsonObject(); - json->emplace("name", extVisibleName(f)); + cstring name = extVisibleName(f); + if (!meta) + // since the metadata and the headers parameters + // are flattened in the same JSON namespace, we + // use this trick to prevent name clashes + name += "@"; + json->emplace("name", name); json->emplace("id", nextId("headers")); json->emplace("header_type", extVisibleName(ft->to())); json->emplace("metadata", meta); @@ -2490,6 +2506,7 @@ void JsonConverter::pushFields(cstring prefix, const IR::Type_StructLike *st, Util::JsonArray *fields) { for (auto f : st->fields) { auto ftype = typeMap->getType(f, true); + if (ftype->to()) { BUG("%1%: nested structure", st); } else if (ftype->is()) { diff --git a/backends/bmv2/jsonconverter.h b/backends/bmv2/jsonconverter.h index 057d9251aa..14c2297fc1 100644 --- a/backends/bmv2/jsonconverter.h +++ b/backends/bmv2/jsonconverter.h @@ -66,7 +66,6 @@ class JsonConverter final { ExpressionConverter* conv; DirectMeterMap meterMap; std::map directCountersMap; // map counter name to table - const IR::Parameter* headerParameter; const IR::Parameter* userMetadataParameter; const IR::Parameter* stdMetadataParameter; cstring jsonMetadataParameterName = "standard_metadata"; From 35c694821ac69b57c8a5b3ca71927f384dd7a87e Mon Sep 17 00:00:00 2001 From: mbudiu-vmw Date: Wed, 19 Apr 2017 14:06:00 -0700 Subject: [PATCH 2/2] Fix for issue #496 --- backends/bmv2/jsonconverter.cpp | 25 +--- backends/bmv2/jsonconverter.h | 1 + backends/p4test/midend.cpp | 2 +- frontends/p4/unusedDeclarations.cpp | 10 ++ frontends/p4/unusedDeclarations.h | 12 +- midend/inlining.cpp | 123 ++++++++++-------- midend/inlining.h | 53 +++++--- testdata/p4_16_samples/issue496.p4 | 51 ++++++++ .../issue414-bmv2-frontend.p4 | 7 - .../issue414-bmv2-midend.p4 | 16 --- .../issue496-frontend.p4 | 60 +++++++++ .../p4_16_samples_outputs/issue496-midend.p4 | 46 +++++++ testdata/p4_16_samples_outputs/issue496.p4 | 60 +++++++++ .../p4_16_samples_outputs/issue496.p4-stderr | 0 .../p4_16_samples_outputs/module-frontend.p4 | 27 ---- .../p4_16_samples_outputs/module.p4-stderr | 18 --- .../spec-ex16-frontend.p4 | 1 - .../p4_16_samples_outputs/spec-ex16-midend.p4 | 1 - 18 files changed, 343 insertions(+), 170 deletions(-) create mode 100644 testdata/p4_16_samples/issue496.p4 create mode 100644 testdata/p4_16_samples_outputs/issue496-frontend.p4 create mode 100644 testdata/p4_16_samples_outputs/issue496-midend.p4 create mode 100644 testdata/p4_16_samples_outputs/issue496.p4 create mode 100644 testdata/p4_16_samples_outputs/issue496.p4-stderr diff --git a/backends/bmv2/jsonconverter.cpp b/backends/bmv2/jsonconverter.cpp index 23b8151cd2..c19c17a4a4 100644 --- a/backends/bmv2/jsonconverter.cpp +++ b/backends/bmv2/jsonconverter.cpp @@ -470,12 +470,6 @@ class ExpressionConverter : public Inspector { e->append(converter->jsonMetadataParameterName); e->append(fieldName); } else { - if (param != converter->userMetadataParameter) - // I.e., this is the headers parameter. - // Since the metadata and the headers parameters - // are flattened in the same JSON namespace, we - // use this trick to prevent name clashes. - fieldName += "@"; if (type->is()) { result->emplace("type", "header_stack"); result->emplace("value", fieldName); @@ -806,7 +800,7 @@ JsonConverter::JsonConverter(const CompilerOptions& options) : corelib(P4::P4CoreLibrary::instance), refMap(nullptr), typeMap(nullptr), toplevelBlock(nullptr), conv(new ExpressionConverter(this)), - userMetadataParameter(nullptr), stdMetadataParameter(nullptr) + headerParameter(nullptr), userMetadataParameter(nullptr), stdMetadataParameter(nullptr) {} // return calculation name @@ -2087,11 +2081,7 @@ void JsonConverter::addHeaderStacks(const IR::Type_Struct* headersStruct) { LOG3("Creating " << stack); auto json = new Util::JsonObject(); - cstring fname = extVisibleName(f) + "@"; - // since the metadata and the headers parameters - // are flattened in the same JSON namespace, we - // use this trick to prevent name clashes - json->emplace("name", fname); + json->emplace("name", extVisibleName(f)); json->emplace("id", nextId("stack")); json->emplace("size", stack->getSize()); auto type = typeMap->getTypeType(stack->elementType, true); @@ -2106,7 +2096,7 @@ void JsonConverter::addHeaderStacks(const IR::Type_Struct* headersStruct) { unsigned id = nextId("headers"); stackMembers->append(id); auto header = new Util::JsonObject(); - cstring name = fname + "[" + Util::toString(i) + "]"; + cstring name = extVisibleName(f) + "[" + Util::toString(i) + "]"; header->emplace("name", name); header->emplace("id", id); header->emplace("header_type", header_type); @@ -2461,13 +2451,7 @@ void JsonConverter::addTypesAndInstances(const IR::Type_StructLike* type, bool m auto ft = typeMap->getType(f, true); if (ft->is()) { auto json = new Util::JsonObject(); - cstring name = extVisibleName(f); - if (!meta) - // since the metadata and the headers parameters - // are flattened in the same JSON namespace, we - // use this trick to prevent name clashes - name += "@"; - json->emplace("name", name); + json->emplace("name", extVisibleName(f)); json->emplace("id", nextId("headers")); json->emplace("header_type", extVisibleName(ft->to())); json->emplace("metadata", meta); @@ -2506,7 +2490,6 @@ void JsonConverter::pushFields(cstring prefix, const IR::Type_StructLike *st, Util::JsonArray *fields) { for (auto f : st->fields) { auto ftype = typeMap->getType(f, true); - if (ftype->to()) { BUG("%1%: nested structure", st); } else if (ftype->is()) { diff --git a/backends/bmv2/jsonconverter.h b/backends/bmv2/jsonconverter.h index 14c2297fc1..057d9251aa 100644 --- a/backends/bmv2/jsonconverter.h +++ b/backends/bmv2/jsonconverter.h @@ -66,6 +66,7 @@ class JsonConverter final { ExpressionConverter* conv; DirectMeterMap meterMap; std::map directCountersMap; // map counter name to table + const IR::Parameter* headerParameter; const IR::Parameter* userMetadataParameter; const IR::Parameter* stdMetadataParameter; cstring jsonMetadataParameterName = "standard_metadata"; diff --git a/backends/p4test/midend.cpp b/backends/p4test/midend.cpp index a99903f038..120c44f87d 100644 --- a/backends/p4test/midend.cpp +++ b/backends/p4test/midend.cpp @@ -83,7 +83,7 @@ MidEnd::MidEnd(CompilerOptions& options) { new P4::RemoveAllUnusedDeclarations(&refMap), new P4::ClearTypeMap(&typeMap), evaluator, - new VisitFunctor([v1controls, evaluator](const IR::Node *root) -> const IR::Node * { + new VisitFunctor([v1controls, evaluator](const IR::Node *root) -> const IR::Node * { auto toplevel = evaluator->getToplevelBlock(); auto main = toplevel->getMain(); if (main == nullptr) diff --git a/frontends/p4/unusedDeclarations.cpp b/frontends/p4/unusedDeclarations.cpp index e6712a2d86..d7d4769999 100644 --- a/frontends/p4/unusedDeclarations.cpp +++ b/frontends/p4/unusedDeclarations.cpp @@ -106,6 +106,16 @@ const IR::Node* RemoveUnusedDeclarations::preorder(IR::Declaration_Instance* dec if (!refMap->isUsed(getOriginal())) { if (giveWarning(getOriginal())) ::warning("%1%: unused instance", decl); + // We won't delete extern instances; these may be useful even if not references. + auto type = decl->type; + if (type->is()) + type = type->to()->baseType; + if (type->is()) + type = refMap->getDeclaration(type->to()->path, true)->to(); + if (!type->is()) + return process(decl); + prune(); + return decl; } // don't scan the initializer: we don't want to delete virtual methods prune(); diff --git a/frontends/p4/unusedDeclarations.h b/frontends/p4/unusedDeclarations.h index ac141da8c8..78a52dd4a0 100644 --- a/frontends/p4/unusedDeclarations.h +++ b/frontends/p4/unusedDeclarations.h @@ -24,7 +24,7 @@ namespace P4 { class RemoveUnusedDeclarations : public Transform { const ReferenceMap* refMap; - // If non-null give warnings about unused declarations + /// If non-null give warnings about unused declarations std::set* warned; const IR::Node* process(const IR::IDeclaration* decl); @@ -38,8 +38,8 @@ class RemoveUnusedDeclarations : public Transform { using Transform::preorder; using Transform::init_apply; - // True if we should report a warning; the node is - // added to warned in this case + /// True if we should report a warning; the node is + /// added to warned in this case bool giveWarning(const IR::Node* node); Visitor::profile_t init_apply(const IR::Node *root) override; @@ -52,7 +52,7 @@ class RemoveUnusedDeclarations : public Transform { const IR::Node* preorder(IR::Declaration_Instance* decl) override; - // Do not delete the following even if unused + // The following kinds of nodes are not deleted even if they are unreferenced const IR::Node* preorder(IR::Type_Error* type) override { prune(); return type; } const IR::Node* preorder(IR::Declaration_MatchKind* decl) override @@ -63,14 +63,14 @@ class RemoveUnusedDeclarations : public Transform { { prune(); return type; } const IR::Node* preorder(IR::Type_Method* type) override { prune(); return type; } + const IR::Node* preorder(IR::Parameter* param) override { return param; } // never dead const IR::Node* preorder(IR::Declaration_Variable* decl) override; - const IR::Node* preorder(IR::Parameter* param) override { return param; } // never dead const IR::Node* preorder(IR::Declaration* decl) override { return process(decl); } const IR::Node* preorder(IR::Type_Declaration* decl) override { return process(decl); } }; -// Iterates RemoveUnusedDeclarations until convergence. +/// Iterates RemoveUnusedDeclarations until convergence. class RemoveAllUnusedDeclarations : public PassManager { public: explicit RemoveAllUnusedDeclarations(ReferenceMap* refMap, bool warn = false) { diff --git a/midend/inlining.cpp b/midend/inlining.cpp index 3135af335f..400f527e6f 100644 --- a/midend/inlining.cpp +++ b/midend/inlining.cpp @@ -164,22 +164,24 @@ class FindLocationSets : public Inspector { } }; -// This class computes new names for inlined objects. -// An object's name is prefixed with the instance name that includes it. -// For example: -// control c() { -// table t() { ... } apply { t.apply() } -// } -// control d() { -// c() cinst; -// apply { cinst.apply(); } -// } -// After inlining we will get: -// control d() { -// @name("cinst.t") table cinst_t() { ... } -// apply { cinst_t.apply(); } -// } -// So the externally visible name for the table is "cinst.t" +/** +This class computes new names for inlined objects. +An object's name is prefixed with the instance name that includes it. +For example: +control c() { + table t() { ... } apply { t.apply() } +} +control d() { + c() cinst; + apply { cinst.apply(); } +} +After inlining we will get: +control d() { + @name("cinst.t") table cinst_t() { ... } + apply { cinst_t.apply(); } +} +So the externally visible name for the table is "cinst.t" +*/ class ComputeNewNames : public Inspector { cstring prefix; P4::ReferenceMap* refMap; @@ -220,11 +222,12 @@ setNameAnnotation(cstring name, const IR::Annotations* annos) { new IR::StringLiteral(name)); } - -// Perform multiple substitutions and rename global objects, such as -// tables, actions and instances. Unfortunately these transformations -// have to be performed at the same time, because otherwise the refMap -// is invalidated. +/** +Perform multiple substitutions and rename global objects, such as +tables, actions and instances. Unfortunately these transformations +have to be performed at the same time, because otherwise the refMap +is invalidated. +*/ class Substitutions : public SubstituteParameters { P4::ReferenceMap* refMap; // updated const SymRenameMap* renameMap; // map with new names for global objects @@ -241,7 +244,7 @@ class Substitutions : public SubstituteParameters { auto orig = getOriginal(); cstring newName = renameMap->getName(orig); cstring extName = renameMap->getExtName(orig); - LOG1("Renaming " << dbp(orig) << " to " << newName << " (" << extName << ")"); + LOG3("Renaming " << dbp(orig) << " to " << newName << " (" << extName << ")"); auto annos = setNameAnnotation(extName, table->annotations); auto result = new IR::P4Table(table->srcInfo, newName, annos, table->properties); @@ -251,7 +254,7 @@ class Substitutions : public SubstituteParameters { auto orig = getOriginal(); cstring newName = renameMap->getName(orig); cstring extName = renameMap->getExtName(orig); - LOG1("Renaming " << dbp(orig) << " to " << newName << "(" << extName << ")"); + LOG3("Renaming " << dbp(orig) << " to " << newName << "(" << extName << ")"); auto annos = setNameAnnotation(extName, action->annotations); auto result = new IR::P4Action(action->srcInfo, newName, annos, action->parameters, action->body); @@ -261,7 +264,7 @@ class Substitutions : public SubstituteParameters { auto orig = getOriginal(); cstring newName = renameMap->getName(orig); cstring extName = renameMap->getExtName(orig); - LOG1("Renaming " << dbp(orig) << " to " << newName << "(" << extName << ")"); + LOG3("Renaming " << dbp(orig) << " to " << newName << "(" << extName << ")"); auto annos = setNameAnnotation(extName, instance->annotations); instance->name = newName; instance->annotations = annos; @@ -271,18 +274,18 @@ class Substitutions : public SubstituteParameters { auto orig = getOriginal(); cstring newName = renameMap->getName(orig); cstring extName = renameMap->getExtName(orig); - LOG1("Renaming " << dbp(orig) << " to " << newName << "(" << extName << ")"); + LOG3("Renaming " << dbp(orig) << " to " << newName << "(" << extName << ")"); decl->name = newName; return decl; } const IR::Node* postorder(IR::PathExpression* expression) override { - LOG1("(Substitutions) visiting" << dbp(getOriginal())); + LOG3("(Substitutions) visiting" << dbp(getOriginal())); auto decl = refMap->getDeclaration(expression->path, true); auto param = decl->to(); if (param != nullptr && subst->contains(param)) { // This path is the same as in SubstituteParameters auto value = subst->lookup(param); - LOG1("(Substitutions) Replaced " << dbp(expression) << " for parameter " + LOG3("(Substitutions) Replaced " << dbp(expression) << " for parameter " << decl << " with " << dbp(value)); return value; } @@ -296,7 +299,7 @@ class Substitutions : public SubstituteParameters { auto newpath = new IR::Path(newid, expression->path->absolute); auto result = new IR::PathExpression(newpath); refMap->setDeclaration(newpath, decl); - LOG1("(Substitutions) replaced " << dbp(getOriginal()) << " with " << dbp(result)); + LOG3("(Substitutions) replaced " << dbp(getOriginal()) << " with " << dbp(result)); return result; } }; @@ -362,12 +365,12 @@ InlineSummary* InlineWorkList::next() { } const IR::Node* InlineDriver::preorder(IR::P4Program* program) { - LOG1("InlineDriver"); + LOG3("InlineDriver"); const IR::P4Program* prog = program; // we need the 'const' toInline->analyze(true); while (auto todo = toInline->next()) { - LOG1("Processing " << todo); + LOG3("Processing " << todo); inliner->prepare(toInline, todo); prog = prog->apply(*inliner); if (::errorCount() > 0) @@ -376,7 +379,7 @@ const IR::Node* InlineDriver::preorder(IR::P4Program* program) { #if 0 // debugging code; we don't have an easy way to dump the program here, // since we are not between passes - ToP4 top4(&std::cout, true, nullptr); + ToP4 top4(&std::cout, false, nullptr); prog->apply(top4); #endif } @@ -388,7 +391,7 @@ const IR::Node* InlineDriver::preorder(IR::P4Program* program) { ///////////////////////////////////////////////////////////////////////////////////////////// void DiscoverInlining::postorder(const IR::MethodCallStatement* statement) { - LOG2("Visiting " << statement); + LOG4("Visiting " << statement); auto mi = MethodInstance::resolve(statement, refMap, typeMap); if (!mi->isApply()) return; @@ -411,13 +414,13 @@ void DiscoverInlining::visit_all(const IR::Block* block) { } bool DiscoverInlining::preorder(const IR::ControlBlock* block) { - LOG2("Visiting " << block); + LOG4("Visiting " << block); if (getContext()->node->is()) { ::error("%1%: invocation of a control from a parser", block->node); } else if (getContext()->node->is() && allowControls) { auto parent = getContext()->node->to(); - LOG1("Will inline " << block << "@" << block->node << " into " << parent); + LOG3("Will inline " << block << "@" << block->node << " into " << parent); auto instance = block->node->to(); auto callee = block->container; inlineList->addInstantiation(parent->container, callee, instance); @@ -429,13 +432,13 @@ bool DiscoverInlining::preorder(const IR::ControlBlock* block) { } bool DiscoverInlining::preorder(const IR::ParserBlock* block) { - LOG2("Visiting " << block); + LOG4("Visiting " << block); if (getContext()->node->is()) { ::error("%1%: invocation of a parser from a control", block->node); } else if (getContext()->node->is()) { auto parent = getContext()->node->to(); - LOG1("Will inline " << block << "@" << block->node << " into " << parent); + LOG3("Will inline " << block << "@" << block->node << " into " << parent); auto instance = block->node->to(); auto callee = block->container; inlineList->addInstantiation(parent->container, callee, instance); @@ -464,14 +467,24 @@ const IR::Node* GeneralInliner::preorder(IR::P4Control* caller) { } workToDo = &toInline->callerToWork[orig]; - LOG1("Analyzing " << dbp(caller)); + LOG3("Analyzing " << dbp(caller)); IR::IndexedVector locals; for (auto s : caller->controlLocals) { + /* Even if we inline the block, the declaration may still be needed. + Consider this example: + control D() { apply { } } + control C()(D d) { apply { d.apply(); }} + control I() { + D() d; // we can't delete this instantiation here after, because it is used below + C(d) c; + apply { c.apply(); } + } + */ + locals.push_back(s); auto inst = s->to(); if (inst == nullptr || workToDo->declToCallee.find(inst) == workToDo->declToCallee.end()) { // not a call - locals.push_back(s); } else { auto callee = workToDo->declToCallee[inst]->to(); CHECK_NULL(callee); @@ -513,7 +526,7 @@ const IR::Node* GeneralInliner::preorder(IR::P4Control* caller) { if (param1 == param2) continue; auto ls2 = ::get(locationSets, param2); if (ls1->overlaps(ls2)) { - LOG2("Arg for " << dbp(param1) << " aliases with arg for " + LOG4("Arg for " << dbp(param1) << " aliases with arg for " << dbp(param2) << ": using temp"); useTemporary.emplace(param1); useTemporary.emplace(param2); @@ -531,7 +544,7 @@ const IR::Node* GeneralInliner::preorder(IR::P4Control* caller) { // Substitute argument directly CHECK_NULL(mcd); auto initializer = mcd->substitution.lookup(param); - LOG1("Substituting callee parameter " << dbp(param) + LOG3("Substituting callee parameter " << dbp(param) << " with " << dbp(initializer)); substs->paramSubst.add(param, initializer); } else { @@ -539,7 +552,7 @@ const IR::Node* GeneralInliner::preorder(IR::P4Control* caller) { cstring newName = refMap->newName(param->name); auto path = new IR::PathExpression(newName); substs->paramSubst.add(param, path); - LOG1("Replacing " << param->name << " with " << newName); + LOG3("Replacing " << param->name << " with " << newName); auto vardecl = new IR::Declaration_Variable(newName, param->annotations, param->type); locals.push_back(vardecl); @@ -570,7 +583,7 @@ const IR::Node* GeneralInliner::preorder(IR::MethodCallStatement* statement) { auto orig = getOriginal(); if (workToDo->callToInstance.find(orig) == workToDo->callToInstance.end()) return statement; - LOG1("Inlining invocation " << dbp(orig)); + LOG3("Inlining invocation " << dbp(orig)); auto decl = workToDo->callToInstance[orig]; CHECK_NULL(decl); @@ -586,7 +599,7 @@ const IR::Node* GeneralInliner::preorder(IR::MethodCallStatement* statement) { MethodCallDescription mcd(statement->methodCall, refMap, typeMap); for (auto param : *mcd.substitution.getParameters()) { - LOG1("Looking for " << param->name); + LOG3("Looking for " << param->name); auto initializer = substs->paramSubst.lookup(param); auto arg = mcd.substitution.lookup(param); if ((param->direction == IR::Direction::In || param->direction == IR::Direction::InOut) && @@ -619,7 +632,7 @@ const IR::Node* GeneralInliner::preorder(IR::MethodCallStatement* statement) { auto annotations = callee->type->annotations->where( [](const IR::Annotation* a) { return a->name != IR::Annotation::nameAnnotation; }); auto result = new IR::BlockStatement(statement->srcInfo, annotations, body); - LOG1("Replacing " << dbp(orig) << " with " << dbp(result)); + LOG3("Replacing " << dbp(orig) << " with " << dbp(result)); prune(); return result; } @@ -648,10 +661,12 @@ class ComputeNewStateNames : public Inspector { } }; -// Renames the states in a parser for inlining. We cannot rely on the -// ReferenceMap for identifying states - it is currently inconsistent, -// so we rely on the fact that state names only appear in very -// specific places. +/** +Renames the states in a parser for inlining. We cannot rely on the +ReferenceMap for identifying states - it is currently inconsistent, +so we rely on the fact that state names only appear in very +specific places. +*/ class RenameStates : public Transform { std::map *stateRenameMap; @@ -697,7 +712,7 @@ class RenameStates : public Transform { } // namespace const IR::Node* GeneralInliner::preorder(IR::ParserState* state) { - LOG1("Visiting state " << dbp(state)); + LOG3("Visiting state " << dbp(state)); auto states = new IR::IndexedVector(); IR::IndexedVector current; @@ -716,7 +731,7 @@ const IR::Node* GeneralInliner::preorder(IR::ParserState* state) { continue; } - LOG1("Inlining invocation " << dbp(call)); + LOG3("Inlining invocation " << dbp(call)); auto decl = workToDo->callToInstance[call]; CHECK_NULL(decl); @@ -729,7 +744,7 @@ const IR::Node* GeneralInliner::preorder(IR::ParserState* state) { auto it = call->methodCall->arguments->begin(); for (auto param : callee->type->applyParams->parameters) { auto initializer = *it; - LOG1("Looking for " << param->name); + LOG3("Looking for " << param->name); if (param->direction == IR::Direction::In || param->direction == IR::Direction::InOut) { auto expr = substs->paramSubst.lookupByName(param->name); auto stat = new IR::AssignmentStatement(expr, initializer); @@ -788,7 +803,7 @@ const IR::Node* GeneralInliner::preorder(IR::ParserState* state) { auto newState = new IR::ParserState(name, annotations, current, state->selectExpression); states->push_back(newState); - LOG1("Replacing with " << states->size() << " states"); + LOG3("Replacing with " << states->size() << " states"); prune(); return states; } @@ -805,7 +820,7 @@ const IR::Node* GeneralInliner::preorder(IR::P4Parser* caller) { } workToDo = &toInline->callerToWork[orig]; - LOG1("Analyzing " << dbp(caller)); + LOG3("Analyzing " << dbp(caller)); IR::IndexedVector locals; for (auto s : caller->parserLocals) { auto inst = s->to(); @@ -840,7 +855,7 @@ const IR::Node* GeneralInliner::preorder(IR::P4Parser* caller) { cstring newName = refMap->newName(param->name); auto path = new IR::PathExpression(newName); substs->paramSubst.add(param, path); - LOG1("Replacing " << param->name << " with " << newName); + LOG3("Replacing " << param->name << " with " << newName); auto vardecl = new IR::Declaration_Variable(newName, param->annotations, param->type); locals.push_back(vardecl); diff --git a/midend/inlining.h b/midend/inlining.h index d554d87b68..f5efdc2177 100644 --- a/midend/inlining.h +++ b/midend/inlining.h @@ -31,7 +31,7 @@ limitations under the License. namespace P4 { -// Describes information about a caller-callee pair +/// Describes information about a caller-callee pair struct CallInfo { const IR::IContainer* caller; const IR::IContainer* callee; @@ -94,18 +94,18 @@ struct PerInstanceSubstitutions { const T* rename(ReferenceMap* refMap, const IR::Node* node); }; -// Summarizes all inline operations to be performed. +/// Summarizes all inline operations to be performed. struct InlineSummary { - // Various substitutions that must be applied for each instance + /// Various substitutions that must be applied for each instance struct PerCaller { // information for each caller - // For each instance (key) the container that is intantiated. + /// For each instance (key) the container that is intantiated. std::map declToCallee; - // For each instance (key) we must apply a bunch of substitutions + /// For each instance (key) we must apply a bunch of substitutions std::map substitutions; - // For each invocation (key) call the instance that is invoked. + /// For each invocation (key) call the instance that is invoked. std::map callToInstance; - // nullptr if there isn't exactly one caller, - // otherwise the single caller of this instance. + /// @returns nullptr if there isn't exactly one caller, + /// otherwise the single caller of this instance. const IR::MethodCallStatement* uniqueCaller( const IR::Declaration_Instance* instance) const { const IR::MethodCallStatement* call = nullptr; @@ -170,13 +170,13 @@ class InlineWorkList { InlineSummary* next(); }; -// Base class for an inliner: -// Information to inline is in list -// Future inlining information is in toInline; must be updated -// as inlining is performed (since callers change into new nodes). +/// Base class for an inliner. class AbstractInliner : public Transform { protected: + /// Information to inline InlineWorkList* list; + /// Future inlining information; must be updated as inlining is + /// performed (since callers change into new nodes). InlineSummary* toInline; AbstractInliner() : list(nullptr), toInline(nullptr) {} public: @@ -193,7 +193,7 @@ class AbstractInliner : public Transform { virtual ~AbstractInliner() {} }; -// Repeatedly invokes an abstract inliner with work from the worklist +/// Repeatedly invokes an abstract inliner with work from the worklist class InlineDriver : public Transform { InlineWorkList* toInline; AbstractInliner* inliner; @@ -201,12 +201,12 @@ class InlineDriver : public Transform { InlineDriver(InlineWorkList* toInline, AbstractInliner* inliner) : toInline(toInline), inliner(inliner) { CHECK_NULL(toInline); CHECK_NULL(inliner); setName("InlineDriver"); } - // Not really a visitor, but we want to embed it into a PassManager, + // This is not really a visitor, but we want to embed it into a PassManager, // so we make it look like a visitor. const IR::Node* preorder(IR::P4Program* program) override; }; -// Must be run after an evaluator; uses the blocks to discover caller/callee relationships. +/// Must be run after an evaluator; uses the blocks to discover caller/callee relationships. class DiscoverInlining : public Inspector { InlineWorkList* inlineList; // output: result is here ReferenceMap* refMap; // input @@ -240,7 +240,7 @@ class DiscoverInlining : public Inspector { { visit_all(toplevel); return false; } }; -// Performs actual inlining work +/// Performs actual inlining work class GeneralInliner : public AbstractInliner { ReferenceMap* refMap; TypeMap* typeMap; @@ -259,14 +259,31 @@ class GeneralInliner : public AbstractInliner { Visitor::profile_t init_apply(const IR::Node* node) override; }; -class Inline : public PassManager { +/// Performs one round of inlining bottoms-up +class InlinePass : public PassManager { InlineWorkList toInline; public: - Inline(ReferenceMap* refMap, TypeMap* typeMap, EvaluatorPass* evaluator) { + InlinePass(ReferenceMap* refMap, TypeMap* typeMap, EvaluatorPass* evaluator) { passes.push_back(new TypeChecking(refMap, typeMap)); passes.push_back(new DiscoverInlining(&toInline, refMap, typeMap, evaluator)); passes.push_back(new InlineDriver(&toInline, new P4::GeneralInliner(refMap->isV1()))); passes.push_back(new RemoveAllUnusedDeclarations(refMap)); + setName("InlinePass"); + } +}; + +/** +Performs inlining as many times as necessary. Most frequently once +will be enough. Multiple iterations are necessary only when instances are +passed as arguments using constructor arguments. +*/ +class Inline : public PassRepeated { + public: + Inline(ReferenceMap* refMap, TypeMap* typeMap, EvaluatorPass* evaluator) { + passes.push_back(new InlinePass(refMap, typeMap, evaluator)); + // After inlining the output of the evaluator changes, so + // we have to run it again + passes.push_back(evaluator); setName("Inline"); } }; diff --git a/testdata/p4_16_samples/issue496.p4 b/testdata/p4_16_samples/issue496.p4 new file mode 100644 index 0000000000..3ff3e67f3b --- /dev/null +++ b/testdata/p4_16_samples/issue496.p4 @@ -0,0 +1,51 @@ +#include + +header h_t { + bit<8> f; +} + +struct my_packet { + h_t h; +} + +struct my_metadata { } + +parser MyParser(packet_in b, out my_packet p, inout my_metadata m, inout standard_metadata_t s) { + state start { + transition accept; + } +} + +control MyVerifyChecksum(in my_packet hdr, inout my_metadata meta) { + apply { } +} + +control D() { + apply { } +} + +control C()(D d) { + apply { + d.apply(); + } +} + +control MyIngress(inout my_packet p, inout my_metadata m, inout standard_metadata_t s) { + D() d; + C(d) c; + apply { c.apply(); } +} + +control MyEgress(inout my_packet p, inout my_metadata m, inout standard_metadata_t s) { + apply { } +} + +control MyComputeChecksum(inout my_packet p, inout my_metadata m) { + apply { } +} + +control MyDeparser(packet_out b, in my_packet p) { + apply { } +} + +V1Switch(MyParser(), MyVerifyChecksum(), MyIngress(), MyEgress(), MyComputeChecksum(), MyDeparser()) main; diff --git a/testdata/p4_16_samples_outputs/issue414-bmv2-frontend.p4 b/testdata/p4_16_samples_outputs/issue414-bmv2-frontend.p4 index e413ea705f..7c62f153ba 100644 --- a/testdata/p4_16_samples_outputs/issue414-bmv2-frontend.p4 +++ b/testdata/p4_16_samples_outputs/issue414-bmv2-frontend.p4 @@ -23,12 +23,6 @@ control DeparserI(packet_out packet, in Parsed_packet hdr) { } } -control cBar(inout mystruct1 meta) { - apply { - meta.a = meta.a + 4w15; - } -} - parser parserI(packet_in pkt, out Parsed_packet hdr, inout mystruct1 meta, inout standard_metadata_t stdmeta) { state start { pkt.extract(hdr.ethernet); @@ -37,7 +31,6 @@ parser parserI(packet_in pkt, out Parsed_packet hdr, inout mystruct1 meta, inout } control cIngress(inout Parsed_packet hdr, inout mystruct1 meta, inout standard_metadata_t stdmeta) { - @name("cbar_inst2") cBar() cbar_inst2_0; @name("foo") action foo_0() { meta.b = meta.b + 4w5; } diff --git a/testdata/p4_16_samples_outputs/issue414-bmv2-midend.p4 b/testdata/p4_16_samples_outputs/issue414-bmv2-midend.p4 index 720dc96bd8..e76d320f8e 100644 --- a/testdata/p4_16_samples_outputs/issue414-bmv2-midend.p4 +++ b/testdata/p4_16_samples_outputs/issue414-bmv2-midend.p4 @@ -23,21 +23,6 @@ control DeparserI(packet_out packet, in Parsed_packet hdr) { } } -control cBar(inout mystruct1 meta) { - @hidden action act() { - meta.a = meta.a + 4w15; - } - @hidden table tbl_act { - actions = { - act(); - } - const default_action = act(); - } - apply { - tbl_act.apply(); - } -} - parser parserI(packet_in pkt, out Parsed_packet hdr, inout mystruct1 meta, inout standard_metadata_t stdmeta) { state start { pkt.extract(hdr.ethernet); @@ -46,7 +31,6 @@ parser parserI(packet_in pkt, out Parsed_packet hdr, inout mystruct1 meta, inout } control cIngress(inout Parsed_packet hdr, inout mystruct1 meta, inout standard_metadata_t stdmeta) { - @name("cbar_inst2") cBar() cbar_inst2; @name("foo") action foo_0() { meta.b = meta.b + 4w5; } diff --git a/testdata/p4_16_samples_outputs/issue496-frontend.p4 b/testdata/p4_16_samples_outputs/issue496-frontend.p4 new file mode 100644 index 0000000000..95130b3c43 --- /dev/null +++ b/testdata/p4_16_samples_outputs/issue496-frontend.p4 @@ -0,0 +1,60 @@ +#include +#include + +header h_t { + bit<8> f; +} + +struct my_packet { + h_t h; +} + +struct my_metadata { +} + +parser MyParser(packet_in b, out my_packet p, inout my_metadata m, inout standard_metadata_t s) { + state start { + transition accept; + } +} + +control MyVerifyChecksum(in my_packet hdr, inout my_metadata meta) { + apply { + } +} + +control D() { + apply { + } +} + +control C()(D d) { + apply { + d.apply(); + } +} + +control MyIngress(inout my_packet p, inout my_metadata m, inout standard_metadata_t s) { + @name("d") D() d_0; + @name("c") C(d_0) c_0; + apply { + c_0.apply(); + } +} + +control MyEgress(inout my_packet p, inout my_metadata m, inout standard_metadata_t s) { + apply { + } +} + +control MyComputeChecksum(inout my_packet p, inout my_metadata m) { + apply { + } +} + +control MyDeparser(packet_out b, in my_packet p) { + apply { + } +} + +V1Switch(MyParser(), MyVerifyChecksum(), MyIngress(), MyEgress(), MyComputeChecksum(), MyDeparser()) main; diff --git a/testdata/p4_16_samples_outputs/issue496-midend.p4 b/testdata/p4_16_samples_outputs/issue496-midend.p4 new file mode 100644 index 0000000000..1e7583a862 --- /dev/null +++ b/testdata/p4_16_samples_outputs/issue496-midend.p4 @@ -0,0 +1,46 @@ +#include +#include + +header h_t { + bit<8> f; +} + +struct my_packet { + h_t h; +} + +struct my_metadata { +} + +parser MyParser(packet_in b, out my_packet p, inout my_metadata m, inout standard_metadata_t s) { + state start { + transition accept; + } +} + +control MyVerifyChecksum(in my_packet hdr, inout my_metadata meta) { + apply { + } +} + +control MyIngress(inout my_packet p, inout my_metadata m, inout standard_metadata_t s) { + apply { + } +} + +control MyEgress(inout my_packet p, inout my_metadata m, inout standard_metadata_t s) { + apply { + } +} + +control MyComputeChecksum(inout my_packet p, inout my_metadata m) { + apply { + } +} + +control MyDeparser(packet_out b, in my_packet p) { + apply { + } +} + +V1Switch(MyParser(), MyVerifyChecksum(), MyIngress(), MyEgress(), MyComputeChecksum(), MyDeparser()) main; diff --git a/testdata/p4_16_samples_outputs/issue496.p4 b/testdata/p4_16_samples_outputs/issue496.p4 new file mode 100644 index 0000000000..7e36079686 --- /dev/null +++ b/testdata/p4_16_samples_outputs/issue496.p4 @@ -0,0 +1,60 @@ +#include +#include + +header h_t { + bit<8> f; +} + +struct my_packet { + h_t h; +} + +struct my_metadata { +} + +parser MyParser(packet_in b, out my_packet p, inout my_metadata m, inout standard_metadata_t s) { + state start { + transition accept; + } +} + +control MyVerifyChecksum(in my_packet hdr, inout my_metadata meta) { + apply { + } +} + +control D() { + apply { + } +} + +control C()(D d) { + apply { + d.apply(); + } +} + +control MyIngress(inout my_packet p, inout my_metadata m, inout standard_metadata_t s) { + D() d; + C(d) c; + apply { + c.apply(); + } +} + +control MyEgress(inout my_packet p, inout my_metadata m, inout standard_metadata_t s) { + apply { + } +} + +control MyComputeChecksum(inout my_packet p, inout my_metadata m) { + apply { + } +} + +control MyDeparser(packet_out b, in my_packet p) { + apply { + } +} + +V1Switch(MyParser(), MyVerifyChecksum(), MyIngress(), MyEgress(), MyComputeChecksum(), MyDeparser()) main; diff --git a/testdata/p4_16_samples_outputs/issue496.p4-stderr b/testdata/p4_16_samples_outputs/issue496.p4-stderr new file mode 100644 index 0000000000..e69de29bb2 diff --git a/testdata/p4_16_samples_outputs/module-frontend.p4 b/testdata/p4_16_samples_outputs/module-frontend.p4 index 80d2b208db..e69de29bb2 100644 --- a/testdata/p4_16_samples_outputs/module-frontend.p4 +++ b/testdata/p4_16_samples_outputs/module-frontend.p4 @@ -1,27 +0,0 @@ -parser Filter(out bool filter); -package switch0(Filter f); -parser f(out bool x) { - state start { - transition accept; - } -} - -parser f2(out bool x, out bool y) { - state start { - transition accept; - } -} - -parser Extractor(out T dt); -parser Extractor2(out T1 data1, out T2 data2); -package switch1(Extractor e); -package switch2(Extractor e); -package switch3(Extractor2 e); -package switch4(Extractor2 e); -switch0(f()) main1; -switch1(f()) main3; -switch2(f()) main4; -switch3(f2()) main5; -switch4(f2()) main6; -switch1(f()) main2; -switch4(f2()) main7; diff --git a/testdata/p4_16_samples_outputs/module.p4-stderr b/testdata/p4_16_samples_outputs/module.p4-stderr index 6e86a1dfc5..6429fe2e1c 100644 --- a/testdata/p4_16_samples_outputs/module.p4-stderr +++ b/testdata/p4_16_samples_outputs/module.p4-stderr @@ -19,22 +19,4 @@ switch1(f()) main2; ../testdata/p4_16_samples/module.p4(44): warning: main7: unused instance switch4(f2()) main7; ^^^^^ -../testdata/p4_16_samples/module.p4(23): warning: out parameter x may be uninitialized when f terminates -parser f(out bool x) - ^ -../testdata/p4_16_samples/module.p4(23) -parser f(out bool x) - ^ -../testdata/p4_16_samples/module.p4(26): warning: out parameter x may be uninitialized when f2 terminates -parser f2(out bool x, out bool y) - ^ -../testdata/p4_16_samples/module.p4(26) -parser f2(out bool x, out bool y) - ^^ -../testdata/p4_16_samples/module.p4(26): warning: out parameter y may be uninitialized when f2 terminates -parser f2(out bool x, out bool y) - ^ -../testdata/p4_16_samples/module.p4(26) -parser f2(out bool x, out bool y) - ^^ warning: Program does not contain a `main' module diff --git a/testdata/p4_16_samples_outputs/spec-ex16-frontend.p4 b/testdata/p4_16_samples_outputs/spec-ex16-frontend.p4 index c094ef97be..7e539f29ff 100644 --- a/testdata/p4_16_samples_outputs/spec-ex16-frontend.p4 +++ b/testdata/p4_16_samples_outputs/spec-ex16-frontend.p4 @@ -15,4 +15,3 @@ control Map1(in bit<32> d) { } Switch>(P(), Map1()) main; -Switch>(P(), Map1()) main1; diff --git a/testdata/p4_16_samples_outputs/spec-ex16-midend.p4 b/testdata/p4_16_samples_outputs/spec-ex16-midend.p4 index c094ef97be..7e539f29ff 100644 --- a/testdata/p4_16_samples_outputs/spec-ex16-midend.p4 +++ b/testdata/p4_16_samples_outputs/spec-ex16-midend.p4 @@ -15,4 +15,3 @@ control Map1(in bit<32> d) { } Switch>(P(), Map1()) main; -Switch>(P(), Map1()) main1;