trueDualPortBlockRam# cleanup

[martijnbastiaan]

Originally as preparation for adding byte enable signals to trueDualPortBlockRam# (but this will go to clash-cores instead). I'd like to keep the separate commits so we can easily bisect if it turns out I messed something up. I'm publishing this as a separate PR as to not pollute the next one.

Still TODO:

• Write a changelog entry (see changelog/README.md)
• Check copyright notices are up to date in edited files

[DigitalBrains1]

So much more pleasant code to look at!

When we were discussing this stuff on Feb 10, I made a diff with some improvements I'd like to see. It's this:

--- a/clash-prelude/src/Clash/Explicit/BlockRam.hs
+++ b/clash-prelude/src/Clash/Explicit/BlockRam.hs
@@ -1332,7 +1332,7 @@ trueDualPortBlockRam# clkA enA weA addrA datA clkB enB weB addrB datB =
   writeRam enable addr dat mem
     | Left enaMsg <- enableUndefined
     , Left addrMsg <- addrUndefined
-    = let msg = "Unknown enable and address" <>
+    = let msg = "Write enable and address unknown" <>
                 "\nWrite enable error message: " <> enaMsg <>
                 "\nAddress error message: " <> addrMsg
        in ( Just (deepErrorX msg)
@@ -1377,31 +1377,26 @@ trueDualPortBlockRam# clkA enA weA addrA datA clkB enB weB addrB datB =
       conflict = getConflict enA_ enB_ weA_ addrA_ weB_ addrB_
 
       (datA1_,datB1_) = case conflict of
-        Just Conflict{cfWW=IsDefined True} ->
-          ( deepErrorX "trueDualPortBlockRam: conflicting write/write queries"
-          , deepErrorX "trueDualPortBlockRam: conflicting write/write queries" )
-        Just Conflict{cfWW=IsX} ->
-          ( deepErrorX "trueDualPortBlockRam: conflicting write/write queries"
-          , deepErrorX "trueDualPortBlockRam: conflicting write/write queries" )
+        Just Conflict{cfWW=IsDefined True} -> (wwErr, wwErr)
+        Just Conflict{cfWW=IsX} -> (wwErr, wwErr)
         _ -> (datA_,datB_)
 
-      (wroteA,ram1) = writeRam weA_ addrA_ datA1_ ram0
-      (wroteB,ram2) = writeRam weB_ addrB_ datB1_ ram1
+      (wroteA,!ram1) = writeRam weA_ addrA_ datA1_ ram0
+      (wroteB,!ram2) = writeRam weB_ addrB_ datB1_ ram1
 
       outA1 = case conflict of
-        Just Conflict{cfRWA=IsDefined True} ->
-          deepErrorX "trueDualPortBlockRam: conflicting read/write queries"
-        Just Conflict{cfRWA=IsX} ->
-          deepErrorX "trueDualPortBlockRam: conflicting read/write queries"
+        Just Conflict{cfRWA=IsDefined True} -> rwErr
+        Just Conflict{cfRWA=IsX} -> rwErr
         _ -> fromMaybe (ram0 `Seq.index` addrA_) wroteA
 
       outB1 = case conflict of
-        Just Conflict{cfRWB=IsDefined True} ->
-          deepErrorX "trueDualPortBlockRam: conflicting read/write queries"
-        Just Conflict{cfRWB=IsX} ->
-          deepErrorX "trueDualPortBlockRam: conflicting read/write queries"
+        Just Conflict{cfRWB=IsDefined True} -> rwErr
+        Just Conflict{cfRWB=IsX} -> rwErr
         _ -> fromMaybe (ram0 `Seq.index` addrB_) wroteB
 
+      wwErr = deepErrorX "trueDualPortBlockRam: conflicting write/write queries"
+      rwErr = deepErrorX "trueDualPortBlockRam: conflicting read/write queries"
+
       outA2 = if enA_ then outA1 else prevA
       outB2 = if enB_ then outB1 else prevB
       (as2,bs2) = go ram2 ticks as1 bs1 outA2 outB2

It harmonises the error msg between the output and the RAM, it moves repeating error messages into a definition, and it reduces ram to WHNF in goBoth since this is also done in goA and goB, so it seems like it should be done there as well.

Could you put it into this PR?

[DigitalBrains1]

Another case of differing error messages. If write enable is deasserted and address is unknown, we put the XException of the address on the output. If write enable is asserted, we simply output an XException with the text Unknown address on the output. I propose we pass the XException from the address there as well:

diff --git a/clash-prelude/src/Clash/Explicit/BlockRam.hs b/clash-prelude/src/Clash/Explicit/BlockRam.hs
index fcd3825c5..b86e0a6ce 100644
--- a/clash-prelude/src/Clash/Explicit/BlockRam.hs
+++ b/clash-prelude/src/Clash/Explicit/BlockRam.hs
@@ -1564,7 +1564,7 @@ trueDualPortBlockRam# clkA enA weA addrA datA clkB enB weB addrB datB =
        in writeRam True addr (deepErrorX msg) mem
     | enable
     , Left addrMsg <- addrUndefined
-    = ( Just (deepErrorX "Unknown address")
+    = ( addr `seq` error "Unreachable"
       , Seq.fromFunction (natToNum @nAddrs) (unknownAddr addrMsg) )
     | enable
     = (Just dat, Seq.update addr dat mem)

[christiaanb]

So I see you updated the expected simulation results for asyncRamSynchronizer: do we know which expected simulation results correspond to simulaton of the generated HDL? The old or the new values?

[DigitalBrains1]

I cannot imagine the previous model was correct. By introducing an artificial edge on clock B, you effectively sample from the future. You eat one sample at the beginning, meaning all other samples are shifted in time.

I don't think we looked at HDL simulation parity yet, but maybe Martijn did look at it. It's also still the question whether there is parity between simulators. But that should be covered by us generating X's in the non-deterministic situations.

[martijnbastiaan]

The previous model certainly wasn't correct; but the question remains valid. I'll try to replicate the unit tests in HDL.

[DigitalBrains1]

You should change the PR cover letter now that you have abandoned the idea of implementing HDL tool agnostic byte enables.

Also, I feel the changes to clash-prelude/src/Clash/Explicit/BlockRam.hs are such that you should increment the year in the copyright message.

[martijnbastiaan]

Can we do the error messages in a separate PR please? I don't have much time. I've applied the other suggestions.