Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

write_verilog: don't emit code with dangling else related to wrong condition #4150

Merged
merged 2 commits into from
Jan 24, 2024
Merged

write_verilog: don't emit code with dangling else related to wrong condition #4150

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
027e410
write_verilog: don't emit code with dangling else related to wrong co…
whitequark Jan 23, 2024
e6a9e72
Test roundtripping some processes to Verilog and back
povik Jan 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 32 additions & 30 deletions backends/verilog/verilog_backend.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1949,13 +1949,8 @@ void dump_conn(std::ostream &f, std::string indent, const RTLIL::SigSpec &left,

void dump_proc_switch(std::ostream &f, std::string indent, RTLIL::SwitchRule *sw);

void dump_case_body(std::ostream &f, std::string indent, RTLIL::CaseRule *cs, bool omit_trailing_begin = false)
void dump_case_actions(std::ostream &f, std::string indent, RTLIL::CaseRule *cs)
{
int number_of_stmts = cs->switches.size() + cs->actions.size();

if (!omit_trailing_begin && number_of_stmts >= 2)
f << stringf("%s" "begin\n", indent.c_str());

for (auto it = cs->actions.begin(); it != cs->actions.end(); ++it) {
if (it->first.size() == 0)
continue;
Expand All @@ -1965,15 +1960,6 @@ void dump_case_body(std::ostream &f, std::string indent, RTLIL::CaseRule *cs, bo
dump_sigspec(f, it->second);
f << stringf(";\n");
}

for (auto it = cs->switches.begin(); it != cs->switches.end(); ++it)
dump_proc_switch(f, indent + " ", *it);

if (!omit_trailing_begin && number_of_stmts == 0)
f << stringf("%s /* empty */;\n", indent.c_str());

if (omit_trailing_begin || number_of_stmts >= 2)
f << stringf("%s" "end\n", indent.c_str());
}

bool dump_proc_switch_ifelse(std::ostream &f, std::string indent, RTLIL::SwitchRule *sw)
Expand All @@ -1996,36 +1982,52 @@ bool dump_proc_switch_ifelse(std::ostream &f, std::string indent, RTLIL::SwitchR
}
}

dump_attributes(f, indent, sw->attributes);
Copy link
Member

Choose a reason for hiding this comment

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

So this is something we were missing before, unrelated to the fix

Copy link
Member Author

Choose a reason for hiding this comment

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

We were dumping slightly different attributes (for case rules and not switch itself), the switch is probably better.

f << indent;
auto sig_it = sw->signal.begin();
for (auto it = sw->cases.begin(); it != sw->cases.end(); ++it, ++sig_it) {
bool had_newline = true;
if (it != sw->cases.begin()) {
if ((*it)->compare.empty()) {
f << indent << "else\n";
had_newline = true;
} else {
f << indent << "else ";
had_newline = false;
}
if ((*it)->compare.empty())
f << " else begin\n";
else
f << " else ";
}
if (!(*it)->compare.empty()) {
if (!(*it)->attributes.empty()) {
if (!had_newline)
f << "\n" << indent;
dump_attributes(f, "", (*it)->attributes, "\n" + indent);
}
f << stringf("if (");
dump_sigspec(f, *sig_it);
f << stringf(")\n");
f << stringf(") begin\n");
}
dump_case_body(f, indent, *it);

dump_case_actions(f, indent, (*it));
for (auto it2 = (*it)->switches.begin(); it2 != (*it)->switches.end(); ++it2)
dump_proc_switch(f, indent + " ", *it2);

f << indent << "end";
if ((*it)->compare.empty())
break;
}
f << "\n";
return true;
}

void dump_case_body(std::ostream &f, std::string indent, RTLIL::CaseRule *cs, bool omit_trailing_begin = false)
{
int number_of_stmts = cs->switches.size() + cs->actions.size();

if (!omit_trailing_begin && number_of_stmts >= 2)
f << stringf("%s" "begin\n", indent.c_str());

dump_case_actions(f, indent, cs);
for (auto it = cs->switches.begin(); it != cs->switches.end(); ++it)
dump_proc_switch(f, indent + " ", *it);

if (!omit_trailing_begin && number_of_stmts == 0)
f << stringf("%s /* empty */;\n", indent.c_str());

if (omit_trailing_begin || number_of_stmts >= 2)
f << stringf("%s" "end\n", indent.c_str());
}

void dump_proc_switch(std::ostream &f, std::string indent, RTLIL::SwitchRule *sw)
{
if (sw->signal.size() == 0) {
Expand Down
65 changes: 65 additions & 0 deletions tests/verilog/roundtrip_proc.ys
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
# Test "casez to if/elif/else conversion" in backend

read_verilog <<EOT
module top(a, b, c, out);
input wire a, b, c;
output reg [3:0] out;
always @(*) begin
casez ({c, b, a})
3'b??1: begin
out = 0;
end
3'b?1?: begin
out = 1;
end
3'b1??: begin
out = 2;
end
default: begin
out = 3;
end
endcase
end
endmodule
EOT
write_verilog roundtrip_proc_1.v
design -stash gold
read_verilog roundtrip_proc_1.v
design -stash gate
design -copy-from gold -as gold top
design -copy-from gate -as gate top
prep
miter -equiv -flatten -make_assert gold gate miter
hierarchy -top miter
sat -prove-asserts -verify

design -reset
read_verilog <<EOT
module top(a, b, c, out);
input wire a, b, c;
output reg [3:0] out;
always @(*) begin
out <= 0;
if (a) begin
if (b)
out <= 1;
end else begin
if (c)
out <= 2;
else
out <= 3;
end
end
endmodule
EOT
proc_clean
write_verilog roundtrip_proc_2.v
design -stash gold
read_verilog roundtrip_proc_2.v
design -stash gate
design -copy-from gold -as gold top
design -copy-from gate -as gate top
prep
miter -equiv -flatten -make_assert gold gate miter
hierarchy -top miter
sat -prove-asserts -verify
Loading