Skip to content

Commit

Permalink
Wrap InstanceBodySymbol parameter list in an accessor to ensure elabo…
Browse files Browse the repository at this point in the history 
…ration
  • Loading branch information
@MikePopoloski
MikePopoloski committed Apr 27, 2024
1 parent 502eb9c commit b9d5423
Show file tree
Hide file tree
Showing 6 changed files with 28 additions and 6 deletions.
2 changes: 1 addition & 1 deletion bindings/python/SymbolBindings.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -425,8 +425,8 @@ void registerSymbols(py::module_& m) {

py::class_<InstanceBodySymbol, Symbol, Scope>(m, "InstanceBodySymbol")
.def_readonly("parentInstance", &InstanceBodySymbol::parentInstance)
.def_readonly("parameters", &InstanceBodySymbol::parameters)
.def_readonly("isUninstantiated", &InstanceBodySymbol::isUninstantiated)
.def_property_readonly("parameters", &InstanceBodySymbol::getParameters)
.def_property_readonly("portList", &InstanceBodySymbol::getPortList)
.def_property_readonly("definition", &InstanceBodySymbol::getDefinition)
.def("findPort", &InstanceBodySymbol::findPort, byrefint, "portName"_a)
Expand Down
7 changes: 6 additions & 1 deletion include/slang/ast/symbols/InstanceSymbols.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,6 @@ class SLANG_EXPORT InstanceBodySymbol : public Symbol, public Scope {
const HierarchyOverrideNode* hierarchyOverrideNode = nullptr;

/// A copy of all port parameter symbols used to construct the instance body.
std::span<const ParameterSymbolBase* const> parameters;

/// Indicates whether the module isn't actually instantiated in the design.
/// This might be because it was created with invalid parameters simply to
Expand All @@ -158,6 +157,11 @@ class SLANG_EXPORT InstanceBodySymbol : public Symbol, public Scope {
const HierarchyOverrideNode* hierarchyOverrideNode, bool isUninstantiated,
bool isFromBind);

std::span<const ParameterSymbolBase* const> getParameters() const {
ensureElaborated();
return parameters;
}

std::span<const Symbol* const> getPortList() const {
ensureElaborated();
return portList;
Expand Down Expand Up @@ -193,6 +197,7 @@ class SLANG_EXPORT InstanceBodySymbol : public Symbol, public Scope {

const DefinitionSymbol& definition;
mutable std::span<const Symbol* const> portList;
std::span<const ParameterSymbolBase* const> parameters;
};

class SLANG_EXPORT InstanceArraySymbol : public Symbol, public Scope {
Expand Down
2 changes: 1 addition & 1 deletion source/ast/Compilation.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2120,7 +2120,7 @@ void Compilation::checkBindTargetParams(const syntax::BindDirectiveSyntax& synta
continue;

auto& inst = sym->as<InstanceSymbol>();
for (auto param : inst.body.parameters) {
for (auto param : inst.body.getParameters()) {
if (param->symbol.kind == SymbolKind::TypeParameter) {
auto& typeParam = param->symbol.as<TypeParameterSymbol>();
auto& type = typeParam.targetType.getType();
Expand Down
2 changes: 1 addition & 1 deletion source/ast/types/TypePrinter.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ void TypePrinter::visit(const VirtualInterfaceType& type, std::string_view) {

buffer->append(type.iface.getDefinition().name);

auto params = type.iface.body.parameters;
auto params = type.iface.body.getParameters();
if (!params.empty()) {
buffer->append("#(");
for (auto param : params) {
Expand Down
17 changes: 17 additions & 0 deletions tests/unittests/ast/ParameterTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1149,3 +1149,20 @@ endmodule
compilation.addSyntaxTree(tree);
NO_COMPILATION_ERRORS;
}

TEST_CASE("Parameter type evaluation loop regress") {
auto tree = SyntaxTree::fromText(R"(
module m #(parameter I=3, W=I+1)
(output [W-1:0] w);
assign w = {W{1'b0}};
endmodule
)");

Compilation compilation;
compilation.addSyntaxTree(tree);

for (auto param : compilation.getRoot().topInstances[0]->body.getParameters())
param->symbol.as<ParameterSymbol>().getValue();

NO_COMPILATION_ERRORS;
}
4 changes: 2 additions & 2 deletions tools/hier/hier.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,10 +84,10 @@ int main(int argc, char** argv) {
if (!instRegex.has_value() || std::regex_search(tmp_path, match, regex)) {
OS::print(fmt::format("Module=\"{}\" Instance=\"{}\" ",
type.getDefinition().name, tmp_path));
int size = type.body.parameters.size();
int size = type.body.getParameters().size();
if (size && params.value_or(false)) {
OS::print(fmt::format("Parameters: "));
for (auto p : type.body.parameters) {
for (auto p : type.body.getParameters()) {
std::string v;
size--;
if (p->symbol.kind == SymbolKind::Parameter)
Expand Down

3 comments on commit b9d5423

@udif
Copy link
Contributor

@udif udif commented on b9d5423 Apr 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't really dug into your fix, but from what I remember, the issue appeared only when both parameter definitions were part of a macro.
You testcase simply has one parameter depending on another.
What am I missing?

@MikePopoloski
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The macro was not necessary to reproduce the problem.

@udif
Copy link
Contributor

@udif udif commented on b9d5423 Apr 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please checkout the eversion before this change, and test the following two test cases:

`define PARAM_DECL parameter X=1,\
                           parameter I=3,\
                           parameter W=I+1\

module t
#(// Parameters
  `PARAM_DECL,
  parameter J=2,
  parameter Y=I+1,
  parameter Z=J+1
)
  (
output [Y-1:0]y,
output [Z-1:0]z
);
endmodule

Output:

% slang-hier --params ~/t13.v
Module="t" Instance="t" Parameters: X=1, I=3, W=4, J=2, Y=4, Z=3
Top level design units:
    t

Build succeeded: 0 errors, 0 warnings

No error even though both y and z depends on Y,Z that fulfill the bug requirements.

2nd Version:

module t
#(// Parameters
  parameter J=2,
  parameter Z=J+1
)
  (
output [Z-1:0]z
);
endmodule

Output:

% ~/bin/slang-hier --params ~/t14.v 
Module="t" Instance="t" Parameters: J=2, Z=3
Top level design units:
    t

../../../../../../../home/udif/t14.v:7:9: error: 'Z' recursively depends on its own definition
output [Z-1:0]z
        ^
../../../../../../../home/udif/t14.v:4:13: note: declared here
  parameter Z=J+1
            ^

Build failed: 1 error, 0 warnings

This time we do get an error without the macros.

My point is that the original #972 example should have failed on all 3 parameters, X,Y,Z, if it did not depend on the macro, and the 1st example above still doesn't fail despite fufilling the conditions for the error.

Please sign in to comment.