Move std_mult_pipe to pipelined_mult. Add std_mult_seq.
#1742
Conversation
|
You might want to leave the old 'std_mult_pipe' for now since that is what's used by the head of circt. I can change the name back to that in my branch for now. But yeah I agree with the pipelined mult changes. |
| @@ -1,12 +1,12 @@ | |||
| extern "pipelined.sv" { | |||
| // A latency-sensitive multiplier that takes 4 cycles to compute its result. | |||
| static<4> primitive pipelined_mult ( | |||
| static<4> primitive pipelined_mult[WIDTH] ( | |||
There was a problem hiding this comment.
According to the new static interface (#1725), where static<n> states that the II of the module is n, this should be static<1> right? CC @calebmkim @paili0628
There was a problem hiding this comment.
Are we ready to use the newest iterations of the static interface @andrewb1999? That might be another layer of complication if we can't support it.
I'm not up to date on what #1725 entails, but wouldn't both II and latency need to be tracked by a static component as two separate attributes?
There was a problem hiding this comment.
I saw the comments about static referring to the II before but doesn't that prevent us from using pipelined primitives in a non-pipelined context? I almost think that latency and II should be two different parts of the static interface.
There was a problem hiding this comment.
Yeah, on the current plan, we would like to make this static<1>. However, you can still use them it a non-pipelined context (i.e., read the output of the multiplier after 4 cycles): you'll just have to know that yourself (i.e., you'll just have to know to read the outputs after four cycles, that information won't be exposed in the interface anywhere).
The the latency static<n> annotation will tell you the II. Maybe in the future it'll become a big enough problem where we'll expose two numbers: one for the latency and one for the II.
(Edited a typo)
There was a problem hiding this comment.
The the latency annotation will tell you the II.
Do you mean the static annotation here? If so, yeah that makes. The latency annotation should be a future addition and kind of approximates the kind of reasoning that Filament does.
primitives/binary_operators.futil
Outdated
| @@ -86,8 +86,7 @@ extern "binary_operators.sv" { | |||
| /// =================== Unsigned, Bitnum ========================= | |||
| /// Other unsigned bitnum primitives are found in the core library, | |||
| /// since they're required for FSM encoding. | |||
|
|
|||
| primitive std_mult_pipe<"state_share"=1>[WIDTH]( | |||
| primitive std_mult_seq<"state_share"=1>[WIDTH]( | |||
There was a problem hiding this comment.
Let's keep the original name and add a comment to #1596 to rename this primitive when we update the library.
| buff0 <= tmp_prod; | ||
| buff1 <= buff0; | ||
| buff2 <= buff1; | ||
| buff3 <= buff2; |
There was a problem hiding this comment.
Why is this extra buffer needed?
There was a problem hiding this comment.
It's not strictly needed, but 4 cycle is the natural pipelining of DSPs on modern xilinx devices in my understanding. If you don't have this buffer it is 3 cycle latency which gives a warning when using Vivado.
There was a problem hiding this comment.
Sounds good to me! There is a test that might start failing but GH is refusing to run it because this is not a branch within the repo. @matth2k I've invited you as a collaborator to the repo. Can you open a new branch in this repo?
There was a problem hiding this comment.
Got it. I accepted the invite. I'll work on this tomorrow or Sunday. I will just close this PR and open a new one later.
|
Take a look at #1748 |
I am trying to get these primitives settled into the main branch so more people can use the AMC/LoopSchedule tool flow. These are the primitives that work with @andrewb1999 's LoopSchedule code.
Is this how we want to deal with multi-cycle operators? Have a
seqversion inbinary_operators.futiland a pipelined version inpipelined.futil?