Skip to content

Commit

Permalink
Instance caching: fix handling of modport exports and extern iface me…
Browse files Browse the repository at this point in the history 
…thods
  • Loading branch information
@MikePopoloski
MikePopoloski committed Feb 16, 2025
1 parent 4a3a738 commit 0032182
Show file tree
Hide file tree
Showing 8 changed files with 132 additions and 64 deletions.
40 changes: 22 additions & 18 deletions include/slang/ast/Compilation.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -765,6 +765,26 @@ class SLANG_EXPORT Compilation : public BumpAllocator {
const ResolvedConfig* resolvedConfig = nullptr;
};

// Captures the side effects that are applied by an instance indirectly instead
// of via a port connection.
struct InstanceSideEffects {
struct IfacePortDriver {
not_null<const HierarchicalReference*> ref;
not_null<const ValueDriver*> driver;
};

// Drivers that are applied through interface ports.
std::vector<IfacePortDriver> ifacePortDrivers;

// All upward names that extend out of the instance.
std::vector<const HierarchicalReference*> upwardNames;

// Indicates whether this instance can't be cached
// due to something like declaring bind directives
// or extern interface methods.
bool cannotCache = false;
};

// These functions are called by Scopes to create and track various members.
Scope::DeferredMemberData& getOrAddDeferredData(Scope::DeferredMemberIndex& index);

Expand All @@ -790,6 +810,8 @@ class SLANG_EXPORT Compilation : public BumpAllocator {
void checkBindTargetParams(const syntax::BindDirectiveSyntax& syntax, const Scope& scope,
const ResolvedBind& resolvedBind);
void checkVirtualIfaceInstance(const InstanceSymbol& instance);
InstanceSideEffects& getOrAddSideEffects(const Symbol& instanceBody);
void noteCannotCache(const Scope& scope);
std::pair<DefinitionLookupResult, bool> resolveConfigRule(const Scope& scope,
const ConfigRule& rule) const;
std::pair<DefinitionLookupResult, bool> resolveConfigRules(
Expand Down Expand Up @@ -862,24 +884,6 @@ class SLANG_EXPORT Compilation : public BumpAllocator {
flat_hash_map<std::tuple<std::string_view, SymbolKind>, std::shared_ptr<SystemSubroutine>>
methodMap;

// Captures the side effects that are applied by an instance indirectly instead
// of via a port connection.
struct InstanceSideEffects {
struct IfacePortDriver {
not_null<const HierarchicalReference*> ref;
not_null<const ValueDriver*> driver;
};

// Drivers that are applied through interface ports.
std::vector<IfacePortDriver> ifacePortDrivers;

// All upward names that extend out of the instance.
std::vector<const HierarchicalReference*> upwardNames;

// Whether the instance has bind directives within it.
bool hasBindDirectives = false;
};

// Map from instance bodies to side effects applied by them.
flat_hash_map<const Symbol*, std::unique_ptr<InstanceSideEffects>> instanceSideEffectMap;

Expand Down
8 changes: 6 additions & 2 deletions include/slang/ast/Scope.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -304,11 +304,15 @@ class SLANG_EXPORT Scope {
std::vector<std::pair<const syntax::SyntaxNode*, const Symbol*>> portDecls;

public:
// A flag indicating whether any enums have been registered in the scope.
// Indicates whether any enums have been registered in the scope.
bool hasEnums = false;

// A flag indicating whether there are any bind directives targeting this scope.
// Indicates whether there are any bind directives targeting this scope.
bool hasBinds = false;

// Indicates whether this scope is uncacheable, for instance if
// it contains an extern iface method implementation.
bool isUncacheable = false;
};

DeferredMemberData& getOrAddDeferredData() const;
Expand Down
6 changes: 3 additions & 3 deletions include/slang/ast/symbols/SubroutineSymbols.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,9 +137,9 @@ class SLANG_EXPORT SubroutineSymbol : public Symbol, public Scope {

void serializeTo(ASTSerializer& serializer) const;

static SubroutineSymbol* fromSyntax(Compilation& compilation,
const syntax::FunctionDeclarationSyntax& syntax,
const Scope& parent, bool outOfBlock);
static std::pair<SubroutineSymbol*, bool> fromSyntax(
Compilation& compilation, const syntax::FunctionDeclarationSyntax& syntax,
const Scope& parent, bool outOfBlock);

static SubroutineSymbol* fromSyntax(Compilation& compilation,
const syntax::ClassMethodDeclarationSyntax& syntax,
Expand Down
52 changes: 28 additions & 24 deletions source/ast/Compilation.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1107,22 +1107,7 @@ void Compilation::noteBindDirective(const BindDirectiveSyntax& syntax, const Sco

if (!scope.isUninstantiated()) {
bindDirectives.emplace_back(&syntax, &scope);

auto currScope = &scope;
do {
auto& symbol = currScope->asSymbol();
if (symbol.kind == SymbolKind::InstanceBody) {
auto& entry = instanceSideEffectMap[&symbol];
if (!entry)
entry = std::make_unique<InstanceSideEffects>();
else if (entry->hasBindDirectives)
break;

entry->hasBindDirectives = true;
}

currScope = symbol.getHierarchicalParent();
} while (currScope);
noteCannotCache(scope);
}
}

Expand Down Expand Up @@ -1342,10 +1327,8 @@ void Compilation::noteHierarchicalReference(const Scope& initialScope,
break;

if (sym.kind == SymbolKind::InstanceBody) {
auto& entry = instanceSideEffectMap[&sym];
if (!entry)
entry = std::make_unique<InstanceSideEffects>();
entry->upwardNames.push_back(&ref);
auto& entry = getOrAddSideEffects(sym);
entry.upwardNames.push_back(&ref);
}

currScope = sym.getHierarchicalParent();
Expand All @@ -1366,10 +1349,8 @@ void Compilation::noteInterfacePortDriver(const HierarchicalReference& ref,
auto& symbol = scope->asSymbol();
SLANG_ASSERT(symbol.kind == SymbolKind::InstanceBody);

auto& entry = instanceSideEffectMap[&symbol];
if (!entry)
entry = std::make_unique<InstanceSideEffects>();
entry->ifacePortDrivers.push_back({&ref, &driver});
auto& entry = getOrAddSideEffects(symbol);
entry.ifacePortDrivers.push_back({&ref, &driver});
}

void Compilation::noteVirtualIfaceInstance(const InstanceSymbol& symbol) {
Expand Down Expand Up @@ -2368,6 +2349,29 @@ void Compilation::checkVirtualIfaceInstance(const InstanceSymbol& instance) {
}
}

Compilation::InstanceSideEffects& Compilation::getOrAddSideEffects(const Symbol& instanceBody) {
auto& entry = instanceSideEffectMap[&instanceBody];
if (!entry)
entry = std::make_unique<InstanceSideEffects>();
return *entry;
}

void Compilation::noteCannotCache(const Scope& scope) {
auto currScope = &scope;
do {
auto& symbol = currScope->asSymbol();
if (symbol.kind == SymbolKind::InstanceBody) {
auto& entry = getOrAddSideEffects(symbol);
if (entry.cannotCache)
break;

entry.cannotCache = true;
}

currScope = symbol.getHierarchicalParent();
} while (currScope);
}

void Compilation::resolveDefParamsAndBinds() {
TimeTraceScope timeScope("resolveDefParamsAndBinds"sv, ""sv);

Expand Down
20 changes: 15 additions & 5 deletions source/ast/ElabVisitors.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,8 +121,14 @@ struct DiagnosticVisitor : public ASTVisitor<DiagnosticVisitor, false, false> {
// processing and checking.
if (!symbol.modport.empty()) {
auto [_, modport] = symbol.getConnection();
if (modport)
modportsWithExports.push_back({&symbol, modport});
if (modport) {
for (auto& method : modport->membersOfType<MethodPrototypeSymbol>()) {
if (method.flags.has(MethodFlags::ModportExport)) {
modportsWithExports.push_back({&symbol, modport});
break;
}
}
}
}
}
}
Expand Down Expand Up @@ -184,8 +190,13 @@ struct DiagnosticVisitor : public ASTVisitor<DiagnosticVisitor, false, false> {
if (auto sub = symbol.getSubroutine())
handle(*sub);

if (symbol.flags.has(MethodFlags::InterfaceExtern))
if (symbol.flags.has(MethodFlags::InterfaceExtern)) {
externIfaceProtos.push_back(&symbol);

auto scope = symbol.getParentScope();
SLANG_ASSERT(scope);
compilation.noteCannotCache(*scope);
}
}

void handle(const GenericClassDefSymbol& symbol) {
Expand Down Expand Up @@ -504,7 +515,6 @@ struct DiagnosticVisitor : public ASTVisitor<DiagnosticVisitor, false, false> {
bool tryApplyFromCache(const InstanceSymbol& symbol) {
// TODO: Downward hierarchical references into such instances need to be accounted for
// - Make sure this doesn't automatically cause dup drivers
// TODO: modport exports, extern interface prototypes?
// TODO: multiple levels of iface ports connected to iface ports

// We can use a cached version of this instance's body if we have already
Expand Down Expand Up @@ -550,7 +560,7 @@ struct DiagnosticVisitor : public ASTVisitor<DiagnosticVisitor, false, false> {
// This could be optimized in the future by having another layer of caching based
// on what the name resolves to for each instance.
auto sideEffects = entry.sideEffects.value();
if (sideEffects && (sideEffects->hasBindDirectives || !sideEffects->upwardNames.empty())) {
if (sideEffects && (sideEffects->cannotCache || !sideEffects->upwardNames.empty())) {
return false;
}

Expand Down
12 changes: 9 additions & 3 deletions source/ast/Scope.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -276,11 +276,14 @@ void Scope::addMembers(const SyntaxNode& syntax) {
break;
case SyntaxKind::FunctionDeclaration:
case SyntaxKind::TaskDeclaration: {
auto subroutine = SubroutineSymbol::fromSyntax(compilation,
syntax.as<FunctionDeclarationSyntax>(),
*this, /* outOfBlock */ false);
auto [subroutine, isExternIfaceMethod] = SubroutineSymbol::fromSyntax(
compilation, syntax.as<FunctionDeclarationSyntax>(), *this, /* outOfBlock */ false);

if (subroutine)
addMember(*subroutine);

if (isExternIfaceMethod)
getOrAddDeferredData().isUncacheable = true;
break;
}
case SyntaxKind::DataDeclaration: {
Expand Down Expand Up @@ -813,6 +816,9 @@ void Scope::elaborate() const {
auto deferred = deferredData.getMembers();
deferredMemberIndex = DeferredMemberIndex::Invalid;

if (deferredData.isUncacheable)
compilation.noteCannotCache(*this);

// Enums need to be handled first because their value members may need
// to be looked up by methods below.
if (deferredData.hasEnums) {
Expand Down
21 changes: 12 additions & 9 deletions source/ast/symbols/SubroutineSymbols.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,10 @@ const Statement& SubroutineSymbol::getBody() const {
return *stmt;
}

SubroutineSymbol* SubroutineSymbol::fromSyntax(Compilation& compilation,
const FunctionDeclarationSyntax& syntax,
const Scope& parent, bool outOfBlock) {
std::pair<SubroutineSymbol*, bool> SubroutineSymbol::fromSyntax(
Compilation& compilation, const FunctionDeclarationSyntax& syntax, const Scope& parent,
bool outOfBlock) {

// If this subroutine has a scoped name, it should be an out of block declaration.
// We shouldn't create a symbol now, since we need the class prototype to hook
// us in to the correct scope. Register this syntax with the compilation so that
Expand All @@ -74,6 +75,7 @@ SubroutineSymbol* SubroutineSymbol::fromSyntax(Compilation& compilation,
if (auto last = parent.getLastMember())
index = (uint32_t)last->getIndex() + 1;

bool isExternIfaceMethod = false;
auto& scopedName = proto->name->as<ScopedNameSyntax>();
if (scopedName.separator.kind == TokenKind::DoubleColon) {
// This is an out-of-block class method implementation.
Expand All @@ -84,14 +86,15 @@ SubroutineSymbol* SubroutineSymbol::fromSyntax(Compilation& compilation,
// We should create the method like normal but not add it to
// the parent name map (because it can only be looked up via
// the interface instance).
auto result = SubroutineSymbol::fromSyntax(compilation, syntax, parent,
/* outOfBlock */ true);
auto [result, _] = SubroutineSymbol::fromSyntax(compilation, syntax, parent,
/* outOfBlock */ true);
SLANG_ASSERT(result);

result->setParent(parent, SymbolIndex(index));
compilation.addExternInterfaceMethod(*result);
isExternIfaceMethod = true;
}
return nullptr;
return {nullptr, isExternIfaceMethod};
}

Token nameToken = proto->name->getLastToken();
Expand Down Expand Up @@ -180,7 +183,7 @@ SubroutineSymbol* SubroutineSymbol::fromSyntax(Compilation& compilation,
}

result->arguments = arguments.copy(compilation);
return result;
return {result, false};
}

static std::pair<bitmask<MethodFlags>, Visibility> getMethodFlags(
Expand Down Expand Up @@ -239,7 +242,7 @@ static std::pair<bitmask<MethodFlags>, Visibility> getMethodFlags(
SubroutineSymbol* SubroutineSymbol::fromSyntax(Compilation& compilation,
const ClassMethodDeclarationSyntax& syntax,
const Scope& parent) {
auto result = fromSyntax(compilation, *syntax.declaration, parent, /* outOfBlock */ false);
auto [result, _] = fromSyntax(compilation, *syntax.declaration, parent, /* outOfBlock */ false);
if (!result)
return nullptr;

Expand Down Expand Up @@ -325,7 +328,7 @@ SubroutineSymbol& SubroutineSymbol::createOutOfBlock(Compilation& compilation,
const Scope& parent,
const Scope& definitionScope,
SymbolIndex outOfBlockIndex) {
auto result = fromSyntax(compilation, syntax, parent, /* outOfBlock */ true);
auto [result, _] = fromSyntax(compilation, syntax, parent, /* outOfBlock */ true);
SLANG_ASSERT(result);

// Set the parent pointer of the new subroutine so that lookups work correctly.
Expand Down
37 changes: 37 additions & 0 deletions tests/unittests/ast/InterfaceTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -893,3 +893,40 @@ endinterface:sliceIfc
CHECK(diags[0].code == diag::VirtualIfaceIfacePort);
CHECK(diags[1].code == diag::VirtualIfaceHierRef);
}

TEST_CASE("Extern and export methods with instance caching") {
auto tree = SyntaxTree::fromText(R"(
interface I;
extern task foo;
modport m(export foo);
modport n(import task foo);
endinterface
module m(I.m i);
task i.foo; endtask
endmodule
module n(I i);
o o1(i);
endmodule
module o(I i);
m m1(i);
endmodule
module top;
I i1();
n m1(i1), m2(i1);
I i2();
n m3(i2);
endmodule
)");

Compilation compilation;
compilation.addSyntaxTree(tree);

auto& diags = compilation.getAllDiagnostics();
REQUIRE(diags.size() == 1);
CHECK(diags[0].code == diag::DupInterfaceExternMethod);
}

0 comments on commit 0032182

Please sign in to comment.