Synthesis Errors

[mp-17]

Hello folks,

The following lines of code trigger elaboration errors with DC Compiler 2022.12.

https://github.com/pulp-platform/axi_llc/blob/master/src/axi_llc_read_unit.sv#L100
https://github.com/pulp-platform/axi_llc/blob/master/src/axi_llc_write_unit.sv#L109
https://github.com/pulp-platform/axi_llc/blob/master/src/axi_llc_hit_miss.sv#L375

Can you please fix this?

Thank you for the help,
Matteo

[FrancescoConti]

Interesting, I never saw this [base +: offset] bit-indexing syntax before. From a quick look at the standard, it seems base is not necessarily constant, which I guess triggers DC in the wrong way!
IMHO we should avoid such exoteric little quirks of SystemVerilog (which, besides being sometimes non-synthesizable, are also absolutely non-obvious to read - particularly if there is no accompanying comment)... unless absolutely necessary.
Refs:

• https://stackoverflow.com/questions/18067571/indexing-vectors-and-arrays-with
• SystemVerilog IEEE 1800-2017 https://ieeexplore.ieee.org/stamp/stamp.jsp?tp=&arnumber=8299595 paragraph 11.5.1

[mp-17]

Hey @FrancescoConti, thank you for the quick answer!
Apparently, the struct assignment is not welcome :-)

@suehtamacv and I created a fix in this PR: #9

[paulsc96]

Interesting, I never saw this [base +: offset] bit-indexing syntax before. From a quick look at the standard, it seems base is not necessarily constant, which I guess triggers DC in the wrong way!

This is a fairly common construct, especially in our codebases. It is the width expression which must be constant according to 1800-2017, not the base; in all examples referenced, both the base and width expressions are elaboration-time-constant parameters, so there is no standard violation here.

IMHO we should avoid such exoteric little quirks of SystemVerilog (which, besides being sometimes non-synthesizable, are also absolutely non-obvious to read - particularly if there is no accompanying comment)... unless absolutely necessary.

Maybe this is a matter of experience, but to me, this construct is neither uncommon nor esoteric, and I have never seen it fail in any tool so far, including DC. Sometimes, tools have issues; if we rewrite our RTL every time one tool experiences a regression, that is probably all we would be doing. Just my two cents.

Can you please fix this?

There is nothing to fix. If you want to merge a hacky workaround, you will have to take personal responsibility and do it without my approval.

[suehtamacv]

I would not call the proposed solution "hacky." Workaround perhaps, but definitely not hacky. The tool is not being particularly verbose regarding why it cannot elaborate this struct assignment construct, (it fails somewhat silently) and we found out that exploding the struct assignment fixes the issue.

The single point against the proposed solution, in my opinion, is that we lose the default: assignment. Instead, @mp-17 had to manually assign zero to the unused struct elements. Is this why you do not agree with this solution?

How do other EDA tools handle this repo's RTL? Is SpyGlass happy? Verilator? What about Formality, does it report that the design is being elaborated correctly?

[FrancescoConti]

This is a fairly common construct, especially in our codebases. It is the width expression which must be constant according to 1800-2017, not the base; in all examples referenced, both the base and width expressions are elaboration-time-constant parameters, so there is no standard violation here.

Good, always something to learn :) I did not imply there was a standard violation in the code, though, if you read what I commented above. I simply stated that being unable to prove that a given dimension is constant might be troublesome for a tool, which is clearly a limitation of the tool itself. It seems that was not the case anyways.

Sometimes, tools have issues; if we rewrite our RTL every time one tool experiences a regression, that is probably all we would be doing. Just my two cents.

I strongly disagree with this point, in general, unless we have a viable way to push for fixes upstream, which as far as I know is not that easy for closed-source tools (it is indeed possible for open-source ones).
And it is 1) not hard, 2) not hampering quality-of-results to write code that uses only 95% of the expressiveness of the "theoretically synthesizable subset" of the SV language instead of 99% of it, insisting that it "should work as it is standard" instead of just rewriting code that works.

Then if you argue that the standard is broken, I fully agree - but that's what we have, and I do not find that (as of today) other solutions are really that competitive.

[paulsc96]

How do other EDA tools handle this repo's RTL? Is SpyGlass happy? Verilator? What about Formality, does it report that the design is being elaborated correctly?

What I can tell you is that to the best of my knowledge, there have never been issues with this RTL in simulation or synthesis except for one (tool-side) simulation bug unrelated to this discussion. If you want to imply that there are deeper issues with the IP, I believe the onus is on you to prove this.

You are all pulp-platform members. If you want to degrade the quality our codebases, I cannot stop you. I will simply not approve a change I disagree with and validate such behavior.

[mp-17]

Let us see what @thommythomaso thinks as well.

if we rewrite our RTL every time one tool experiences a regression, that is probably all we would be doing

Merging these changes would ease our development process, and if the changes do not worsen the RTL, the only downside of rewriting the code is the time spent on it, which is almost completely on who re-wrote the code and issued the PR :-)