Skip to content

Conversation

@Scheremo
Copy link
Contributor

@Scheremo Scheremo commented Sep 13, 2025

This PR adds support for materializing slang::ast::FixedSizeUnpackedArrayType as moore::UnpackedArrayType.
Previously, only constant immediateSVInts could be materialized, and working with RTL that used constant-valued, fixed-size unpacked arrays led to errors if they were used after declaration.

This conversion is necessary for supporting unpacked system verilog parameter arrays, e.g.:
parameter int unsigned SomeParam [8] = {default: 0}
This pattern is valid system verilog and widely used in the snitch cluster repository, for example in the snitch_cluster and snitch_cluster_wrapper modules.

This implementation adds the function Context::materializeFixedSizeUnpackedArrayType as a target for Context::materializeConstant if the provided type can be safely cast to slang::ast::FixedSizeUnpackedArrayType (at runtime); otherwise the old logic is applied.
This PR also adds a test where the materialization conversion is applied on a module parameter and used in a function declared within the module body; it checks that the array is properly set up from constants and array access use a moore::dyn_extract.

I'm not fully sure the logic to extract the maximum bitwidth of all elements or inference of four-value logic is needed. Please let me know if you see a better way!
EDIT: Removed what appears to me to be superfluous checking for the bitwidth of the underlying type and the presence of X/Z values.

@Scheremo Scheremo changed the title [ImportVerilog] Add support for materializing FixedSizeUnpackedArrayType from ConstantValue [ImportVerilog] Add support for materializing FixedSizeUnpackedArrayType as UnpackedArrayType. Sep 13, 2025
@Scheremo Scheremo marked this pull request as draft September 13, 2025 15:56
@Scheremo Scheremo marked this pull request as ready for review September 13, 2025 16:24
Copy link
Contributor

@fabianschuiki fabianschuiki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really cool! 💯 Just a minor nit about testing for element order.

Comment on lines 3167 to 3183
// CHECK-LABEL: moore.module @UnpackedParameterArray(
module UnpackedParameterArray #(
parameter int unsigned ParameterArray [2] = '{default: 0}
) (
input logic clk_i,
input logic rst_ni
);

function automatic int unsigned returnParameterArrayElement (int idx);
// CHECK: [[CONST0:%.+]] = moore.constant 0 : i32
// CHECK-NEXT: [[CONST1:%.+]] = moore.constant 0 : i32
// CHECK-NEXT: [[ARR:%.+]] = moore.array_create [[CONST0]], [[CONST1]] : !moore.i32, !moore.i32 -> uarray<2 x i32>
// CHECK: [[RETURN:%.+]] = moore.dyn_extract [[ARR]] from {{%.+}} : uarray<2 x i32>, i32 -> i32
return ParameterArray[idx];
endfunction

endmodule // UnpackedParameterArray
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool! 🥳

Could you remove the ports, and maybe pull the parameter into the module as a localparam, just to make the test very small? You might even be able to move the localparam into the function such that you don't need a module at all 🙂.

One thing that might be good to check is the order of elements. Can you populate the array with two distinct values, something like '{42, 9001}, and then check in the IR that the elements have the correct order? CIRCT does some weird stuff around some of the array_create ops where the operand order is reversed to mimic Verilog syntax, which has been a major source of headache. Good to nail these things down in tests 🙂.

@Scheremo
Copy link
Contributor Author

Those are all very useful comments! I managed to refactor this into a single function declaration with a localparam containing discernable values (42 & 9001 - any specific reason?).
If you're happy with it we can merge 🚀

@fabianschuiki
Copy link
Contributor

I managed to refactor this into a single function declaration with a localparam containing discernable values (42 & 9001 - any specific reason?).

Very neat! I usually use some meme numbers to ensure things don't all line up with power of two boundaries, or very regular bit patterns. And it makes the tests fun to read 😝

If you're happy with it we can merge

Absolutely! Let me wait for CI and then hit merge. Feel free to get commit access to the LLVM org if you'd like to land PRs yourself. If you Google it you'll probably find the link/community guidelines for that 🙂

@fabianschuiki fabianschuiki merged commit 38663fa into llvm:main Sep 13, 2025
7 checks passed
@Scheremo Scheremo deleted the pr-import-stmt branch October 21, 2025 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants