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

Improved sim handling of $display/$print #3963

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
+108 −42
Draft

Improved sim handling of $display/$print #3963

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0e92cc1
sim: Include $display output in JSON summary
jix Sep 22, 2023
8c89b46
fmt: Allow non-constant $display calls in initial blocks
jix Sep 22, 2023
1dbd3cc
fmt, sim: Hierarchical $display %m output and dynamic %t output
jix Sep 22, 2023
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
2 changes: 1 addition & 1 deletion frontends/ast/ast.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,7 @@ namespace AST
bool is_simple_const_expr();

// helper for parsing format strings
Fmt processFormat(int stage, bool sformat_like, int default_base = 10, size_t first_arg_at = 0);
Fmt processFormat(int stage, bool sformat_like, int default_base = 10, size_t first_arg_at = 0, bool required = true);

bool is_recursive_function() const;
std::pair<AstNode*, AstNode*> get_tern_choice();
Expand Down
14 changes: 10 additions & 4 deletions frontends/ast/simplify.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ void AstNode::fixup_hierarchy_flags(bool force_descend)

// Process a format string and arguments for $display, $write, $sprintf, etc

Fmt AstNode::processFormat(int stage, bool sformat_like, int default_base, size_t first_arg_at) {
Fmt AstNode::processFormat(int stage, bool sformat_like, int default_base, size_t first_arg_at, bool required) {
std::vector<VerilogFmtArg> args;
for (size_t index = first_arg_at; index < children.size(); index++) {
AstNode *node_arg = children[index];
Expand All @@ -156,6 +156,9 @@ Fmt AstNode::processFormat(int stage, bool sformat_like, int default_base, size_
arg.type = VerilogFmtArg::INTEGER;
arg.sig = node_arg->bitsAsConst();
arg.signed_ = node_arg->is_signed;
} else if (!required) {
log_file_info(filename, location.first_line, "Skipping system task `%s' with non-constant argument at position %zu.\n", str.c_str(), index + 1);
whitequark marked this conversation as resolved.
Show resolved Hide resolved
return Fmt();
} else {
log_file_error(filename, location.first_line, "Failed to evaluate system task `%s' with non-constant argument at position %zu.\n", str.c_str(), index + 1);
}
Expand Down Expand Up @@ -1063,10 +1066,13 @@ bool AstNode::simplify(bool const_fold, int stage, int width_hint, bool sign_hin
default_base = 16;

// when $display()/$write() functions are used in an initial block, print them during synthesis
Fmt fmt = processFormat(stage, /*sformat_like=*/false, default_base);
Fmt fmt = processFormat(stage, /*sformat_like=*/false, default_base, /*first_arg_at=*/0, /*required=*/false);
if (str.substr(0, 8) == "$display")
fmt.append_string("\n");
log("%s", fmt.render().c_str());
log("%s", fmt.render(unescape_id(current_module->name).c_str()).c_str());
for (auto node : children)
while (node->simplify(true, stage, -1, false)) {}
return false;
} else {
// when $display()/$write() functions are used in an always block, simplify the expressions and
// convert them to a special cell later in genrtlil
Expand Down Expand Up @@ -3738,7 +3744,7 @@ skip_dynamic_range_lvalue_expansion:;

if (str == "\\$sformatf") {
Fmt fmt = processFormat(stage, /*sformat_like=*/true);
newNode = AstNode::mkconst_str(fmt.render());
newNode = AstNode::mkconst_str(fmt.render(unescape_id(current_module->name)));
goto apply_newNode;
}

Expand Down
69 changes: 49 additions & 20 deletions kernel/fmt.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,8 @@ void Fmt::parse_rtlil(const RTLIL::Cell *cell) {
} else if (fmt[i] == 'r') {
part.type = FmtPart::TIME;
part.realtime = true;
} else if (fmt[i] == 'm') {
part.type = FmtPart::HIERNAME;
} else {
log_assert(false && "Unexpected character in format substitution");
}
Expand Down Expand Up @@ -173,6 +175,7 @@ void Fmt::emit_rtlil(RTLIL::Cell *cell) const {
break;

case FmtPart::TIME:
case FmtPart::HIERNAME:
log_assert(part.sig.size() == 0);
YS_FALLTHROUGH
case FmtPart::CHARACTER:
Expand Down Expand Up @@ -210,6 +213,8 @@ void Fmt::emit_rtlil(RTLIL::Cell *cell) const {
fmt += 'r';
else
fmt += 't';
} else if (part.type == FmtPart::HIERNAME) {
fmt += 'm';
} else log_abort();
fmt += '}';
break;
Expand Down Expand Up @@ -340,9 +345,6 @@ void Fmt::parse_verilog(const std::vector<VerilogFmtArg> &args, bool sformat_lik
} else if (fmt.substr(i, 2) == "%l" || fmt.substr(i, 2) == "%L") {
i++;
part.str += module_name.str();
} else if (fmt.substr(i, 2) == "%m" || fmt.substr(i, 2) == "%M") {
i++;
part.str += module_name.str();
} else {
if (!part.str.empty()) {
part.type = FmtPart::STRING;
Expand All @@ -353,12 +355,6 @@ void Fmt::parse_verilog(const std::vector<VerilogFmtArg> &args, bool sformat_lik
log_file_error(fmtarg->filename, fmtarg->first_line, "System task `%s' called with incomplete format specifier in argument %zu.\n", task_name.c_str(), fmtarg - args.begin() + 1);
}

if (++arg == args.end()) {
log_file_error(fmtarg->filename, fmtarg->first_line, "System task `%s' called with fewer arguments than the format specifiers in argument %zu require.\n", task_name.c_str(), fmtarg - args.begin() + 1);
}
part.sig = arg->sig;
part.signed_ = arg->signed_;

for (; i < fmt.size(); i++) {
if (fmt[i] == '-') {
// left justify; not in IEEE 1800-2017 or verilator but iverilog has it
Expand Down Expand Up @@ -408,14 +404,11 @@ void Fmt::parse_verilog(const std::vector<VerilogFmtArg> &args, bool sformat_lik
// %10s and %010s not fully defined in IEEE 1800-2017 and do the same thing in iverilog
part.padding = ' ';
} else if (fmt[i] == 't' || fmt[i] == 'T') {
if (arg->type == VerilogFmtArg::TIME) {
part.type = FmtPart::TIME;
part.realtime = arg->realtime;
if (!has_width && !has_leading_zero)
part.width = 20;
} else {
log_file_error(fmtarg->filename, fmtarg->first_line, "System task `%s' called with format character `%c' in argument %zu, but the argument is not $time or $realtime.\n", task_name.c_str(), fmt[i], fmtarg - args.begin() + 1);
}
part.type = FmtPart::TIME;
if (!has_width && !has_leading_zero)
part.width = 20;
} else if (fmt[i] == 'm' || fmt[i] == 'M') {
part.type = FmtPart::HIERNAME;
} else {
log_file_error(fmtarg->filename, fmtarg->first_line, "System task `%s' called with unrecognized format character `%c' in argument %zu.\n", task_name.c_str(), fmt[i], fmtarg - args.begin() + 1);
}
Expand All @@ -425,6 +418,22 @@ void Fmt::parse_verilog(const std::vector<VerilogFmtArg> &args, bool sformat_lik
log_file_error(fmtarg->filename, fmtarg->first_line, "System task `%s' called with incomplete format specifier in argument %zu.\n", task_name.c_str(), fmtarg - args.begin() + 1);
}

if (part.type != FmtPart::HIERNAME) {
if (++arg == args.end()) {
log_file_error(fmtarg->filename, fmtarg->first_line, "System task `%s' called with fewer arguments than the format specifiers in argument %zu require.\n", task_name.c_str(), fmtarg - args.begin() + 1);
}
part.sig = arg->sig;
part.signed_ = arg->signed_;

if (part.type == VerilogFmtArg::TIME) {
if (arg->type == VerilogFmtArg::TIME) {
part.realtime = arg->realtime;
} else {
log_file_error(fmtarg->filename, fmtarg->first_line, "System task `%s' called with format character `%c' in argument %zu, but the argument is not $time or $realtime.\n", task_name.c_str(), fmt[i], fmtarg - args.begin() + 1);
}
}
}

if (part.padding == '\0')
part.padding = (has_leading_zero && part.justify == FmtPart::RIGHT) ? '0' : ' ';

Expand Down Expand Up @@ -551,6 +560,23 @@ std::vector<VerilogFmtArg> Fmt::emit_verilog() const
break;
}

case FmtPart::HIERNAME: {
VerilogFmtArg arg;
arg.type = VerilogFmtArg::HIERNAME;
args.push_back(arg);
fmt.str += '%';
if (part.plus)
Copy link
Member

Choose a reason for hiding this comment

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

Are these modifiers actually valid for %m?

Copy link
Member Author

Choose a reason for hiding this comment

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

AFAICT the spec is not clear on this and IIRC there was at least some inconsistency between other implementations. In places where the spec is unclear I think it would make sense to support whatever other commonly used open-source tools like iverilog support as long as that's a reasonable interpretation of the spec (although I don't remember what exactly iverilog does here and in general I'd also be fine with not supporting any modifiers for %m).

Copy link
Member

Choose a reason for hiding this comment

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

In places where the spec is unclear I think it would make sense to support whatever other commonly used open-source tools like iverilog support as long as that's a reasonable interpretation of the spec (although I don't remember what exactly iverilog does here and in general I'd also be fine with not supporting any modifiers for %m).

I agree with this stance. I just want to make sure that whichever modifiers we implement here actually match other tools, if we do support them.

fmt.str += '+';
if (part.justify == FmtPart::LEFT)
fmt.str += '-';
log_assert(part.padding == ' ' || part.padding == '0');
if (part.padding == '0' && part.width > 0)
fmt.str += '0';
fmt.str += std::to_string(part.width);
fmt.str += 'm';
break;
}

default: log_abort();
}
}
Expand Down Expand Up @@ -637,7 +663,7 @@ void Fmt::emit_cxxrtl(std::ostream &f, std::function<void(const RTLIL::SigSpec &
}
}

std::string Fmt::render() const
std::string Fmt::render(const std::string &hiername, int time) const
{
std::string str;

Expand All @@ -649,7 +675,8 @@ std::string Fmt::render() const

case FmtPart::INTEGER:
case FmtPart::TIME:
case FmtPart::CHARACTER: {
case FmtPart::CHARACTER:
case FmtPart::HIERNAME: {
std::string buf;
if (part.type == FmtPart::INTEGER) {
RTLIL::Const value = part.sig.as_const();
Expand Down Expand Up @@ -734,7 +761,9 @@ std::string Fmt::render() const
buf = part.sig.as_const().decode_string();
} else if (part.type == FmtPart::TIME) {
// We only render() during initial, so time is always zero.
Copy link
Member

Choose a reason for hiding this comment

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

This comment no longer applies.

buf = "0";
buf = stringf("%d", time);
} else if (part.type == FmtPart::HIERNAME) {
buf = hiername;
}

log_assert(part.width == 0 || part.padding != '\0');
Expand Down
10 changes: 6 additions & 4 deletions kernel/fmt.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,10 @@ YOSYS_NAMESPACE_BEGIN
// $display("foo %d bar %01x", 4'b0, $signed(2'b11))
struct VerilogFmtArg {
enum {
STRING = 0,
INTEGER = 1,
TIME = 2,
STRING = 0,
INTEGER = 1,
TIME = 2,
HIERNAME = 3,
} type;

// All types
Expand All @@ -56,6 +57,7 @@ struct FmtPart {
INTEGER = 1,
CHARACTER = 2,
TIME = 3,
HIERNAME = 4,
} type;

// STRING type
Expand Down Expand Up @@ -95,7 +97,7 @@ struct Fmt {

void emit_cxxrtl(std::ostream &f, std::function<void(const RTLIL::SigSpec &)> emit_sig) const;

std::string render() const;
std::string render(const std::string &hiername, int time = 0) const;

private:
void apply_verilog_automatic_sizing_and_add(FmtPart &part);
Expand Down
55 changes: 42 additions & 13 deletions passes/sat/sim.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,17 @@ struct TriggeredAssertion {
{ }
};

struct DisplayOutput {
int step;
SimInstance *instance;
Cell *cell;
std::string output;

DisplayOutput(int step, SimInstance *instance, Cell *cell, std::string output) :
step(step), instance(instance), cell(cell), output(output)
{ }
};

struct SimShared
{
bool debug = false;
Expand All @@ -109,7 +120,9 @@ struct SimShared
bool multiclock = false;
int next_output_id = 0;
int step = 0;
int trace_time = 0;
std::vector<TriggeredAssertion> triggered_assertions;
std::vector<DisplayOutput> display_output;
bool serious_asserts = false;
};

Expand Down Expand Up @@ -839,8 +852,9 @@ struct SimInstance
pos += part.sig.size();
}

std::string rendered = print.fmt.render();
std::string rendered = print.fmt.render(hiername(), shared->trace_time);
log("%s", rendered.c_str());
shared->display_output.emplace_back(shared->step, this, cell, rendered);
}

update_print:
Expand Down Expand Up @@ -1279,8 +1293,9 @@ struct SimWorker : SimShared
}
}

void update(bool gclk)
void update(bool gclk, int trace_time)
{
this->trace_time = trace_time;
if (gclk)
step += 1;

Expand Down Expand Up @@ -1356,7 +1371,7 @@ struct SimWorker : SimShared
set_inports(clock, State::Sx);
set_inports(clockn, State::Sx);

update(false);
update(false, 0);

register_output_step(0);

Expand All @@ -1369,7 +1384,7 @@ struct SimWorker : SimShared
set_inports(clock, State::S0);
set_inports(clockn, State::S1);

update(true);
update(true, 10*cycle + 5);
register_output_step(10*cycle + 5);

if (debug)
Expand All @@ -1385,7 +1400,7 @@ struct SimWorker : SimShared
set_inports(resetn, State::S1);
}

update(true);
update(true, 10*cycle + 10);
register_output_step(10*cycle + 10);
}

Expand Down Expand Up @@ -1497,7 +1512,7 @@ struct SimWorker : SimShared
initial = false;
}
if (did_something)
update(true);
update(true, time);
register_output_step(time);

bool status = top->checkSignals();
Expand Down Expand Up @@ -1641,12 +1656,12 @@ struct SimWorker : SimShared
set_inports(clock, State::S0);
set_inports(clockn, State::S1);
}
update(true);
update(true, 10*cycle);
register_output_step(10*cycle);
if (!multiclock && cycle) {
set_inports(clock, State::S0);
set_inports(clockn, State::S1);
update(true);
update(true, 10*cycle + 5);
register_output_step(10*cycle + 5);
}
cycle++;
Expand Down Expand Up @@ -1718,12 +1733,12 @@ struct SimWorker : SimShared
log("Simulating cycle %d.\n", cycle);
set_inports(clock, State::S1);
set_inports(clockn, State::S0);
update(true);
update(true, 10*cycle+0);
register_output_step(10*cycle+0);
if (!multiclock) {
set_inports(clock, State::S0);
set_inports(clockn, State::S1);
update(true);
update(true, 10*cycle+5);
register_output_step(10*cycle+5);
}
cycle++;
Expand Down Expand Up @@ -1963,7 +1978,7 @@ struct SimWorker : SimShared
if (debug)
log("Simulating non-active clock edge.\n");
set_yw_clocks(yw, hierarchy, false);
update(false);
update(false, 5);
register_output_step(5);
}
top->set_initstate_outputs(State::S0);
Expand All @@ -1976,14 +1991,14 @@ struct SimWorker : SimShared
if (cycle < GetSize(yw.steps))
set_yw_state(yw, hierarchy, cycle);
set_yw_clocks(yw, hierarchy, true);
update(true);
update(true, 10 * cycle);
register_output_step(10 * cycle);

if (!yw.clocks.empty()) {
if (debug)
log("Simulating non-active clock edge.\n");
set_yw_clocks(yw, hierarchy, false);
update(false);
update(false, 5 + 10 * cycle);
register_output_step(5 + 10 * cycle);
}
}
Expand Down Expand Up @@ -2020,6 +2035,20 @@ struct SimWorker : SimShared
json.end_object();
}
json.end_array();
json.name("display_output");
json.begin_array();
for (auto &output : display_output) {
json.begin_object();
json.entry("step", output.step);
json.entry("path", output.instance->witness_full_path(output.cell));
auto src = output.cell->get_string_attribute(ID::src);
if (!src.empty()) {
json.entry("src", src);
}
json.entry("output", output.output);
json.end_object();
}
json.end_array();
json.end_object();
}

Expand Down
Loading