Merge pull request #5219 from YosysHQ/emil/fix-liberty-expr-space

dfflibmap: fix dffe inference when spaces used for ANDs in liberty next_state

passes/techmap/dfflibmap.cc

@@ -92,7 +92,7 @@ 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"))
+	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.
@@ -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)) {

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())
@@ -204,13 +210,6 @@ LibertyExpression LibertyExpression::parse(Lexer &s, int min_prio) {
 
 		c = s.peek();
 
-		while (isspace(c)) {
-			if (s.empty())
-				return lhs;
-			s.next();
-			c = s.peek();
-		}
-
 		if (c == '\'') { // postfix NOT
 			if (min_prio > 7)
 				break;
@@ -235,13 +234,31 @@ 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;
+
+			if (isspace(c)) {
+				// Rewind past this space and any further spaces
+				while (isspace(c)) {
+					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);
+			if (rhs.kind == EMPTY)
+				continue;
 			auto n = LibertyExpression{};
 			n.kind = Kind::AND;
 			n.children.push_back(lhs);
@@ -306,6 +323,45 @@ bool LibertyExpression::eval(dict<std::string, bool>& values) {
 	}
 	return false;
 }
+
+std::string LibertyExpression::str(int indent)
+{
+	std::string prefix;
+	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" + std::string(indent + add_indent, ' ');
+		}
+		prefix += child.str(indent + add_indent);
+		first = false;
+	}
+	prefix += ")";
+	return prefix;
+}
+
 #endif
 
 int LibertyParser::lexer(std::string &str)

passes/techmap/libparse.h

@@ -93,6 +93,9 @@ namespace Yosys
 		static LibertyExpression parse(Lexer &s, int min_prio = 0);
 		void get_pin_names(pool<std::string>& names);
 		bool eval(dict<std::string, bool>& values);
+		std::string str(int indent = 0);
+	private:
+		static bool is_nice_binop(char c);
 	};
 
 	class LibertyInputStream {

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) {

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))

tests/unit/techmap/libparseTest.cc

@@ -0,0 +1,88 @@
+#include <gtest/gtest.h>
+#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<std::string> 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