[ImportVerilog] add stream concat operation #7784
Conversation
|
Hey @chenbo-again, thanks a lot for working on this! 🥳 I'm wondering if we need a dedicated type for the streams. Do you know whether SystemVerilog has a specific type that it returns for streaming operations such as |
|
I thought that before but have no answer, because streaming concat operation can be operated on bit-stream type(described in IEEE 1800-2017), dynamically sized array can be included.
And @hailongSun2000 proposed that we can distinguish sized one and unsized one, and maybe we don't have any choice to deal all the things in ImportVerilog stage. |
hailongSun2000
requested review from
fabianschuiki,
maerhart and
hailongSun2000
|
Hey, @chenbo-again! Thanks for your work on the streaming operator. Presently, we have a dedicated op( |
Hi, @hailongSun2000 . |
|
Here are some cases: Slang will throw an error if slice size is not a constant value. So maybe we only need to use |
|
Recently I pushed a commit try to implement streaming concat operation with concat & extract operation. But currently it's limited that the operand must be sized IntType. It's not easy to deal the cases which the operands is unsized, and I believe in the future we will find an answer and implement it. for now, streaming concat assignment expression maybe it's usable for many case for now. |
|
This looks nice! I like your idea of splitting this into a dynamically-sized and statically-sized problem. Only supporting streaming operators with statically-known sizes sounds like a great starting point. Especially since Moore doesn't support dynamic arrays yet. It might be easier to add streaming operator support for dynamic arrays after we add support for dynamic arrays themselves, because we've established all the available ops at that point. Very cool work! |
|
Your example is LGTM! However, could we reduce the finally, we can create |
chenbo-again
force-pushed
the
stream_concat
branch
from
6f6d3ae
to
ce040b7
Compare
|
thank you for your review! :) |
| for (auto stream : expr.streams()) { | ||
| auto value = context.convertLvalueExpression(*stream.operand); | ||
| if (!value) | ||
| return {}; | ||
| operands.push_back(value); | ||
| } |
There was a problem hiding this comment.
I noticed that each stream can also have a withExpr and constantWithWidth. Could you add error messages if either of these are set, and tell the user that we don't support those cases? It's important that we only succeed if we were able to process everything the user has type, and not silently ignore parts of the input.
| for (auto stream : expr.streams()) { | ||
| auto value = context.convertRvalueExpression(*stream.operand); | ||
| if (!value) |
There was a problem hiding this comment.
I noticed that each stream can also have a withExpr and constantWithWidth. Could you add error messages if either of these are set, and tell the user that we don't support those cases? It's important that we only succeed if we were able to process everything the user has type, and not silently ignore parts of the input.
| // CHECK: [[BYTE4:%.+]] = moore.extract [[CONCAT]] from 32 : l48 -> l8 | ||
| // CHECK: [[BYTE5:%.+]] = moore.extract [[CONCAT]] from 40 : l48 -> l8 | ||
| // CHECK: [[RESULT:%.+]] = moore.concat [[BYTE5]], [[BYTE4]], [[BYTE3]], [[BYTE2]], [[BYTE1]], [[BYTE0]] : (!moore.l8, !moore.l8, !moore.l8, !moore.l8, !moore.l8, !moore.l8) -> l48 | ||
| vec_5 = {<<byte{vec_3, vec_4}}; |
There was a problem hiding this comment.
Nice, thanks for checking different-sized variables here!
| // CHECK: [[BYTE4_REF:%.+]] = moore.extract_ref [[CONCAT_REF]] from 32 : <l48> -> <l8> | ||
| // CHECK: [[BYTE5_REF:%.+]] = moore.extract_ref [[CONCAT_REF]] from 40 : <l48> -> <l8> | ||
| // CHECK: [[RESULT_REF:%.+]] = moore.concat_ref [[BYTE5_REF]], [[BYTE4_REF]], [[BYTE3_REF]], [[BYTE2_REF]], [[BYTE1_REF]], [[BYTE0_REF]] : (!moore.ref<l8>, !moore.ref<l8>, !moore.ref<l8>, !moore.ref<l8>, !moore.ref<l8>, !moore.ref<l8>) -> <l48> | ||
| {<<byte{vec_3, vec_4}} = vec_5; |
There was a problem hiding this comment.
Can you add a few more test cases that check other ways of how streams can be described? For example, I think your implementation already supports nested streams. And there's also the possibility of having a stream go in the other direction >>. I also noticed that the spec allows for streams to specify a slice size instead of a type, so something like {<<16{...}}.
The SystemVerilog specification contains a few examples for streams which might be great to include here, or use for inspiration for the different kinds of streams that we should test. For example, in IEEE 1800-2017 section 11.4.14.2 "Re-ordering of the generic stream" I see the following test:
int j = { "A", "B", "C", "D" };
{ >> {j}} // generates stream "A" "B" "C" "D"
{ << byte {j}} // generates stream "D" "C" "B" "A" (little endian)
{ << 16 {j}} // generates stream "C" "D" "A" "B"
{ << { 8'b0011_0101 }} // generates stream 'b1010_1100 (bit reverse)
{ << 4 { 6'b11_0101 }} // generates stream 'b0101_11
{ >> 4 { 6'b11_0101 }} // generates stream 'b1101_01 (same)
{ << 2 { { << { 4'b1101 }} }} // generates stream 'b1110Pretty sure you could remove the string constants "A", "B", "C", "D" and then just check whether you get the correct slices of the variable j with your implementation. Or replace the string with a easily-recognizable constant, like 32'h87654321.
In IEEE 1800-2017 section 11.4.14.3 "Streaming concatenation as an assignment target (unpack)" I see the following test:
int a, b, c;
logic [10:0] up [3:0];
logic [11:1] p1, p2, p3, p4;
bit [96:1] y = {>>{ a, b, c }}; // OK: pack a, b, c
int j = {>>{ a, b, c }}; // error: j is 32 bits < 96 bits
bit [99:0] d = {>>{ a, b, c }}; // OK: d is padded with 4 bits
{>>{ a, b, c }} = 23'b1; // error: too few bits in stream
{>>{ a, b, c }} = 96'b1; // OK: unpack a = 0, b = 0, c = 1
{>>{ a, b, c }} = 100'b11111; // OK: unpack a = 0, b = 0, c = 1; 96 MSBs unpacked, 4 LSBs truncated
{ >> {p1, p2, p3, p4}} = up; // OK: unpack p1 = up[3], p2 = up[2], p3 = up[1], p4 = up[0]Variations of these tests might be good to have. You don't need to check for error reporting if the error is reported by Slang and not your code; Slang already contains tests for this and we don't need to re-check Slang. But for any error message you produce in your code, and every branch/early exit in your code, you'd want to have a test that checks whether things work as expected there 😃
@fabianschuiki, @hailongSun2000
Add moore.stream_concat operation for streaming like concat, it's currently a draft version and call for comments. for lvalue, {1 << {i32, i32, i32}} will be treated like
and it will be process in the core dialect.
Because the stream concat operation is so flexible, various datatype is acceptable as input, so I just Postponed the actual analysis. And stream concat oepration just be treated like a conversion from input to a packed type.