clash-testsuite ... Ram/RMultiTop includes top entity in test bench

[DigitalBrains1]

Instead of creating a test bench named e.g. vhdl/RMultiTop.testBench/testBench.vhdl that tests a vhdl/RMultiTop.topEntity/topEntity.vhdl, the former includes a copy of the latter.

It is rather odd that this faulty version of the code ended up in the commit and I know what to do to fix it.

I don't know the repercussions of the fault. I only know that for instance an Enable input on a top entity that is driven by enableGen in the test bench might get the enable elided from the final HDL. This will never happen with a properly declared top entity / test bench relation. But since it is generally expected that test benches do not include a copy of the top entities they test, there might be other repercussions? Alternatively it's just about sharing of work and it has no functional differences (other than the Enable).

[martijnbastiaan]

I'm not sure I entirely understand. testBench doesn't seem to use topEntity:

clash-compiler/tests/shouldwork/Signal/Ram/RMultiTop.hs

Lines 16 to 19 in 7843abe

	testBench
	:: Signal P10 Bool
	testBench = tb $(listToVecTH $ sampleN 20 $ tbOutput clockGen clockGen)
	{-# NOINLINE testBench #-}

It only uses tb which is explicitly marked INLINE:

clash-compiler/tests/shouldwork/Signal/Ram/RMulti.hs

Lines 35 to 47 in 7843abe

	tb
	:: ( KnownNat n
	, 1 <= n)
	=> Vec n (Unsigned 2)
	-> Signal P10 Bool
	tb expectedSamples = done
	where
	output = tbOutput wClk rClk
	expectedOutput = outputVerifier' rClk noReset expectedSamples
	done = expectedOutput output
	(rClk, wClk) = biTbClockGen (not <$> done) :: (Clock P10, Clock P20)
	noReset = unsafeFromHighPolarity @P10 (pure False)
	{-# INLINE tb #-}

Am I missing something? :)

[DigitalBrains1]

But tb indirectly includes a copy of ram and topEntity is just another copy of ram. So tb is testing something that is functionally topEntity but is not actually topEntity.

[DigitalBrains1]

Here, lemme show you what I mean. That PR should fix the issue. But I'm going to let it wait for a while until my brain is in a high enough gear to pick all the glaring mistakes from it (all might be zero).

Note that it seems that you need a trail of INLINE from testBench down to the function that "finally" includes topEntity. Omitting an INLINE will once again create a fresh copy of topEntity inside the test bench. It's rather brittle really.

[DigitalBrains1]

I've just created an issue about the INLINE thing: #2196.