Open likeamahoney opened 2 weeks ago
Attention: Patch coverage is 96.48712%
with 15 lines
in your changes missing coverage. Please review.
Project coverage is 94.87%. Comparing base (
73b79be
) to head (751e1c7
). Report is 3 commits behind head on master.
Thanks for the PR. I'll do a more thorough review of the resolution logic, but to comment on the high level approach:
Closes https://github.com/MikePopoloski/slang/issues/536
In this PR, I introduce the implementation of all possible illegal situations from the SystemVerilog LRM 16.13/16.16 sections and also some additional clock checks that support VCS.
Here is the bunch of new error types such as: 1) no clock could be resolved for concurrent property (emitted if there is a property which is not mapped to any clock):
2) explicit clocking event in clocking block (see section 16.16 clocking block rules - (b.1))
3) the property/sequence has clocking event not identical to that of the clocking block (see section 16.16 clocking block rules - (b.3))
4) single semantic leading clock expected for top-level sequence/property expression (see 16.13.7 and 16.16.1)
5) no default, inferred, or explicit leading clocking event and maximal property is not an instance (see section 16.16 clocking block rules - (f))
6) procedural assertion (assert, assume or cover property) is not allowed after delay or event control statement (vcs check which based on 16.14.6 (and also 16.14.6.2 - "In general, procedural concurrent assertions must be used carefully when mixed with time delays.") section where delay and event controls prevents clock inference for always block assertions - I decide to make it error like VCS do but not sure)
7) clocking events of binary sequence/property operands doesn't agree (see 16.13.1)
8) clocking events of delay concatenation operands doesn't agree (see 16.13.1)
9) differently clocked sequence shall not admit any empty match (see 16.13.1)
The main data structure is
ClockingEvent
, which is a wrapper forSignalEventControl
, it stores the event in two different forms - edge (EdgeKind
) + vector of signal references (EventList
) andstrEvent
(string representation for comparing two different clock events - in case the event is a complex expression (e.g. containsiff expr...
)). This data structure is designed to accumulate clock events during AST traversal and then compare them (in the case of a complex event - they are compared as strings, otherwise the symbol vectors are simply compared together with the edge).The algorithm starts its work immediately after the end of the elaboration of the AST. It is launched only in the absence of errors (
invalid
nodes) in it, as incorrect assumptions can be made during the analysis of clock signals.A clock signal is considered to be a signal that is present in the
EventList
sequence or properties, as well as those signals from the sensitivity list of the always block that are not used anywhere inside except for assertions.The algorithm is implemented as nested AST visitors. The main one is the
ClockResolutionVisitor
, which searches for clocking blocks (which override the clock event for the entire instance) by calling theClockingBlockVisitor
(checking the correctness of assertion sequences and properties inside it) for each instance of the module. It first callsMarkInvalidVisitor
to mark invalid nodes that slang may leave even in the absence of errors. When encountering an assertion, it callsConcurrentAssertionVisitor
, which traverses the property under the assertion and tries to infer a clock event for it, which in turn callsSequenceVisitor
to traverse and check clock properties of the sequence under the property.There are also 2 separate visitors -
AlwaysTimingVisitor
andAlwaysTimingCheckUseVisitor
, the first of which fills in inferred clocks fromSignalEventControl
, and the second one removes from inferred clocks those which are used outside of assertions - those that are not considered clocks according to the standard.