Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix SV enum bug for dynamic SWG #810

Merged
merged 5 commits into from
Jun 23, 2023
Merged

Fix SV enum bug for dynamic SWG #810

merged 5 commits into from
Jun 23, 2023

Conversation

maltanar
Copy link
Collaborator

@maltanar maltanar commented May 4, 2023

When synthesizing DSWG instances with the latest dev Vivado raises the following error:

ERROR: [Synth 8-9123] an enum variable may only be assigned the same enum typed variable or one of its values [/scratch/finn/build/code_gen_ipgen_ConvolutionInputGenerator_rtl_0_cf9bvsk5/ConvolutionInputGenerator_rtl_0_impl.sv:73]

This is due to a variable in the generated SystemVerilog code

    // state and counters
    typedef enum logic [2:0] {
        STATE_START,
        STATE_LOOP_SIMD,
        STATE_LOOP_KW,
        STATE_LOOP_KH,
        STATE_LOOP_W,
        STATE_LOOP_H
    }  state_e;
    state_e  State = 1; // this value of 1 is not safe to use for the enum, STATE_LOOP_SIMD should be used

The fix is to use the corresponding enum values (e.g. STATE_LOOP_SIMD instead of 1). However, changing this in the codegen breaks the non-dynamic SWG since it uses the same value as a constructor parameter, so fixing that in turn requires moving the definition of the enum to the global scope. To avoid errors due to possible multiple definitions, I've added ifdef guards around it.

Note that the error was previously not detected because the DSWG unit tests do not include any synthesis. This PR adds synthesis to one of the DSWG testcases to detect future occurances of the issue.

@maltanar maltanar requested a review from preusser May 4, 2023 09:11
@auphelia auphelia requested a review from fpjentzsch May 11, 2023 08:36
Copy link
Collaborator

@preusser preusser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switched over to using a package rather than this enum in an `ifdef.
Tests seem to pass.
Would be happy to go with this version.

@auphelia auphelia merged commit 4e85d52 into dev Jun 23, 2023
3 checks passed
@auphelia auphelia deleted the fix/swg_sv_enum branch June 23, 2023 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants