VHDL: don't overflow integer range in RAM/ROM

[christiaanb]

Most RAM and ROM functions take an Int as either a read or write index. On 64-bit systems maxBound :: Int and minBound :: Int exceed VHDL's integer'high and integer'low (on (nearly) all VHDL simulators). This poses an issue, because the blackbox
implementations use to_integer to convert the signed (63 downto 0) (then translation for Int) to VHDL's integer. So any value falling outside of the range would trigger an overflow exception in the VHDL simulator.

So we now masks all indexes to fit the integer'low to integer'high range.

[martijnbastiaan]

@DigitalBrains1 Feel free to create a PR removing all instances of zeroAt while replacing it with ignoreFor. Seems a lot neater!

I don't understand this patch: wouldn't failing loudly be preferable over silently failing and introducing a Clash simulation vs RTL simulation difference? Wouldn't you only see this issue if you're surpassing your simulator's capabilities anyway?

[christiaanb]

@martijnbastiaan isn't the point of the patch clear from the testsuite addition? In Haskell, we wouldn't see the exception (X value) from indexing in the rom with a negative address, becausee we won't observe the output of the rom when the address is negative. However, when we naively translate to concurrent VHDL statements, the VHDL simulator would choke on those exceptions, in this case two even:

1. Indexing with an out-of-bound index (already handled before this pathc)
2. to_integer fails when the argument is outside of the integer range (handled in this patch)
   This will unlikely pose a problem in practice, until we get blockRAMs with more than 2^31-1 addressable words. It's also less of a problem because Haskell simulations will likely run out of memory when you have 2^31-1 addressable words.

[DigitalBrains1]

In Haskell, we wouldn't see the exception (X value) from indexing in the rom with a negative address, becausee we won't observe the output of the rom when the address is negative.

Are you sure about this? Maybe I'm misunderstanding what you are saying, but I don't see where that happens...

$ cabal run clashi
Up to date
Clashi, version 1.5.0 (using clash-lib, version 1.5.0):
https://clash-lang.org/  :? for help
Loaded package environment from /home/peter/src/clash/xilinxfloat/.ghc.environment.x86_64-linux-8.10.4
Loaded Clashi configuration from /home/peter/dotfiles-git/home/.clashi
Clash.Prelude P> :l RomNegative.hs 
[1 of 1] Compiling RomNegative      ( RomNegative.hs, interpreted )
Ok, one module loaded.
*RomNegative P> sampleN 10 testBench 
[False,False*** Exception: Ix{Int}.index: Index (-9223372036854775808) out of range ((0,255))
*RomNegative P> 

Restarting clashi and trying with printX didn't improve it:

*RomNegative P> printX $ sampleN 10 testBench 
[False,False*** Exception: Ix{Int}.index: Index (-9223372036854775808) out of range ((0,255))

[DigitalBrains1]

Thinking about it, it could very well be that the issue shows up if the testbench is changed a bit such that the output isn't observed when the address is negative, leading to a normally running testbench in Haskell, but failing in VHDL simulation because of the concurrent nature.

So perhaps the issue is real and the fix is fine as well, but the testbench needs tweaking.

[christiaanb]

Try running the test added in this PR on head of master, and you’ll see.

[DigitalBrains1]

The test fails in master when run through GHDL, and succeeds in this branch.

The test fails in both master and this branch when run in Clash.

The latter is the problem here (well, alternatively, there is no problem, because the test fails in Clash, so GHDL can do that as well).

[christiaanb]

Ah, right, the output needs to be ignored for 2 cycles, not just on the first. So it needs ignoreFor d2 0 instead of the zeroAtZero

[DigitalBrains1]

All addresses presented by the testBench are negative, so all of them will give an exception. As I mentioned on our Slack, I hit a wall while trying to change the test bench. I don't know why, but it insists on evaluating romFile even though the mux correctly behaves in the following.

--- a/tests/shouldwork/Signal/RomNegative.hs
+++ b/tests/shouldwork/Signal/RomNegative.hs
@@ -20,14 +20,14 @@ topEntity
   -> Signal System (Unsigned 8)
 topEntity = exposeClockResetEnable go where
   go rd = zeroAt0 (unpack <$> dout) where
-    dout = romFile d256 "memory.list" rd
+    dout = mux ((< 0) <$> rd) 0 (romFile d256 "memory.list" rd)
 {-# NOINLINE topEntity #-}
 
 testBench :: Signal System Bool
 testBench = done
   where
     testInput      = Explicit.register clk rst enableGen minBound (testInput + 1)
-    expectedOutput = outputVerifier' clk rst $(listToVecTH [0::Unsigned 8,0,1,2,3,4,5,6,7,8])
+    expectedOutput = outputVerifier' clk rst (replicate d10 0)
     done           = expectedOutput (topEntity clk rst enableGen testInput)
     clk            = tbSystemClockGen (not <$> done)
     rst            = systemResetGen

gives

>>> P.takeWhile not $ sample testBench
[False,False*** Exception: Ix{Int}.index: Index (-9223372036854775808) out of range ((0,255))

the following change makes it work but defeats the purpose:

--- a/tests/shouldwork/Signal/RomNegative.hs
+++ b/tests/shouldwork/Signal/RomNegative.hs
@@ -20,7 +20,7 @@ topEntity
   -> Signal System (Unsigned 8)
 topEntity = exposeClockResetEnable go where
   go rd = zeroAt0 (unpack <$> dout) where
-    dout = mux ((< 0) <$> rd) 0 (romFile d256 "memory.list" rd)
+    dout = mux ((< 0) <$> rd) 0 (romFile d256 "memory.list" ((.&. 255) <$> rd))
 {-# NOINLINE topEntity #-}
 
 testBench :: Signal System Bool

I have yet to discover where the strictness is coming from.

[DigitalBrains1]

And just to make it clear, no, using ignoreFor does not work for any value:

--- a/tests/shouldwork/Signal/RomNegative.hs
+++ b/tests/shouldwork/Signal/RomNegative.hs
@@ -18,9 +18,7 @@ topEntity
   -> Enable System
   -> Signal System (Signed 64)
   -> Signal System (Unsigned 8)
-topEntity = exposeClockResetEnable go where
-  go rd = zeroAt0 (unpack <$> dout) where
-    dout = romFile d256 "memory.list" rd
+topEntity = exposeClockResetEnable (fmap unpack . romFile d256 "memory.list")
 {-# NOINLINE topEntity #-}
 
 testBench :: Signal System Bool
@@ -28,6 +26,7 @@ testBench = done
   where
     testInput      = Explicit.register clk rst enableGen minBound (testInput + 1)
     expectedOutput = outputVerifier' clk rst $(listToVecTH [0::Unsigned 8,0,1,2,3,4,5,6,7,8])
-    done           = expectedOutput (topEntity clk rst enableGen testInput)
+    done           = expectedOutput . ignoreFor clk rst enableGen d2 0
+                       $ topEntity clk rst enableGen testInput
     clk            = tbSystemClockGen (not <$> done)
     rst            = systemResetGen

gives

>>> P.takeWhile not $ sample testBench
[False,False*** Exception: Ix{Int}.index: Index (-9223372036854775808) out of range ((0,255))

and for ignoreFor d20 (which should just give mismatched values because the output should be all zeroes for the length we run the test bench) it's the same:

>>> P.takeWhile not $ sample testBench 
[False,False*** Exception: Ix{Int}.index: Index (-9223372036854775808) out of range ((0,255))

The romFile is evaluated too eagerly, but the test bench's logic is wrong for any value of ignoreFor, it should be the mux from my previous comment.

[DigitalBrains1]

@martijnbastiaan thinks he knows why Clash is too strict here, I'll try a fix based on his suggestion. Although really, the fix should work regardless of the cause.

[DigitalBrains1]

There were two remaining issues which I just fixed:

• The new test bench in 4e139c4 did not test the "overflow VHDL integer range" problem at all.
• The order of commits, first fca9ac2 then 4e139c4, meant that if someone were to run the added test bench in Clash in fca9ac2, that would fail. While we don't run these test benches in Clash in current CI [1], it seems bad form to have a failing test bench committed to master. So I reversed the order.

[1] We use Clash to compile them, the simulation is then done in HDL, not in Clash.

[DigitalBrains1]

Ah, and I added a missing changelog and updated copyright messages :-)