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

Bug with MT_UNROLL_MORE #4

Open
mrmbernardi opened this issue Jun 27, 2022 · 3 comments
Open

Bug with MT_UNROLL_MORE #4

mrmbernardi opened this issue Jun 27, 2022 · 3 comments

Comments

@mrmbernardi
Copy link

The following code is bugged when MT_UNROLL_MORE is set.

    // i = [0 ... 226]
    while (i < DIFF)
    {
        /*
         * We're doing 226 = 113*2, an even number of steps, so we can safely
         * unroll one more step here for speed:
         */
        UNROLL(i + PERIOD);

#ifdef MT_UNROLL_MORE
        UNROLL(i + PERIOD);
#endif
    }

With MT_UNROLL_MORE, the last iteration begins with i set to 226. The loop enters and completes two more iterations, ending with i = 228, completing one more iteration than it should've. Interestingly, G++ is smart enough to produce a warning about this.

With MT_UNROLL_MORE, the following code fixes the bug and produces the same output as without MT_UNROLL_MORE:

    while (i < DIFF - 2)
    {
        /*
         * We're doing 226 = 113*2, an even number of steps, so we can safely
         * unroll one more step here for speed:
         */
        UNROLL(i + PERIOD);

#ifdef MT_UNROLL_MORE
        UNROLL(i + PERIOD);
#endif
    }
    UNROLL(i + PERIOD);
@zhao-shihan
Copy link

I agree - subscripts from 0 to 226 actually iterate 227 times.

@macvip
Copy link

macvip commented Apr 30, 2023

Please excuse my shallow understanding if I were wrong. Literally, the total execution times of UNROLL is determined by SIZE (see the following loop), so the above modifications will not affect the final execution result.

@mrmbernardi
Copy link
Author

The expression passed to UNROLL is different. I discovered this bug when I was comparing the output against std::mt19937 and it was different. Implementing the fix I mentioned brings this code back in line with the output of std::mt19937.

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

No branches or pull requests

3 participants