Add support for packed multidimensional arrays #4100

daglem · 2024-01-01T21:35:04Z

Generalization of dimensions metadata (also simplifies $size et al.)
Parsing and elaboration of multidimensional packed arrays
Implementation of $increment, $dimensions and $unpacked_dimensions
Resolve multiple dimensions defined in stages with typedef

povik · 2024-01-10T15:57:18Z

tests/simple/memory.v

@@ -44,6 +44,7 @@ input [2:0] bit;
 output reg y1, y2;
 output y3, y4;

+(* nomem2reg *)
 reg [7:0] mem1 [3:0];


Is adding the attribute related to the other changes here?

Not really, but while testing the branch I found that mem1 was automatically converted to registers, while the point here seems to be to test both memory (mem1) and memory converted to registers (mem2).

Something has probably changed along the way here wrt. the heuristics for automatic conversion to registers, possibly with the change from memory_bram to memory_libmap?

It's a nit, but it's best to separate changes like this into commits of their own.

Something has probably changed along the way here wrt. the heuristics for automatic conversion to registers, possibly with the change from memory_bram to memory_libmap?

FWIW the mem2reg/nomem2reg attribute is understood by the frontend, once we are operating on the netlist they are not picked up by anything. Right now I don't understand the nature of the test here and the frontend heuristics for mem2reg conversion to say if we want the attribute here.

It's a nit, but it's best to separate changes like this into commits of their own.

Sure, I'll split this change out.

Something has probably changed along the way here wrt. the heuristics for automatic conversion to registers, possibly with the change from memory_bram to memory_libmap?

FWIW the mem2reg/nomem2reg attribute is understood by the frontend, once we are operating on the netlist they are not picked up by anything. Right now I don't understand the nature of the test here and the frontend heuristics for mem2reg conversion to say if we want the attribute here.

Right. I checked why mem1 is converted to registers by default - it's because all the (non-initial) assignments to mem1 are made using constant indices (MEM2REG_FL_CONST_LHS is set in simplify.cc).

I'm pretty sure the (* nomem2reg *) attribute on mem1 makes sense there, since otherwise the test of mem1 is superfluous (covered by the test of mem2).

I checked the timeline of this. Since (* mem2reg *) was added to the test before MEM2REG_FL_CONST_LHS was introduced in simplify.cc, it seems clear that adding (* nomem2reg *) to mem1 is necessary to make the test work according to the original intention:

$ git show -s --format="%h %as %s" 19dba2561e tests/simple/memory.v 19dba2561 2013-11-20 Implemented part/bit select on memory read $ git show -s --format="%h %as %s" 7cfae2c52 frontends/ast/simplify.cc 7cfae2c52 2019-03-01 Use mem2reg on memories that only have constant-index write ports

tests/svtypes/struct_sizebits.sv

povik · 2024-01-22T10:14:18Z

I only gave this a superficial drive-by look, I don't understand the part of the frontend this is touching. We will probably need to wait for @zachjs to give this a proper review, if he has the chance.

zachjs

This is a great change that both extends the frontend and improves its overall code quality. Thank you!

My most substantive request is that we add a few more tests, especially for the "nested" typedefs logic. I think we find a way to break anything in the frontend that isn't fully tested. If you can't do this (you've already done a lot!), I'd be happy to. In any event, it shouldn't prevent this from getting merged. I think we can merge it right after the next release.

zachjs · 2024-01-23T02:49:47Z

frontends/verilog/verilog_parser.y

+			}
+		}
+		if (!valid)
+			frontend_verilog_yyerror("wire/reg/logic packed dimension must be of the form: [<expr>:<expr>], [<expr>+:<expr>], or [<expr>-:<expr>]");


Let's fix this error message while we're here. Yosys indeed allows +: and -: in packed dimension, but this isn't allowed in the standard (range vs. part_select_range), so I'd prefer not to suggest this to users. We could also mention that we allow the SV shorthand [<expr>].

OK, fixed. Note that [<expr>] would not be correct here, since checkRange is only used to check packed ranges, and [<expr>] is only allowed for unpacked ranges in the LRM.

I first renamed checkRange to checkPackedRange in order to make this clearer, but decided against it in order to keep the diff small. This could always be changed later.

zachjs · 2024-01-23T02:50:16Z

frontends/ast/simplify.cc

+	for (int i = unpacked_dimensions; i < GetSize(dimensions); i++) {
+		mem_width *= dimensions[i].range_width;
+	}
+	*/


It's probably going to have to be used at some point (arrays of structs), but I've removed it for now.

zachjs · 2024-01-23T02:55:34Z

frontends/ast/simplify.cc

@@ -4936,6 +4849,9 @@ void AstNode::mem2reg_as_needed_pass1(dict<AstNode*, pool<std::string>> &mem2reg
 	{
 		AstNode *mem = id2ast;

+		if (integer < (unsigned)mem->unpacked_dimensions)
+			input_error("Insufficient number of array indices for %s.\n", log_id(str));


Can you add a test for this error?

This is really a shortcoming in Yosys, which currently treats anything with unpacked ranges as "memory", where the memory word consists of the packed part, and only indexing which ultimately selects the packed part is allowed (contrary to the LRM).

I'd rather not add tests for this shortcoming; I think it would be better to consider what could possibly be done about this in the future. Perhaps automatic mem2reg conversion of "memory" which has unpacked parts selected?

zachjs · 2024-01-23T02:57:46Z

frontends/ast/simplify.cc

 				AstNode *id_ast = NULL;

 				// Is this needed?
-				//while (buf->simplify(true, false, stage, width_hint, sign_hint, false)) { }
+				//while (buf->simplify(true, stage, width_hint, sign_hint)) { }


Do you think this is needed? If not, let's just delete this comment to avoid future confusion.

No idea whether it can be needed in some situation, really - I only adapted the commented out code to earlier changes in Yosys.

In any case I've removed the comment now.

zachjs · 2024-01-23T03:06:17Z

frontends/ast/simplify.cc

-							}
+						int dims = GetSize(id_ast->dimensions);
+						if (dim < 1 || dim > dims)
+							input_error("Dimension %d out of range in `%s', as it only has %d dimensions!\n", dim, id_ast->str.c_str(), dims);


I think this behavior is incorrect. It should produce 'x. I know this is inherited from the existing logic, so you can defer fixing it and just add a TODO.

I actually noticed this (old) incorrect behavior as well, however I had to stop somewhere for now.
TODO added.

zachjs · 2024-01-23T03:07:57Z

frontends/ast/simplify.cc

@@ -3158,6 +3125,8 @@ skip_dynamic_range_lvalue_expansion:;
 	// found right-hand side identifier for memory -> replace with memory read port
 	if (stage > 1 && type == AST_IDENTIFIER && id2ast != NULL && id2ast->type == AST_MEMORY && !in_lvalue &&
 			children.size() == 1 && children[0]->type == AST_RANGE && children[0]->children.size() == 1) {
+		if (integer < (unsigned)id2ast->unpacked_dimensions)
+			input_error("Insufficient number of array indices for %s.\n", log_id(str));


I think this error has no test coverage.

It doesn't, however it is old behavior, and incorrect at that - see comment above.

zachjs · 2024-01-29T04:18:06Z

frontends/ast/simplify.cc

+			type = AST_WIRE;
+			AstNode *expr = children[0];
+			children.erase(children.begin());
+			while (is_custom_type && simplify(const_fold, stage, width_hint, sign_hint)) {};


If this type substitution pattern proliferates, let's factor it out. You can leave it for now.

OK, I'll keep it in mind.

daglem · 2024-01-29T18:56:12Z

This is a great change that both extends the frontend and improves its overall code quality. Thank you!

My most substantive request is that we add a few more tests, especially for the "nested" typedefs logic. I think we find a way to break anything in the frontend that isn't fully tested. If you can't do this (you've already done a lot!), I'd be happy to. In any event, it shouldn't prevent this from getting merged. I think we can merge it right after the next release.

It would be great if you could add the tests you find relevant, thank you!

I am aware of one construct where nested typedefs will yield incorrect results; arrays of structs/unions. However this is material for another PR.

* Generalization of dimensions metadata (also simplifies $size et al.) * Parsing and elaboration of multidimensional packed ranges

The purpose of memtest02 in tests/simple/memory.v is to test bit select on both memory (mem1) and memory converted to registers (mem2). After 7cfae2c, mem1 was automatically converted to registers, and the test no longer worked as intended. This is fixed by adding (* nomem2reg *) to mem1.

daglem requested a review from zachjs as a code owner January 1, 2024 21:35

povik reviewed Jan 10, 2024

View reviewed changes

tests/svtypes/struct_sizebits.sv Show resolved Hide resolved

daglem force-pushed the multidimensional-arrays branch from 1edc3a2 to 741877c Compare January 10, 2024 21:14

daglem force-pushed the multidimensional-arrays branch from 741877c to 7d1a95b Compare January 25, 2024 07:23

zachjs reviewed Jan 29, 2024

View reviewed changes

daglem force-pushed the multidimensional-arrays branch 3 times, most recently from 9cc7498 to de908ae Compare January 29, 2024 19:45

daglem added 9 commits January 29, 2024 20:56

Add support for packed multidimensional arrays

2624a3e

* Generalization of dimensions metadata (also simplifies $size et al.) * Parsing and elaboration of multidimensional packed ranges

Decoding of a few more AST nodes in dumpVlog

f30208e

Add support for $increment

5662b8f

Add support for $dimensions and $unpacked_dimensions

c4ecd4e

Resolve multiple dimensions defined in stages with typedef

e9c58d5

Added test for multidimensional packed arrays

8bb6a44

Add multidimensional arrays to SystemVerilog features in README

3700d47

Resolve struct member multiple dimensions defined in stages with typedef

f57abd4

daglem force-pushed the multidimensional-arrays branch from de908ae to f57abd4 Compare January 29, 2024 19:56

This was referenced Feb 2, 2024

Add support for unpacked structs #4180

Draft

SV arrays of structs #2677

Open

zachjs approved these changes Feb 11, 2024

View reviewed changes

zachjs merged commit f09ea16 into YosysHQ:master Feb 11, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for packed multidimensional arrays #4100

Add support for packed multidimensional arrays #4100

daglem commented Jan 1, 2024 •

edited

Loading

povik Jan 10, 2024

daglem Jan 10, 2024

povik Jan 22, 2024

daglem Jan 24, 2024

povik commented Jan 22, 2024

zachjs left a comment

zachjs Jan 23, 2024

daglem Jan 29, 2024

zachjs Jan 23, 2024

daglem Jan 29, 2024

zachjs Jan 23, 2024

daglem Jan 29, 2024

zachjs Jan 23, 2024

daglem Jan 29, 2024

zachjs Jan 23, 2024

daglem Jan 29, 2024

zachjs Jan 23, 2024

daglem Jan 29, 2024

zachjs Jan 29, 2024

daglem Jan 29, 2024

daglem commented Jan 29, 2024

Add support for packed multidimensional arrays #4100

Add support for packed multidimensional arrays #4100

Conversation

daglem commented Jan 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

povik commented Jan 22, 2024

zachjs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daglem commented Jan 29, 2024

daglem commented Jan 1, 2024 •

edited

Loading