New Rule: Forbid unpacked array declarations

[ronitnallagatla]

This PR implements a lint rule check that forbids unpacked array declarations.

Relevant Issues:

• closes dalance/svlint#279

[DaveMcEwan]

Nice one @ronitnallagatla.

1. Please add your name to the list of contributors.
2. Please add newlines (hard-wrap at 80 characters) in the explanation to match the others and make review a bit easier.
3. The rule is very broad, and I'm unsure if that's what you're aiming for. Would you mind adding some more testcases to show that this applies to any use of unpacked dimensions, not just logic declarations. It also applies to, for example, module ports, localparam/parameters, arrays of interfaces/modules, temporary variables in constant functions, and assignments to unpacked variables from other code. It also forbids Verilog arrays and memories (IEEE 1364-2001, 3.10 Arrays), which sounds maybe unintended(?). If you give me a couple of weeks (to find a free evening), I can make a PR to your branch with some more testcases.
4. Would you mind sharing what kind of issues you're encountering in synthesis? It would be good to give some specific context so that other users can see if it applies to their situation. Wrong logic? Unexpected cells? Tool crashes or confusing messages? Using proprietary or FOSS synthesis tools?

[ronitnallagatla]

Nice one @ronitnallagatla.

1. Please add your name to the [list of contributors](https://github.com/dalance/svlint/blob/master/CONTRIBUTING.md).

2. Please add newlines (hard-wrap at 80 characters) in the [explanation](https://github.com/dalance/svlint/pull/280/files#diff-8f3a28ac3ab4e3465b88f3a2055a8671cbf9d6593dc284b1e7ec134cc3b7792c) to match the others and make review a bit easier.

3. The rule is very broad, and I'm unsure if that's what you're aiming for. Would you mind adding some more testcases to show that this applies to _any use_ of unpacked dimensions, not just logic declarations. It also applies to, for example, module ports, localparam/parameters, arrays of interfaces/modules, temporary variables in constant functions, and assignments to unpacked variables from other code. It also forbids Verilog arrays and memories (IEEE 1364-2001, 3.10 Arrays), which sounds maybe unintended(?). If you give me a couple of weeks (to find a free evening), I can make a PR to your branch with some more testcases.

4. Would you mind sharing what kind of issues you're encountering in synthesis? It would be good to give some specific context so that other users can see if it applies to their situation. Wrong logic? Unexpected cells? Tool crashes or confusing messages? Using proprietary or FOSS synthesis tools?

Thanks for the feedback :)

The motivation behind the rule is to prevent the use of unpacked array signals. So I think for the most part, the rule is relevant for module ports and parameters. I'm not sure about arrays of modules and variables in functions, since the issues I was facing were related to how signals were synthesized. I think for some non-synthesizable types like classes and events, only unpacked arrays are supported -- which might be worth implementing as another rule.

Based on IEEE 1800-2017, 7.4.3 Operations on Arrays, packed arrays can be used with more operators than unpacked arrays. Unpacked arrays don't support all arithmetic and bitwise operators and cannot be assigned as a whole.

I was facing an issue where my synthesis tool would optimize out some indices of the unpacked memory that it thinks are not being used, which caused mismatches between simulation and synthesis (maybe something weird happens when it tries to flatten multidimensional hierarchies during synth). Another issues I came across: https://github.com/verilator/verilator/issues/2907#issuecomment-829971656

[ronitnallagatla]

• The rule is very broad, and I'm unsure if that's what you're aiming for. Would you mind adding some more testcases to show that this applies to any use of unpacked dimensions, not just logic declarations. It also applies to, for example, module ports, localparam/parameters, arrays of interfaces/modules, temporary variables in constant functions, and assignments to unpacked variables from other code.

Do you think it would be better to have this rule only apply to signal/parameter declarations? Or would it be more appropriate to have separate rules for each of these?

[DaveMcEwan]

First, sorry for the delay!

A packed array is a collection of things that can be used as 1 thing - a signed or unsigned integer. That's great for use-cases like packets where you construct 1 thing from many components, then operate on the packet as 1 thing, e.g. calculating a checksum or shifting its bits to a transmitter. An unpacked array is a way for an author to say "it doesn't make sense to treat this collection of things as 1 thing". The fact that you can't perform an arithmetic operation like shift/add/multiplying is therefore a useful limitation because the author is saying that wouldn't make sense.

Do you think it would be better to have this rule only apply to signal/parameter declarations? Or would it be more appropriate to have separate rules for each of these?

I think the best approach is: Just restrictive enough that it helps mitigate your issue, but no more restrictive than that.

My instinct would be to have a flag for each thing that you want to restrict, similar to this which is set on entering the node and cleared on leaving the node. Basically, a flag for each of:

• local_parameter_declaration
• parameter_declaration
• specparam_declaration
• inout_declaration
• input_declaration
• output_declaration
• interface_port_declaration
• ref_declaration
• data_declaration
• net_declaration

Instead of having 10 separate rules (and 10 explanations), you could add 10 Boolean configuration options, so the TOML would look something like:

syntaxrules.unpacked_array = true
option.unpacked_array.localparam = false
option.unpacked_array.parameter = true
option.unpacked_array.input = false
option.unpacked_array.output = true

Then, check for RefNode::UnpackedDimension only if one of those flags are set plus the corresponding option is set. That would give you the specificity of targeting only certain kinds of declarations, and avoids having to rework things if the current approach turns out to be too restrictive.

[ronitnallagatla]

No problem! Thanks for getting back :)

My instinct would be to have a flag for each thing that you want to restrict, similar to this which is set on entering the node and cleared on leaving the node.

I think using the configuration option to enable/disable which declaration the rule applies to makes for an elegant solution.

If the syntax rule is enabled, but the option is not configured, could the rule by default only target data declarations? Or should the documentation specify that the option must be configured as well. Basically, how do we handle the case when the syntax rule is enabled, but none of the options are set.

Thanks again for your time and feedback!

[ronitnallagatla]

I've updated the config to hold the target declaration options, as well as the implementation to check if the corresponding flag and option is set.

The rule on default only targets data declarations, and can be enabled/disabled along with all the other declarations through the config file. I've also updated the testcases to only contain a data declaration testcase since it is enabled by default and will work with cargo test. Let me know what you think!

[dalance]

I think this PR can be merged. Could you resolve the conflict?

[ronitnallagatla]

Thanks @dalance. I will open a new PR to add back the contributors after this PR has been merged