Add true, dual-port blockram #1726

martijnbastiaan · 2021-03-26T15:55:53Z

TODO:

VHDL primitive
SystemVerilog primitive ~~(same as Verilog?)~~
Test with clock B+ C
Test conflict behavior

christiaanb · 2021-03-31T17:30:58Z

@martijnbastiaan @leonschoorl the distinction between slow and fast seems off, at least, veryUnsafeSynchronizer doesn't have a notion of slow and fast:

veryUnsafeSynchronizer
  :: Int
  -- ^ Period of clock belonging to @dom1@
  -> Int
  -- ^ Period of clock belonging to @dom2@
  -> Signal dom1 a
  -> Signal dom2 a
veryUnsafeSynchronizer t1 t2
  -- this case is just an optimization for when the periods are the same
  | t1 == t2 = same

  | otherwise = go 0

  where
  same :: Signal dom1 a -> Signal dom2 a
  same (s :- ss) = s :- same ss

  go :: Int -> Signal dom1 a -> Signal dom2 a
  go relativeTime (a :- s)
    | relativeTime <= 0 = a :- go (relativeTime + t2) (a :- s)
    | otherwise = go (relativeTime - t1) s

so when t1 is larger than t2 (i.e. t1 is slower than t2) then you need to add t2 multiple times to relativeTime to catch up to the subtraction of one t1 from relativeTime. Whereas when t2 is larger than t1 (i.e. t2 is slower than t1) then you need to subtract t1 multiple times from relativeTime to catch up to the addition of one t2 to relativeTime.

I guess what I'm trying to say is, given that the dual port ram model takes after veryUnsafeSynchronizer:

Is this concept of slow and fast clocks something we really need? or
Can we just make the portA and portB behavior symmetric? i.e. it behaves correctly regardless of whether portA is faster than portB or partA is faster than portB

martijnbastiaan · 2021-03-31T18:53:31Z

The purpose of the slow/fast clocks is simply because we know the slow clock will consume exactly one cycle from one of its streams. I.e., the next iteration of go will be spent in the section handling the fast clock. This allows us to set the "current conflict" back to Nothing:

clash-compiler/clash-prelude/src/Clash/Explicit/BlockRam.hs

Line 1258 in efe1208

(as2, bs2) = go Nothing ram1 (relativeTime + tA) as1 bs0

Without switching the clocks at the very start you'd need to check explicitly whether the "next" tick is going to be spent in the "other" part of go (or precalculate how many cycles you spend, but that's ugly IMO as you need to do it again and again).

christiaanb · 2021-03-31T19:49:54Z

Alright, that makes sense.

clash-prelude/src/Clash/Explicit/BlockRam.hs

clash-prelude/src/Clash/Signal/Internal.hs

clash-prelude/src/Clash/Explicit/BlockRam.hs

rowanG077 · 2021-07-26T22:20:40Z

clash-prelude/src/Clash/Explicit/BlockRam.hs

+
+  -> Clock domB
+  -- ^ Clock for port B
+  -> Signal domB (Maybe a)


I would rather have a new type for this or an Either. I assume if you pass in a Just here then it means you want to write and if you pass Nothing then a value is only read.

@rowanG077 What do you think of this new API?
2c0411e
It's isomorphic with Either but a bit more descriptive

@leonschoorl Thx! This is exactly what I meant. There can be no misunderstanding now.

There can be no misunderstanding now

Famous last words

christiaanb · 2021-10-22T12:42:35Z

For a next PR/Issue:

Investigate whether Vivado and Quartus still infer TDP RAM when you switch to same-port read-first (and perhaps no-change) behavior
- If this doesn't work: create new vendor-specific primitives
Investigate whether Vivado and Quartus still infer TDP RAM when you add port-enables
- If this doesn't work: create new vendor-specific primitives
Create configurable Haskell models so you can switch between same-port ReadFirst, WriteFirst (and perhaps NoChange)
If we need to create vendor-specific primitives: created Haskell TDP RAM with a configurable Vendor

christiaanb

LGTM modulo some minor comments

clash-prelude/src/Clash/Explicit/BlockRam.hs

tests/shouldwork/Signal/DualBlockRam0.hs

clash-prelude/src/Clash/Explicit/BlockRam.hs

tests/src/Test/Tasty/Clash/CollectSimResults.hs

clash-prelude/src/Clash/Explicit/BlockRam.hs

I want an implicitly clocked version as well.

christiaanb

I would like to see an implicitly same-clocked counterpart of Clash.Explicit.BlockRam.trueDualPortBlockRam in Clash.Prelude.BlockRam

christiaanb

LGTM modulo the minor comments.

christiaanb · 2021-10-25T14:51:04Z

clash-prelude/src/Clash/Explicit/BlockRam.hs


 {-# LANGUAGE Trustworthy #-}

 {-# OPTIONS_GHC -fplugin GHC.TypeLits.KnownNat.Solver #-}
 {-# OPTIONS_HADDOCK show-extensions #-}

+--Prevent generation of eta port names for trueDualPortBlockRam


Suggested change

--Prevent generation of eta port names for trueDualPortBlockRam

-- Prevent generation of eta port names for trueDualPortBlockRam

christiaanb · 2021-10-25T14:56:24Z

clash-prelude/src/Clash/Prelude/BlockRam.hs

+trueDualPortBlockRam ::
+  forall nAddrs dom  a .
+  ( HasCallStack
+  , KnownNat nAddrs
+  , HiddenClock dom
+  , NFDataX a
+  )
+  => Signal dom (E.RamOp nAddrs a)
+  -- ^ ram operation for port A
+  -> Signal dom (E.RamOp nAddrs a)
+  -- ^ ram operation for port B
+  -> (Signal dom a, Signal dom a)
+  -- ^ Outputs data on /next/ cycle. When writing, the data written
+  -- will be echoed. When reading, the read data is returned.
+trueDualPortBlockRam inA inB = E.trueDualPortBlockRam hasClock hasClock inA inB


Suggested change

trueDualPortBlockRam ::

forall nAddrs dom a .

( HasCallStack

, KnownNat nAddrs

, HiddenClock dom

, NFDataX a

)

=> Signal dom (E.RamOp nAddrs a)

-- ^ ram operation for port A

-> Signal dom (E.RamOp nAddrs a)

-- ^ ram operation for port B

-> (Signal dom a, Signal dom a)

-- ^ Outputs data on /next/ cycle. When writing, the data written

-- will be echoed. When reading, the read data is returned.

trueDualPortBlockRam inA inB = E.trueDualPortBlockRam hasClock hasClock inA inB

trueDualPortBlockRam ::

#ifdef CLASH_MULTIPLE_HIDDEN

forall nAddrs dom1 dom2 a .

( HasCallStack

, KnownNat nAddrs

, HiddenClock dom

, NFDataX a

)

=> Signal dom1 (E.RamOp nAddrs a)

-- ^ ram operation for port A

-> Signal dom2 (E.RamOp nAddrs a)

-- ^ ram operation for port B

-> (Signal dom1 a, Signal dom2 a)

-- ^ Outputs data on /next/ cycle. When writing, the data written

-- will be echoed. When reading, the read data is returned.

trueDualPortBlockRam inA inB = E.trueDualPortBlockRam (hasClock @dom1) (hasClock @dom2) inA inB

#else

forall nAddrs dom a .

( HasCallStack

, KnownNat nAddrs

, HiddenClock dom

, NFDataX a

)

=> Signal dom (E.RamOp nAddrs a)

-- ^ ram operation for port A

-> Signal dom (E.RamOp nAddrs a)

-- ^ ram operation for port B

-> (Signal dom a, Signal dom a)

-- ^ Outputs data on /next/ cycle. When writing, the data written

-- will be echoed. When reading, the read data is returned.

trueDualPortBlockRam inA inB = E.trueDualPortBlockRam hasClock hasClock inA inB

#endif

christiaanb · 2021-10-25T15:05:38Z

changelog/2021-10-25T10_31_41+02_00_add_true_dualport_ram

@@ -0,0 +1 @@
+ADDED: Added basic support for true dual port block ram in WriteFirst mode.


Suggested change

ADDED: Added basic support for true dual port block ram in WriteFirst mode.

ADDED: Added support for true dual ported block ram:

* Implicitly clocked: `Clash.Prelude.BlockRam.trueDualPortBlockRam` and

* Explicitly clocked: `Clash.Explicit.BlockRam.trueDualPortBlockRam`

Any values that's being written on a particular port is also the value that will be read on that port, i.e. the same-port read/write behavior is: WriteFirst. For mixed port read/write, when both ports have the same address, when there is a write on the port A, the output of port B is undefined, and visa versa.

christiaanb · 2021-10-25T15:08:22Z

clash-prelude/src/Clash/Explicit/BlockRam.hs

+-- | Produces vendor-agnostic HDL that will be inferred as a true, dual port
+-- block ram. It has write-first (or "new data") behavior, no clock enables,
+-- no byte enables, and is symmetric in its data ports.


Suggested change

-- | Produces vendor-agnostic HDL that will be inferred as a true, dual port

-- block ram. It has write-first (or "new data") behavior, no clock enables,

-- no byte enables, and is symmetric in its data ports.

-- | Produces vendor-agnostic HDL that will be inferred as a true, dual port

-- block ram. Any values that's being written on a particular port is also the

-- value that will be read on that port, i.e. the same-port read/write behavior

-- is: WriteFirst. For mixed port read/write, when both ports have the same

-- address, when there is a write on the port A, the output of port B is

-- undefined, and visa versa.

clash-prelude/src/Clash/Prelude/BlockRam.hs

alex-mckenna · 2021-10-25T18:25:27Z

@lmbollen You've set off the end of line whitespace check in these places:

./clash-prelude/src/Clash/Explicit/BlockRam.hs:389:{-# OPTIONS_GHC -fno-do-lambda-eta-expansion #-} 
./tests/shouldwork/Signal/DualBlockRamDefinitions.hs:25:

Add SystemVerilog / Verilog / VHDL primitives dualport bram: Move fast/slow domain ordering into the the simulation model This way the domain swapping never ends up in the HDL. Because the HDL primitives don't care about which domain is faster or slower. Add NOINLINEd wrapper around trueDualPortBlockRam# Vivado doesn't like to see bitslices in the path to the data input. Allow non-power-of-two sized rams Added read / write tests, conflict tests and non multiple clock frequency tests.

leonschoorl force-pushed the dual-port-blockram branch from 98b78ba to efe1208 Compare March 31, 2021 17:04

christiaanb reviewed Apr 1, 2021

View reviewed changes

clash-prelude/src/Clash/Explicit/BlockRam.hs Outdated Show resolved Hide resolved

martijnbastiaan commented Apr 6, 2021

View reviewed changes

clash-prelude/src/Clash/Signal/Internal.hs Outdated Show resolved Hide resolved

martijnbastiaan commented Apr 6, 2021

View reviewed changes

clash-prelude/src/Clash/Explicit/BlockRam.hs Show resolved Hide resolved

leonschoorl force-pushed the dual-port-blockram branch from efe1208 to b12210e Compare April 12, 2021 16:25

alex-mckenna reviewed Apr 15, 2021

View reviewed changes

clash-prelude/src/Clash/Explicit/BlockRam.hs Outdated Show resolved Hide resolved

leonschoorl force-pushed the dual-port-blockram branch 2 times, most recently from afe8308 to 0ae350f Compare April 19, 2021 12:49

leonschoorl force-pushed the dual-port-blockram branch from 0ae350f to de8e52e Compare June 1, 2021 09:04

leonschoorl force-pushed the dual-port-blockram branch from de8e52e to 18d9e70 Compare July 20, 2021 16:20

rowanG077 reviewed Jul 26, 2021

View reviewed changes

leonschoorl force-pushed the dual-port-blockram branch from 18d9e70 to 2c0411e Compare October 14, 2021 15:53

christiaanb marked this pull request as ready for review October 25, 2021 07:08

lmbollen force-pushed the dual-port-blockram branch 3 times, most recently from dae7e0c to 38a9b76 Compare October 25, 2021 08:45

christiaanb previously approved these changes Oct 25, 2021

View reviewed changes

christiaanb reviewed Oct 25, 2021

View reviewed changes

clash-prelude/src/Clash/Explicit/BlockRam.hs Outdated Show resolved Hide resolved

christiaanb requested changes Oct 25, 2021

View reviewed changes

christiaanb approved these changes Oct 25, 2021

View reviewed changes

lmbollen force-pushed the dual-port-blockram branch 4 times, most recently from 77748ac to 1cb4ac6 Compare October 26, 2021 11:33

lmbollen force-pushed the dual-port-blockram branch from 1cb4ac6 to e8c94df Compare October 26, 2021 11:53

lmbollen force-pushed the dual-port-blockram branch from e8c94df to 3e86156 Compare October 26, 2021 14:27

christiaanb merged commit fdfef78 into master Oct 27, 2021

christiaanb deleted the dual-port-blockram branch October 27, 2021 09:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add true, dual-port blockram #1726

Add true, dual-port blockram #1726

martijnbastiaan commented Mar 26, 2021 •

edited by lmbollen

Loading

christiaanb commented Mar 31, 2021 •

edited

Loading

martijnbastiaan commented Mar 31, 2021 •

edited

Loading

christiaanb commented Mar 31, 2021

rowanG077 Jul 26, 2021 •

edited

Loading

leonschoorl Oct 18, 2021

rowanG077 Oct 18, 2021

alex-mckenna Oct 18, 2021

christiaanb commented Oct 22, 2021 •

edited

Loading

christiaanb left a comment

christiaanb left a comment

christiaanb left a comment

christiaanb Oct 25, 2021

christiaanb Oct 25, 2021

christiaanb Oct 25, 2021

christiaanb Oct 25, 2021

alex-mckenna commented Oct 25, 2021

	--Prevent generation of eta port names for trueDualPortBlockRam
	-- Prevent generation of eta port names for trueDualPortBlockRam

		@@ -0,0 +1 @@
		ADDED: Added basic support for true dual port block ram in WriteFirst mode.

-ADDED: Added basic support for true dual port block ram in WriteFirst mode.
+ADDED: Added support for true dual ported block ram:
+  * Implicitly clocked: `Clash.Prelude.BlockRam.trueDualPortBlockRam` and
+  * Explicitly clocked: `Clash.Explicit.BlockRam.trueDualPortBlockRam`
+Any values that's being written on a particular port is also the value that will be read on that port, i.e. the same-port read/write behavior is: WriteFirst. For mixed port read/write, when both ports have the same address, when there is a write on the port A, the output of port B is undefined, and visa versa.

Add true, dual-port blockram #1726

Add true, dual-port blockram #1726

Conversation

martijnbastiaan commented Mar 26, 2021 • edited by lmbollen Loading

christiaanb commented Mar 31, 2021 • edited Loading

martijnbastiaan commented Mar 31, 2021 • edited Loading

christiaanb commented Mar 31, 2021

rowanG077 Jul 26, 2021 • edited Loading

Choose a reason for hiding this comment

leonschoorl Oct 18, 2021

Choose a reason for hiding this comment

rowanG077 Oct 18, 2021

Choose a reason for hiding this comment

alex-mckenna Oct 18, 2021

Choose a reason for hiding this comment

christiaanb commented Oct 22, 2021 • edited Loading

christiaanb left a comment

Choose a reason for hiding this comment

christiaanb left a comment

Choose a reason for hiding this comment

christiaanb left a comment

Choose a reason for hiding this comment

christiaanb Oct 25, 2021

Choose a reason for hiding this comment

christiaanb Oct 25, 2021

Choose a reason for hiding this comment

christiaanb Oct 25, 2021

Choose a reason for hiding this comment

christiaanb Oct 25, 2021

Choose a reason for hiding this comment

alex-mckenna commented Oct 25, 2021

martijnbastiaan commented Mar 26, 2021 •

edited by lmbollen

Loading

christiaanb commented Mar 31, 2021 •

edited

Loading

martijnbastiaan commented Mar 31, 2021 •

edited

Loading

rowanG077 Jul 26, 2021 •

edited

Loading

christiaanb commented Oct 22, 2021 •

edited

Loading