sv: auto add nosync to certain always_comb local vars

If a local variable is always assigned before it is used, then adding
nosync prevents latches from being needlessly generated.

diff --git a/CHANGELOG b/CHANGELOG
index cd6c1ed2602d1f731c67b48819e26dfab873c920..c878b3c1cf1e1fb760752831d54888df11860c59 100644 (file)
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -20,6 +20,9 @@ Yosys 0.11 .. Yosys 0.12
       operations
     - Fixed static size casts ignoring expression signedness
     - Fixed static size casts not extending unbased unsized literals
+    - Added automatic `nosync` inference for local variables in `always_comb`
+      procedures which are always assigned before they are used to avoid errant
+      latch inference
 
  * New commands and options
     - Added "-genlib" option to "abc" pass

diff --git a/frontends/ast/simplify.cc b/frontends/ast/simplify.cc
index cb47e641aaab9e45704e2393203bceec093fd402..18b1e1e11a15b95777e395833fdd06056c90b98d 100644 (file)
--- a/frontends/ast/simplify.cc
+++ b/frontends/ast/simplify.cc
@@ -673,6 +673,118 @@ void add_wire_for_ref(const RTLIL::Wire *ref, const std::string &str)
        current_scope[str] = wire;
 }
 
+enum class IdentUsage {
+       NotReferenced, // target variable is neither read or written in the block
+       Assigned, // target variable is always assigned before use
+       SyncRequired, // target variable may be used before it has been assigned
+};
+
+// determines whether a local variable a block is always assigned before it is
+// used, meaning the nosync attribute can automatically be added to that
+// variable
+static IdentUsage always_asgn_before_use(const AstNode *node, const std::string &target)
+{
+       // This variable has been referenced before it has necessarily been assigned
+       // a value in this procedure.
+       if (node->type == AST_IDENTIFIER && node->str == target)
+               return IdentUsage::SyncRequired;
+
+       // For case statements (which are also used for if/else), we check each
+       // possible branch. If the variable is assigned in all branches, then it is
+       // assigned, and a sync isn't required. If it used before assignment in any
+       // branch, then a sync is required.
+       if (node->type == AST_CASE) {
+               bool all_defined = true;
+               bool any_used = false;
+               bool has_default = false;
+               for (const AstNode *child : node->children) {
+                       if (child->type == AST_COND && child->children.at(0)->type == AST_DEFAULT)
+                               has_default = true;
+                       IdentUsage nested = always_asgn_before_use(child, target);
+                       if (nested != IdentUsage::Assigned && child->type == AST_COND)
+                               all_defined = false;
+                       if (nested == IdentUsage::SyncRequired)
+                               any_used = true;
+               }
+               if (any_used)
+                       return IdentUsage::SyncRequired;
+               else if (all_defined && has_default)
+                       return IdentUsage::Assigned;
+               else
+                       return IdentUsage::NotReferenced;
+       }
+
+       // Check if this is an assignment to the target variable. For simplicity, we
+       // don't analyze sub-ranges of the variable.
+       if (node->type == AST_ASSIGN_EQ) {
+               const AstNode *ident = node->children.at(0);
+               if (ident->type == AST_IDENTIFIER && ident->str == target)
+                       return IdentUsage::Assigned;
+       }
+
+       for (const AstNode *child : node->children) {
+               IdentUsage nested = always_asgn_before_use(child, target);
+               if (nested != IdentUsage::NotReferenced)
+                       return nested;
+       }
+       return IdentUsage::NotReferenced;
+}
+
+static const std::string auto_nosync_prefix = "\\AutoNosync";
+
+// mark a local variable in an always_comb block for automatic nosync
+// consideration
+static void mark_auto_nosync(AstNode *block, const AstNode *wire)
+{
+       log_assert(block->type == AST_BLOCK);
+       log_assert(wire->type == AST_WIRE);
+       block->attributes[auto_nosync_prefix + wire->str] = AstNode::mkconst_int(1,
+                       false);
+}
+
+// check a procedural block for auto-nosync markings, remove them, and add
+// nosync to local variables as necessary
+static void check_auto_nosync(AstNode *node)
+{
+       std::vector<RTLIL::IdString> attrs_to_drop;
+       for (const auto& elem : node->attributes) {
+               // skip attributes that don't begin with the prefix
+               if (elem.first.compare(0, auto_nosync_prefix.size(),
+                                       auto_nosync_prefix.c_str()))
+                       continue;
+
+               // delete and remove the attribute once we're done iterating
+               attrs_to_drop.push_back(elem.first);
+
+               // find the wire based on the attribute
+               std::string wire_name = elem.first.substr(auto_nosync_prefix.size());
+               auto it = current_scope.find(wire_name);
+               if (it == current_scope.end())
+                       continue;
+
+               // analyze the usage of the local variable in this block
+               IdentUsage ident_usage = always_asgn_before_use(node, wire_name);
+               if (ident_usage != IdentUsage::Assigned)
+                       continue;
+
+               // mark the wire with `nosync`
+               AstNode *wire = it->second;
+               log_assert(wire->type == AST_WIRE);
+               wire->attributes[ID::nosync] = AstNode::mkconst_int(1, false);
+       }
+
+       // remove the attributes we've "consumed"
+       for (const RTLIL::IdString &str : attrs_to_drop) {
+               auto it = node->attributes.find(str);
+               delete it->second;
+               node->attributes.erase(it);
+       }
+
+       // check local variables in any nested blocks
+       for (AstNode *child : node->children)
+               check_auto_nosync(child);
+}
+
 // convert the AST into a simpler AST that has all parameters substituted by their
 // values, unrolled for-loops, expanded generate blocks, etc. when this function
 // is done with an AST it can be converted into RTLIL using genRTLIL().
@@ -980,6 +1092,11 @@ bool AstNode::simplify(bool const_fold, bool at_zero, bool in_lvalue, int stage,
                                }
                        }
                }
+
+               for (AstNode *child : children)
+                       if (child->type == AST_ALWAYS &&
+                                       child->attributes.count(ID::always_comb))
+                               check_auto_nosync(child);
        }
 
        // create name resolution entries for all objects with names
@@ -2224,6 +2341,16 @@ bool AstNode::simplify(bool const_fold, bool at_zero, bool in_lvalue, int stage,
        {
                expand_genblock(str + ".");
 
+               // if this is an autonamed block is in an always_comb
+               if (current_always && current_always->attributes.count(ID::always_comb)
+                               && str.at(0) == '$')
+                       // track local variables in this block so we can consider adding
+                       // nosync once the block has been fully elaborated
+                       for (AstNode *child : children)
+                               if (child->type == AST_WIRE &&
+                                               !child->attributes.count(ID::nosync))
+                                       mark_auto_nosync(this, child);
+
                std::vector<AstNode*> new_children;
                for (size_t i = 0; i < children.size(); i++)
                        if (children[i]->type == AST_WIRE || children[i]->type == AST_MEMORY || children[i]->type == AST_PARAMETER || children[i]->type == AST_LOCALPARAM || children[i]->type == AST_TYPEDEF) {

diff --git a/tests/verilog/always_comb_latch_1.ys b/tests/verilog/always_comb_latch_1.ys
new file mode 100644 (file)
index 0000000..c98c79f
--- /dev/null
+++ b/tests/verilog/always_comb_latch_1.ys
@@ -0,0 +1,13 @@
+read_verilog -sv <<EOF
+module top;
+logic x;
+always_comb begin
+    logic y;
+    if (x)
+        y = 1;
+    x = y;
+end
+endmodule
+EOF
+logger -expect error "^Latch inferred for signal `\\top\.\$unnamed_block\$1\.y' from always_comb process" 1
+proc

diff --git a/tests/verilog/always_comb_latch_2.ys b/tests/verilog/always_comb_latch_2.ys
new file mode 100644 (file)
index 0000000..567205a
--- /dev/null
+++ b/tests/verilog/always_comb_latch_2.ys
@@ -0,0 +1,15 @@
+read_verilog -sv <<EOF
+module top;
+logic x;
+always_comb begin
+    logic y;
+    if (x)
+        x = 1;
+    else
+        y = 1;
+    x = y;
+end
+endmodule
+EOF
+logger -expect error "^Latch inferred for signal `\\top\.\$unnamed_block\$1\.y' from always_comb process" 1
+proc

diff --git a/tests/verilog/always_comb_latch_3.ys b/tests/verilog/always_comb_latch_3.ys
new file mode 100644 (file)
index 0000000..b9b028a
--- /dev/null
+++ b/tests/verilog/always_comb_latch_3.ys
@@ -0,0 +1,20 @@
+read_verilog -sv <<EOF
+module top;
+logic x;
+logic z;
+assign z = 1'b1;
+always_comb begin
+    logic y;
+    case (x)
+    1'b0:
+        y = 1;
+    endcase
+    if (z)
+        x = y;
+    else
+        x = 1'b0;
+end
+endmodule
+EOF
+logger -expect error "^Latch inferred for signal `\\top\.\$unnamed_block\$1\.y' from always_comb process" 1
+proc

diff --git a/tests/verilog/always_comb_latch_4.ys b/tests/verilog/always_comb_latch_4.ys
new file mode 100644 (file)
index 0000000..46b7880
--- /dev/null
+++ b/tests/verilog/always_comb_latch_4.ys
@@ -0,0 +1,17 @@
+read_verilog -sv <<EOF
+module top;
+parameter AVOID_LATCH = 0;
+logic x, z;
+assign z = 1'b1;
+always_comb begin
+    logic y;
+    if (z)
+        y = 0;
+    for (int i = 1; i == AVOID_LATCH; i++)
+        y = 1;
+    x = z ? y : 1'b0;
+end
+endmodule
+EOF
+logger -expect error "^Latch inferred for signal `\\top\.\$unnamed_block\$3\.y' from always_comb process" 1
+proc

diff --git a/tests/verilog/always_comb_nolatch_1.ys b/tests/verilog/always_comb_nolatch_1.ys
new file mode 100644 (file)
index 0000000..4d1952b
--- /dev/null
+++ b/tests/verilog/always_comb_nolatch_1.ys
@@ -0,0 +1,16 @@
+read_verilog -sv <<EOF
+module top;
+logic [4:0] x;
+logic z;
+assign z = 1'b1;
+always_comb begin
+    x = '0;
+    if (z) begin
+        for (int i = 0; i < 5; i++) begin
+            x[i] = 1'b1;
+        end
+    end
+end
+endmodule
+EOF
+proc

diff --git a/tests/verilog/always_comb_nolatch_2.ys b/tests/verilog/always_comb_nolatch_2.ys
new file mode 100644 (file)
index 0000000..2ec6ca0
--- /dev/null
+++ b/tests/verilog/always_comb_nolatch_2.ys
@@ -0,0 +1,17 @@
+read_verilog -sv <<EOF
+module top;
+logic [4:0] x;
+logic z;
+assign z = 1'b1;
+always_comb begin
+    x = '0;
+    if (z) begin
+        int i;
+        for (i = 0; i < 5; i++) begin
+            x[i] = 1'b1;
+        end
+    end
+end
+endmodule
+EOF
+proc

diff --git a/tests/verilog/always_comb_nolatch_3.ys b/tests/verilog/always_comb_nolatch_3.ys
new file mode 100644 (file)
index 0000000..33f9833
--- /dev/null
+++ b/tests/verilog/always_comb_nolatch_3.ys
@@ -0,0 +1,21 @@
+read_verilog -sv <<EOF
+module top;
+logic x;
+logic z;
+assign z = 1'b1;
+always_comb begin
+    logic y;
+    case (x)
+    1'b0:
+        y = 1;
+    default:
+        y = 0;
+    endcase
+    if (z)
+        x = y;
+    else
+        x = 1'b0;
+end
+endmodule
+EOF
+proc

diff --git a/tests/verilog/always_comb_nolatch_4.ys b/tests/verilog/always_comb_nolatch_4.ys
new file mode 100644 (file)
index 0000000..bc29b27
--- /dev/null
+++ b/tests/verilog/always_comb_nolatch_4.ys
@@ -0,0 +1,16 @@
+read_verilog -sv <<EOF
+module top;
+parameter AVOID_LATCH = 1;
+logic x, z;
+assign z = 1'b1;
+always_comb begin
+    logic y;
+    if (z)
+        y = 0;
+    for (int i = 1; i == AVOID_LATCH; i++)
+        y = 1;
+    x = z ? y : 1'b0;
+end
+endmodule
+EOF
+proc