[slang] Add virtual interface declaration checks

[likeamahoney]

In this PR i add some virtual interface checks such as:

• hierarchical references usage
• interface ports usage

That checks related to part of #522

Although an interface may contain hierarchical references to objects outside its body or ports that reference
other interfaces, it shall be illegal to use an interface containing those references in the declaration of a
virtual interface.

[codecov]

Codecov Report

Attention: Patch coverage is 96.77419% with 2 lines in your changes missing coverage. Please review.

Project coverage is 94.87%. Comparing base (8174a94) to head (12d94ee).
Report is 10 commits behind head on master.

Files	Patch %	Lines
source/ast/types/AllTypes.cpp	94.28%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1088      +/-   ##
==========================================
+ Coverage   94.76%   94.87%   +0.11%     
==========================================
  Files         191      191              
  Lines       47716    47802      +86     
==========================================
+ Hits        45217    45352     +135     
+ Misses       2499     2450      -49     

Files	Coverage Δ
include/slang/ast/symbols/InstanceSymbols.h	100.00% <100.00%> (ø)
source/ast/ElabVisitors.h	96.89% <100.00%> (+0.08%)	⬆️
source/ast/symbols/InstanceSymbols.cpp	94.54% <100.00%> (+0.03%)	⬆️
source/ast/types/AllTypes.cpp	93.52% <94.28%> (+0.01%)	⬆️

... and 18 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8174a94...12d94ee. Read the comment docs.

[MikePopoloski]

We really don't want to rerun this checker on every virtual interface type declaration -- ideally we only run it once per unique interface instantiation. My thinking was to track hierarchical lookups more closely, registering them on instances as they pass through them. Primarily this would be to support caching of instance bodies to avoid unnecessary elaboration work, but as a side effect would make this kind of checking much easier.

[likeamahoney]

instance hierarchical identifiers caching was added

[MikePopoloski]

This seems wrong; instantiating an interface inside another interface doesn't make it a port, so we shouldn't error on it.

[likeamahoney]

still in progress

[likeamahoney]

in the first version of the solution, I misunderstood LRM in this term, sorry

[likeamahoney]

add that check in the last commit

[MikePopoloski]

In general we don't need to keep adding new notes that are very slight variations of existing ones; the more general "declared here" note is fine for these purposes.

[likeamahoney]

removed

[MikePopoloski]

This is definitely not correct; just because the definition is the same doesn't mean the hierarchical identifiers will be the same, since behavior can vary on the parameters of the instance controlling generate blocks in the child.

This is a generally hard problem; my thinking in the past was that I would keep track of found hierarchical identifiers during Lookup and caching based on a more complicated cache key that takes into account parameters, interface ports, any active configurations, etc. Tracking for that is here: #949

[likeamahoney]

Hi, I have an idea for a hash - Can it be a std::hash of std::string consisting of an instance kind (for example interface or module), instance type (for example - module type identifier) and a set of parameter values ​​with their identifiers?

For example for such module:

module mem_model #(
 parameter ADDR_WIDTH=8;
 parameter DATA_WIDTH=32;)
 (clk, addr, data);

 input  clk;
 input  [ADDR_WIDTH-1:0] addr;
 output [DATA_WIDTH-1:0] data;
 .....
 .....
endmodule

and it's instances:

//Creates mem_1 instance with default addr and data widths.
mem_model mem_1 (.clk(clk),
                 .addr(addr_1),
                 .data(data_1));

//Creates mem_2 instance with addr width = 4 and data width = 8.
mem_model #(4,8) mem_2 (.clk(clk),
                        .addr(addr_2),
                        .data(data_2));

//Creates mem_3 instance with addr width = 32 and data width = 64.
mem_model #(32,64) mem_3 (.clk(clk),
                          .addr(addr_3),
                          .data(data_3));

//Creates mem_4 instance with default addr width and data width = 64.
mem_model #(DATA_WIDTH=64) mem_4 (.clk(clk),
                                 .addr(addr_3),
                                 .data(data_3));

hash strings can have a form:

"module mem_model(ADDR_WIDTH=8, DATA_WIDTH=32)" // for mem_1

"module mem_model(ADDR_WIDTH=4, DATA_WIDTH=8)" // for mem_2

"module mem_model(ADDR_WIDTH=32, DATA_WIDTH=64)" // for mem_3

"module mem_model(ADDR_WIDTH=8, DATA_WIDTH=64)" // for mem_4

What do you think about this suggestion?

[MikePopoloski]

There's no reason for it to be a string; you can just hash the relevant fields. This diff from a long time ago removed an InstanceCache that I had in slang at one point: 0892da3#diff-18c21a73a1abf2b9e1aa72758848d9bceee11a575bfab7d762dfc51217ec26c3

That's a good start but it also needs to work correctly with hierarchical references into and out of cached instances, which is very tricky and something I need to think more carefully about (which is one of the reasons why I removed this back in the day).