Duplicate code in uvmt_cv32e40p_debug_assert.sv

[MikeOpenHWGroup]

Hi @silabs-oysteink, this is a somewhat unfair question about code you contributed back in September of 2020.

Type

• Duplicated code

Issue Description

The file in question is uvmt_cv32e40p_debug_assert.sv. The duplicated code is shared by property p_cebreak_exception, starting on line 121 and property p_ebreak_exception, starting on line 136.

These are complex properties with only a single clause difference between them (and that difference is only one character, p_cebreak_exception vs. p_ebreak_exception. Given that, future maintenance of these properties could be problematic.

Perhaps these properties could be combined into a single property:

    // ebreak or c.ebreak without dcsr.ebreakm results in exception at mtvec
    // Exclude single stepping as the sequence gets very complicated
    property p_cebreak_exception;
        disable iff(cov_assert_if.debug_req_i | !cov_assert_if.rst_ni)
        $rose(cov_assert_if.is_ebreak||cov_assert_if.is_cebreak) && cov_assert_if.dcsr_q[15] == 1'b0 && !cov_assert_if.debug_mode_q  && cov_assert_if.is_decoding && cov_assert_if.id_valid &&
        !cov_assert_if.debug_req_i && !cov_assert_if.dcsr_q[2]
        |-> (decode_valid) [->1:2] ##0  !cov_assert_if.debug_mode_q && (cov_assert_if.mcause_q[5:0] === cv32e40p_pkg::EXC_CAUSE_BREAKPOINT) 
                                                                && (cov_assert_if.mepc_q == pc_at_ebreak) &&
                                                                   (cov_assert_if.id_stage_pc == cov_assert_if.mtvec);
    endproperty

Additional context

This was flagged by the AMIQ EDA Verissimo SV/UVM Linter. AMIQ runs lint checks of core-v-verif every six hours and publishes the results on their website. The link to the above check is here.

[silabs-oysteink]

Forwarding this to silabs-robin since he worked on the debug assertions more recently than me and has greater knowledge of SVA.

[MikeOpenHWGroup]

Hi @silabs-robin can you have a look at this?

[silabs-robin]

Perhaps these properties could be combined into a single property

If the code is truly as duplicated as mentioned, then yes. A single property which takes as many arguments as needed, that should fix the lint message while preserving the original behavior.

---

future maintenance of these properties could be problematic

My suggestion is to add a comment: Note: Before future maintenance, refactor with <name of other property>. (See gh issue 1084.)

[MikeOpenHWGroup]

Thanks @silabs-robin. We took your advice (see #2346). I am advising that we keep this issue open for now and waive it for CV32E40P v2.0.0.

[MikeOpenHWGroup]

Another update. See #2347.

[MikeOpenHWGroup]

@YoanFournier pointed out that the combined property takes into account the different decoding signals by using $rose(cov_assert_if.is_ebreak||cov_assert_if.is_cebreak). Since both properties have the same effect, it should be equivalent.

I like this idea and leave it to the Dolphin team to decide whether to implement that fix now, or waive this issue.