laforest / FPGADesignElements

A self-contained online book containing a library of FPGA design modules and related coding/design guides.
MIT License
401 stars 42 forks source link

Faster Arbiter_Priority #17

Closed BartHaagdorens closed 2 years ago

BartHaagdorens commented 2 years ago

The logic needed to keep the current grant if still requested is now after the Bitmask_Isolate_Rightmost_1_Bit module instead of before. This shortens the combinatorial path from grant_previous to grant, allowing for higher clock speeds.

This also addresses the concern raised by @laforest in https://github.com/laforest/FPGADesignElements/pull/16#issuecomment-993663413.

laforest commented 2 years ago

This is an interesting change (and good catch on removing the unnecessary "assign"). However, have you tested this change to confirm it results in a higher clock speed? (either in a stand-alone test harness, or in the actual system using the Arbiter)

BartHaagdorens commented 2 years ago

I am using the Arbiter in a Xilinx 7-series speed grade -1 device with a 155MHz clock. At a certain point in the design, Vivado couldn't close timing on the path from grant_previous to grant. With the proposed change, timing closure succeeded. So I concluded that, although both versions of the Arbiter seem functionally equivalent, Vivado couldn't reduce one into the other.

I tried to reproduce this today, but my design has evolved for 12 more days and Vivado can now close timing on both versions. However, Vivado still reports .100ns more slack with the proposed f755235 compared to the original c7c3e8b.

It obviously depends on the synthesis tool and the rest of the design, YMMV.

laforest commented 2 years ago

Agreed. Once there is a loop, it's not uncommon to have to manually move things around to get better optimization. Thank you for doing this work. Those results sound very good. I've accepted the change.

laforest commented 2 years ago

Just a note: since 'grant' is now a reg port, I added an initial block to give it an initial value of zero.