Merge pull request #2317 from zachjs/expand-genblock

Fix generate scoping issues

frontends/ast/ast.h

@@ -250,7 +250,7 @@ namespace AST
 		// it also sets the id2ast pointers so that identifier lookups are fast in genRTLIL()
 		bool simplify(bool const_fold, bool at_zero, bool in_lvalue, int stage, int width_hint, bool sign_hint, bool in_param);
 		AstNode *readmem(bool is_readmemh, std::string mem_filename, AstNode *memory, int start_addr, int finish_addr, bool unconditional_init);
-		void expand_genblock(std::string index_var, std::string prefix, std::map<std::string, std::string> &name_map);
+		void expand_genblock(std::string index_var, std::string prefix, std::map<std::string, std::string> &name_map, bool original_scope = true);
 		void replace_ids(const std::string &prefix, const std::map<std::string, std::string> &rules);
 		void mem2reg_as_needed_pass1(dict<AstNode*, pool<std::string>> &mem2reg_places,
 				dict<AstNode*, uint32_t> &mem2reg_flags, dict<AstNode*, uint32_t> &proc_flags, uint32_t &status_flags);

frontends/ast/simplify.cc

@@ -3711,8 +3711,11 @@ AstNode *AstNode::readmem(bool is_readmemh, std::string mem_filename, AstNode *m
 }
 
 // annotate the names of all wires and other named objects in a generate block
-void AstNode::expand_genblock(std::string index_var, std::string prefix, std::map<std::string, std::string> &name_map)
+void AstNode::expand_genblock(std::string index_var, std::string prefix, std::map<std::string, std::string> &name_map, bool original_scope)
 {
+	// `original_scope` defaults to false, and is used to prevent the premature
+	// prefixing of items in named sub-blocks
+
 	if (!index_var.empty() && type == AST_IDENTIFIER && str == index_var) {
 		if (children.empty()) {
 			current_scope[index_var]->children[0]->cloneInto(this);
@@ -3725,53 +3728,85 @@ void AstNode::expand_genblock(std::string index_var, std::string prefix, std::ma
 		}
 	}
 
-	if ((type == AST_IDENTIFIER || type == AST_FCALL || type == AST_TCALL || type == AST_WIRETYPE) && name_map.count(str) > 0)
+	if (type == AST_IDENTIFIER || type == AST_FCALL || type == AST_TCALL || type == AST_WIRETYPE) {
-		str = name_map[str];
+		if (name_map.count(str) > 0) {
+			str = name_map[str];
+		} else {
+			// remap the prefix of this ident if it is a local generate scope
+			size_t pos = str.rfind('.');
+			if (pos != std::string::npos) {
+				std::string existing_prefix = str.substr(0, pos);
+				if (name_map.count(existing_prefix) > 0) {
+					str = name_map[existing_prefix] + str.substr(pos);
+				}
+			}
+		}
+	}
 
 	std::map<std::string, std::string> backup_name_map;
 
+	auto prefix_node = [&](AstNode* child) {
+		if (backup_name_map.size() == 0)
+			backup_name_map = name_map;
+
+		// if within a nested scope
+		if (!original_scope) {
+			// this declaration shadows anything in the parent scope(s)
+			name_map[child->str] = child->str;
+			return;
+		}
+
+		std::string new_name = prefix[0] == '\\' ? prefix.substr(1) : prefix;
+		size_t pos = child->str.rfind('.');
+		if (pos == std::string::npos)
+			pos = child->str[0] == '\\' && prefix[0] == '\\' ? 1 : 0;
+		else
+			pos = pos + 1;
+		new_name = child->str.substr(0, pos) + new_name + child->str.substr(pos);
+		if (new_name[0] != '$' && new_name[0] != '\\')
+			new_name = prefix[0] + new_name;
+
+		name_map[child->str] = new_name;
+		if (child->type == AST_FUNCTION)
+			replace_result_wire_name_in_function(child, child->str, new_name);
+		else
+			child->str = new_name;
+		current_scope[new_name] = child;
+	};
+
 	for (size_t i = 0; i < children.size(); i++) {
 		AstNode *child = children[i];
-		if (child->type == AST_WIRE || child->type == AST_MEMORY || child->type == AST_PARAMETER || child->type == AST_LOCALPARAM ||
+
-				child->type == AST_FUNCTION || child->type == AST_TASK || child->type == AST_CELL || child->type == AST_TYPEDEF || child->type == AST_ENUM_ITEM) {
+		switch (child->type) {
-			if (backup_name_map.size() == 0)
+		case AST_WIRE:
-				backup_name_map = name_map;
+		case AST_MEMORY:
-			std::string new_name = prefix[0] == '\\' ? prefix.substr(1) : prefix;
+		case AST_PARAMETER:
-			size_t pos = child->str.rfind('.');
+		case AST_LOCALPARAM:
-			if (pos == std::string::npos)
+		case AST_FUNCTION:
-				pos = child->str[0] == '\\' && prefix[0] == '\\' ? 1 : 0;
+		case AST_TASK:
-			else
+		case AST_CELL:
-				pos = pos + 1;
+		case AST_TYPEDEF:
-			new_name = child->str.substr(0, pos) + new_name + child->str.substr(pos);
+		case AST_ENUM_ITEM:
-			if (new_name[0] != '$' && new_name[0] != '\\')
+		case AST_GENVAR:
-				new_name = prefix[0] + new_name;
+			prefix_node(child);
-			name_map[child->str] = new_name;
+			break;
-			if (child->type == AST_FUNCTION)
+
-				replace_result_wire_name_in_function(child, child->str, new_name);
+		case AST_BLOCK:
-			else
+		case AST_GENBLOCK:
-				child->str = new_name;
+			if (!child->str.empty())
-			current_scope[new_name] = child;
+				prefix_node(child);
-		}
+			break;
-		if (child->type == AST_ENUM){
+
+		case AST_ENUM:
 			current_scope[child->str] = child;
 			for (auto enode : child->children){
 				log_assert(enode->type == AST_ENUM_ITEM);
-				if (backup_name_map.size() == 0)
+				prefix_node(enode);
-					backup_name_map = name_map;
-				std::string new_name = prefix[0] == '\\' ? prefix.substr(1) : prefix;
-				size_t pos = enode->str.rfind('.');
-				if (pos == std::string::npos)
-					pos = enode->str[0] == '\\' && prefix[0] == '\\' ? 1 : 0;
-				else
-					pos = pos + 1;
-				new_name = enode->str.substr(0, pos) + new_name + enode->str.substr(pos);
-				if (new_name[0] != '$' && new_name[0] != '\\')
-					new_name = prefix[0] + new_name;
-				name_map[enode->str] = new_name;
-
-				enode->str = new_name;
-				current_scope[new_name] = enode;
 			}
+			break;
+
+		default:
+			break;
 		}
 	}
 
@@ -3781,8 +3816,14 @@ void AstNode::expand_genblock(std::string index_var, std::string prefix, std::ma
 		// still needs to recursed-into
 		if (type == AST_PREFIX && i == 1 && child->type == AST_IDENTIFIER)
 			continue;
-		if (child->type != AST_FUNCTION && child->type != AST_TASK)
+		// functions/tasks may reference wires, constants, etc. in this scope
-			child->expand_genblock(index_var, prefix, name_map);
+		if (child->type == AST_FUNCTION || child->type == AST_TASK)
+			child->expand_genblock(index_var, prefix, name_map, false);
+		// continue prefixing if this child block is anonymous
+		else if (child->type == AST_GENBLOCK || child->type == AST_BLOCK)
+			child->expand_genblock(index_var, prefix, name_map, original_scope && child->str.empty());
+		else
+			child->expand_genblock(index_var, prefix, name_map, original_scope);
 	}
 
 

tests/simple/generate.v

@@ -159,3 +159,88 @@ generate
     end
 endgenerate
 endmodule
+
+// ------------------------------------------
+
+module gen_test7;
+	reg [2:0] out1;
+	reg [2:0] out2;
+	wire [2:0] out3;
+	generate
+		begin : cond
+			reg [2:0] sub_out1;
+			reg [2:0] sub_out2;
+			wire [2:0] sub_out3;
+			initial begin : init
+				reg signed [31:0] x;
+				x = 2 ** 2;
+				out1 = x;
+				sub_out1 = x;
+			end
+			always @* begin : proc
+				reg signed [31:0] x;
+				x = 2 ** 1;
+				out2 = x;
+				sub_out2 = x;
+			end
+			genvar x;
+			for (x = 0; x < 3; x = x + 1) begin
+				assign out3[x] = 1;
+				assign sub_out3[x] = 1;
+			end
+		end
+	endgenerate
+
+// `define VERIFY
+`ifdef VERIFY
+	assert property (out1 == 4);
+	assert property (out2 == 2);
+	assert property (out3 == 7);
+	assert property (cond.sub_out1 == 4);
+	assert property (cond.sub_out2 == 2);
+	assert property (cond.sub_out3 == 7);
+`endif
+endmodule
+
+// ------------------------------------------
+
+module gen_test8;
+
+// `define VERIFY
+`ifdef VERIFY
+	`define ASSERT(expr) assert property (expr);
+`else
+	`define ASSERT(expr)
+`endif
+
+	wire [1:0] x = 2'b11;
+	generate
+		begin : A
+			wire [1:0] x;
+			begin : B
+				wire [1:0] x = 2'b00;
+				`ASSERT(x == 0)
+				`ASSERT(A.x == 2)
+				`ASSERT(A.C.x == 1)
+				`ASSERT(A.B.x == 0)
+			end
+			begin : C
+				wire [1:0] x = 2'b01;
+				`ASSERT(x == 1)
+				`ASSERT(A.x == 2)
+				`ASSERT(A.C.x == 1)
+				`ASSERT(A.B.x == 0)
+			end
+			assign x = B.x ^ 2'b11 ^ C.x;
+			`ASSERT(x == 2)
+			`ASSERT(A.x == 2)
+			`ASSERT(A.C.x == 1)
+			`ASSERT(A.B.x == 0)
+		end
+	endgenerate
+
+	`ASSERT(x == 3)
+	`ASSERT(A.x == 2)
+	`ASSERT(A.C.x == 1)
+	`ASSERT(A.B.x == 0)
+endmodule