gtaylormb / opl3_fpga

Reverse engineered SystemVerilog RTL version of the Yamaha OPL3 (YMF262) FM Synthesizer
GNU Lesser General Public License v3.0
359 stars 43 forks source link

Small optimization in calc_rhythm_phase.sv? #8

Closed laanwj closed 6 years ago

laanwj commented 8 years ago

May be wrong here, but looks like it's possible to make rand_num one bit smaller by XORing in the (rightshifted) polynomial after the right shift.

        if (sample_clk_en && bank_num == 0 && op_num == 0)
            if (rand_num & 1)
                rand_num <= (rand_num ^ RAND_POLYNOMIAL) >> 1;
            else
                rand_num <= rand_num >> 1;
gtaylormb commented 6 years ago

But rand_num is the exact width of the polynomial (see line 58 in calc_rhythm_phase.sv). If you reduce it's width by 1, when you XOR with the polynomial you will be dropping the upper bit. You will also get a different result from the XOR as you are now XORing rand_num one bit to the right.

laanwj commented 6 years ago

Okay, thanks!

On Jan 6, 2018 18:35, "Greg Taylor" notifications@github.com wrote:

Closed #8 https://github.com/gtaylormb/opl3_fpga/issues/8.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/gtaylormb/opl3_fpga/issues/8#event-1412471238, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHutnHoMtoZ6WZlCDxIM5XZIhEkn5xaks5tH67wgaJpZM4IuGRI .

gtaylormb commented 6 years ago

Hold on. Funny I was just thinking about this, hours later, and I realized--there is a bug. Line 58 is incorrect. I should have used the $bits() function, not the $clog2() function. rand_num is in the incorrect bit width!

Thanks making me take a second look!

gtaylormb commented 6 years ago

Heh. No it was correct.

But good to always reaffirm the code!

Thanks.