From 6ee01308f22bdbbad3ab6717e98d78917b84c43a Mon Sep 17 00:00:00 2001 From: "Emil J. Tywoniak" Date: Fri, 11 Jul 2025 00:33:01 +0200 Subject: [PATCH 1/7] dfflibmap: show dffe inference is broken by space ANDs --- tests/techmap/dfflibmap.lib | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/techmap/dfflibmap.lib b/tests/techmap/dfflibmap.lib index d0cd472c3..54a44a296 100644 --- a/tests/techmap/dfflibmap.lib +++ b/tests/techmap/dfflibmap.lib @@ -55,7 +55,7 @@ library(test) { cell (dffe) { area : 6; ff("IQ", "IQN") { - next_state : "(D&EN) | (IQ&!EN)"; + next_state : "(D EN) | (IQ !EN)"; clocked_on : "!CLK"; } pin(D) { From 4b1a8a3b667d133fc583ba40e20192f3f4e520da Mon Sep 17 00:00:00 2001 From: "Emil J. Tywoniak" Date: Fri, 11 Jul 2025 18:27:19 +0200 Subject: [PATCH 2/7] libparse: add LibertyExpression::str for testing --- passes/techmap/dfflibmap.cc | 1 + passes/techmap/libparse.cc | 41 +++++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/passes/techmap/dfflibmap.cc b/passes/techmap/dfflibmap.cc index f0b0ef20b..ae9498a9c 100644 --- a/passes/techmap/dfflibmap.cc +++ b/passes/techmap/dfflibmap.cc @@ -117,6 +117,7 @@ static bool parse_next_state(const LibertyAst *cell, const LibertyAst *attr, std // the next_state variable isn't just a pin name; perhaps this is an enable? auto helper = LibertyExpression::Lexer(expr); auto tree = LibertyExpression::parse(helper); + log_debug("liberty expression:\n%s\n", tree.str().c_str()); if (tree.kind == LibertyExpression::Kind::EMPTY) { if (!warned_cells.count(cell_name)) { diff --git a/passes/techmap/libparse.cc b/passes/techmap/libparse.cc index 85ed35ea1..0df7af347 100644 --- a/passes/techmap/libparse.cc +++ b/passes/techmap/libparse.cc @@ -306,6 +306,47 @@ bool LibertyExpression::eval(dict& values) { } return false; } + +std::string LibertyExpression::str(int indent) +{ + std::string prefix; + prefix = std::string(indent, ' '); + switch (kind) { + case AND: + prefix += "(and "; + break; + case OR: + prefix += "(or "; + break; + case NOT: + prefix += "(not "; + break; + case XOR: + prefix += "(xor "; + break; + case PIN: + prefix += "(pin \"" + name + "\""; + break; + case EMPTY: + prefix += "("; + break; + default: + log_assert(false); + } + size_t add_indent = prefix.length(); + bool first = true; + for (auto child : children) { + if (!first) { + prefix += "\n" + child.str(indent + add_indent); + } else { + prefix += child.str(0); + } + first = false; + } + prefix += ")"; + return prefix; +} + #endif int LibertyParser::lexer(std::string &str) From c6e1d461faa0e845fa2306ca9015aed0d37e83e6 Mon Sep 17 00:00:00 2001 From: "Emil J. Tywoniak" Date: Fri, 11 Jul 2025 23:09:30 +0200 Subject: [PATCH 3/7] libparse: support space ANDs --- passes/techmap/dfflibmap.cc | 5 +---- passes/techmap/libparse.cc | 43 ++++++++++++++++++++++++------------- passes/techmap/libparse.h | 3 +++ 3 files changed, 32 insertions(+), 19 deletions(-) diff --git a/passes/techmap/dfflibmap.cc b/passes/techmap/dfflibmap.cc index ae9498a9c..ade155791 100644 --- a/passes/techmap/dfflibmap.cc +++ b/passes/techmap/dfflibmap.cc @@ -92,9 +92,6 @@ static bool parse_next_state(const LibertyAst *cell, const LibertyAst *attr, std auto expr = attr->value; auto cell_name = cell->args[0]; - for (size_t pos = expr.find_first_of("\" \t"); pos != std::string::npos; pos = expr.find_first_of("\" \t")) - expr.erase(pos, 1); - // if this isn't an enable flop, the next_state variable is usually just the input pin name. if (expr[expr.size()-1] == '\'') { data_name = expr.substr(0, expr.size()-1); @@ -117,7 +114,7 @@ static bool parse_next_state(const LibertyAst *cell, const LibertyAst *attr, std // the next_state variable isn't just a pin name; perhaps this is an enable? auto helper = LibertyExpression::Lexer(expr); auto tree = LibertyExpression::parse(helper); - log_debug("liberty expression:\n%s\n", tree.str().c_str()); + // log_debug("liberty expression:\n%s\n", tree.str().c_str()); if (tree.kind == LibertyExpression::Kind::EMPTY) { if (!warned_cells.count(cell_name)) { diff --git a/passes/techmap/libparse.cc b/passes/techmap/libparse.cc index 0df7af347..957a529b9 100644 --- a/passes/techmap/libparse.cc +++ b/passes/techmap/libparse.cc @@ -163,6 +163,12 @@ void LibertyAst::dump(FILE *f, sieve &blacklist, sieve &whitelist, std::string i } #ifndef FILTERLIB + +// binary operators excluding ' ' +bool LibertyExpression::is_nice_binop(char c) { + return c == '*' || c == '&' || c == '^' || c == '+' || c == '|'; +} + // https://matklad.github.io/2020/04/13/simple-but-powerful-pratt-parsing.html LibertyExpression LibertyExpression::parse(Lexer &s, int min_prio) { if (s.empty()) @@ -201,15 +207,8 @@ LibertyExpression LibertyExpression::parse(Lexer &s, int min_prio) { while (true) { if (s.empty()) break; - - c = s.peek(); - while (isspace(c)) { - if (s.empty()) - return lhs; - s.next(); - c = s.peek(); - } + c = s.peek(); if (c == '\'') { // postfix NOT if (min_prio > 7) @@ -235,11 +234,27 @@ LibertyExpression LibertyExpression::parse(Lexer &s, int min_prio) { lhs = std::move(n); continue; - } else if (c == '&' || c == '*') { // infix AND - // technically space should be considered infix AND. it seems rare in practice. + } else if (c == '&' || c == '*' || isspace(c)) { // infix AND if (min_prio > 3) break; - s.next(); + + if (isspace(c)) { + // Rewind past this space and any further spaces + while (isspace(c) && !s.empty()) { + if (s.empty()) + return lhs; + s.next(); + c = s.peek(); + } + if (is_nice_binop(c)) { + // We found a real binop, so this space wasn't an AND + // and we just discard it as meaningless whitespace + continue; + } + } else { + // Rewind past this op + s.next(); + } auto rhs = parse(s, 4); auto n = LibertyExpression{}; @@ -310,7 +325,6 @@ bool LibertyExpression::eval(dict& values) { std::string LibertyExpression::str(int indent) { std::string prefix; - prefix = std::string(indent, ' '); switch (kind) { case AND: prefix += "(and "; @@ -337,10 +351,9 @@ std::string LibertyExpression::str(int indent) bool first = true; for (auto child : children) { if (!first) { - prefix += "\n" + child.str(indent + add_indent); - } else { - prefix += child.str(0); + prefix += "\n" + std::string(indent + add_indent, ' '); } + prefix += child.str(indent + add_indent); first = false; } prefix += ")"; diff --git a/passes/techmap/libparse.h b/passes/techmap/libparse.h index 8a2ab22e0..ee0f3db42 100644 --- a/passes/techmap/libparse.h +++ b/passes/techmap/libparse.h @@ -93,6 +93,9 @@ namespace Yosys static LibertyExpression parse(Lexer &s, int min_prio = 0); void get_pin_names(pool& names); bool eval(dict& values); + std::string str(int indent = 0); + private: + static bool is_nice_binop(char c); }; class LibertyInputStream { From bf1f236998954bbfd5d3695201c225e7a0ce6800 Mon Sep 17 00:00:00 2001 From: "Emil J. Tywoniak" Date: Fri, 11 Jul 2025 23:12:58 +0200 Subject: [PATCH 4/7] dfflibmap: add back tab and quote filters for good vibes --- passes/techmap/dfflibmap.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/passes/techmap/dfflibmap.cc b/passes/techmap/dfflibmap.cc index ade155791..409ac7865 100644 --- a/passes/techmap/dfflibmap.cc +++ b/passes/techmap/dfflibmap.cc @@ -92,6 +92,9 @@ static bool parse_next_state(const LibertyAst *cell, const LibertyAst *attr, std auto expr = attr->value; auto cell_name = cell->args[0]; + for (size_t pos = expr.find_first_of("\"\t"); pos != std::string::npos; pos = expr.find_first_of("\"\t")) + expr.erase(pos, 1); + // if this isn't an enable flop, the next_state variable is usually just the input pin name. if (expr[expr.size()-1] == '\'') { data_name = expr.substr(0, expr.size()-1); From e960428587b65c56db60ac80c24aab684f5b475f Mon Sep 17 00:00:00 2001 From: "Emil J. Tywoniak" Date: Tue, 15 Jul 2025 12:21:01 +0200 Subject: [PATCH 5/7] unit tests: fix run failure detection --- tests/unit/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/Makefile b/tests/unit/Makefile index 4c2848ea2..48635eb0d 100644 --- a/tests/unit/Makefile +++ b/tests/unit/Makefile @@ -24,7 +24,7 @@ $(OBJTEST)/%.o: $(basename $(subst $(OBJTEST),.,%)).cc .PHONY: prepare run-tests clean run-tests: $(TESTS) - $(subst Test ,Test; ,$^) + $(subst Test ,Test&& ,$^) prepare: mkdir -p $(addprefix $(BINTEST)/,$(TESTDIRS)) From 21e68ec9be2aa5d2827f5fe0a5ac4004fbf3f58e Mon Sep 17 00:00:00 2001 From: "Emil J. Tywoniak" Date: Tue, 15 Jul 2025 12:53:13 +0200 Subject: [PATCH 6/7] libparse: fix space ANDs --- passes/techmap/libparse.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/passes/techmap/libparse.cc b/passes/techmap/libparse.cc index 957a529b9..c6f87b60b 100644 --- a/passes/techmap/libparse.cc +++ b/passes/techmap/libparse.cc @@ -240,7 +240,7 @@ LibertyExpression LibertyExpression::parse(Lexer &s, int min_prio) { if (isspace(c)) { // Rewind past this space and any further spaces - while (isspace(c) && !s.empty()) { + while (isspace(c)) { if (s.empty()) return lhs; s.next(); @@ -257,6 +257,8 @@ LibertyExpression LibertyExpression::parse(Lexer &s, int min_prio) { } auto rhs = parse(s, 4); + if (rhs.kind == EMPTY) + continue; auto n = LibertyExpression{}; n.kind = Kind::AND; n.children.push_back(lhs); From c7a3abbcc4c09d4b6681f5b467dbe8cf62f6ebda Mon Sep 17 00:00:00 2001 From: "Emil J. Tywoniak" Date: Tue, 15 Jul 2025 12:53:30 +0200 Subject: [PATCH 7/7] libparse: LibertyExpression unit test --- tests/unit/techmap/libparseTest.cc | 88 ++++++++++++++++++++++++++++++ 1 file changed, 88 insertions(+) create mode 100644 tests/unit/techmap/libparseTest.cc diff --git a/tests/unit/techmap/libparseTest.cc b/tests/unit/techmap/libparseTest.cc new file mode 100644 index 000000000..b58e55bf2 --- /dev/null +++ b/tests/unit/techmap/libparseTest.cc @@ -0,0 +1,88 @@ +#include +#include "passes/techmap/libparse.h" + +YOSYS_NAMESPACE_BEGIN + +namespace RTLIL { + + class TechmapLibparseTest : public testing::Test { + protected: + TechmapLibparseTest() { + if (log_files.empty()) log_files.emplace_back(stdout); + } + void checkAll(std::initializer_list expressions, std::string expected) { + for (const auto& e : expressions) { + auto helper = LibertyExpression::Lexer(e); + auto tree_s = LibertyExpression::parse(helper).str(); + EXPECT_EQ(tree_s, expected); + } + } + }; + TEST_F(TechmapLibparseTest, LibertyExpressionSpace) + { + checkAll({ + "x", + "x ", + " x", + " x ", + " x ", + }, "(pin \"x\")"); + + checkAll({ + "x y", + " x y ", + "x (y)", + " x (y) ", + "(x) y", + " (x) y ", + + "x & y", + "x&y", + "x &y", + "x& y", + " x&y ", + "x & (y)", + "x&(y)", + "x &(y)", + "x& (y)", + " x&(y) ", + "(x) & y", + "(x)&y", + "(x) &y", + "(x)& y", + " (x)&y ", + }, "(and (pin \"x\")\n" + " (pin \"y\"))" + ); + + checkAll({ + "x ^ y", + "x^y", + "x ^y", + "x^ y", + " x^y ", + }, "(xor (pin \"x\")\n" + " (pin \"y\"))" + ); + checkAll({ + "x !y", + " x !y ", + "x & !y", + "x&!y", + "x &!y", + "x& !y", + " x&!y ", + "x y'", + " x y' ", + "x & y'", + "x&y'", + "x &y'", + "x& y'", + " x&y' ", + }, "(and (pin \"x\")\n" + " (not (pin \"y\")))" + ); + } +} + +YOSYS_NAMESPACE_END