Generated fromSLV isn't always total for sum types

[pbreuer]

I'm still getting reports that the vhdl generated from working Clash code is erroring at runtime in GHDL. The "army of testers" (hah!) is in a different timezone so it takes time to interact to pin it down and I do not know yet if the bug is here or elsewhere, but there is one.

GHDL is reporting "bound check failure" (which in earlier reports turned out to be a negative output from to_integer to somewhere not expecting it) in a generated library function, I think for Enum (or BitPack?):

function fromSLV (slv : in std_logic_vector) return A_Index is
begin
return A_Index'val(to_integer(unsigned(slv))); <-- HERE
end;

The A_Index target is a 'derive Enum'd data type consisting of just under 100 abstract values

data A_Index = A_J | ... | A_ILL

and it is represented in the vhdl by

type A_Index is (A_J, ..., A_ILL);

I believe slv as the fromSLV input must be a 7-bit bitvector in principle. My question is if "you" (collectively) see any scope for say, slv having the top bit set, leading perhaps to a negative result as the input to A_Index'val, which might explain things?

It doesn't happen when clash runs its own code, from which the vhdl is generated.

I think this used to be done via an array in earlier clash versions?

There will be further experiments done this evening that may pin the problem more exactly.

Any comments meanwhile?

Regards

PTB

[alex-mckenna]

Looks to be specific to Clash rendering VHDL enums, you can use -fclash-no-render-enums to get the old "an enum is just a logic vector" behaviour back

[pbreuer]

Thank you. I've placed that at the top of the list of things to try.
I'll report what happens later.
Regards
PTB

[pbreuer]

The -fclash-no-render-enum fixed that problem. It took over memorial weekend to run the tests to completion. Sorry about the report delay.

I didn't try any of the diagnostic changes that might have given more info, such as a '0'& on the front of the slv argument in the library function. Sorry.

I'm not confident the whole vhdl generation part of clash 1.6.3 is presently correct, because that thing about the top bit going through a to_integer to produce an unexpected negative result makes me shiver. Same with the lack of parentheses around "and" compounds (presumably also "or", not that any were generated). I added parentheses like crazy around all non-atomics that I could see in the templates, but that prevents me seeing more problems that may exist. Anyway, that was a different bug report than this.

Very many thanks for pointing at the fix so quickly.

Regards

PTB

PS. A puzzle is that the vhdl seems to be spending about 300 cycles doing nothing at startup, where the clash execution starts doing things at once. I presume this is known and expected, and is some kind of initialization, possibly of sram? prom? everything?

PPS. A problem is that i don't know how to make clash stop the vhdl simulation in ghdl. There is a way because one could just cause 0/0 to be executed and stop the simulation (with error or whatever vhdl says, assertion violation?). I think I've read of a more acceptable way of stopping ghdl and tried it, but it didn't do anything.

PPPS. I'd like to generate verilog and let you know how that goes, but I don't know how to make verilog make noise while running, as with the vhdl report function, which has been so useful in debugging and testing. Report is being generated from Clash via a reasonable class interface now.

PPPPS. There are minor timing differences between running in clash and running the generated vhdl in ghdl, but the 300 cycle startup may be altering on which system clock cycle output from the slower-clocked SRAM is seen, or similar. I haven't seen consistent differences one way or another, but I haven't consistently looked either. Perhaps I should. An alternative would be that Clash perhaps is seeing slower SRAM output on the faster cycle it becomes ready in, whereas ghdl waits for the last possible cycle. Or vice versa.

[alex-mckenna]

The -fclash-no-render-enum fixed that problem.

Good to hear, no worries about the delay

I'm not confident the whole vhdl generation part of clash 1.6.3 is presently correct, because that thing about the top bit going through a to_integer to produce an unexpected negative result makes me shiver.

Hopefully there's nothing inherently wrong with what's produced, but bugs do happen. The codegen in Clash always tries to produce things that should always work, so it can produce a lot of unnecessary type marks / conversions in VHDL to ensure data definitely has some type when it's used. If you can find any of these which are obviously wrong then an issue is appreciated so we can hunt it down.

Same with the lack of parentheses around "and" compounds (presumably also "or", not that any were generated).

Yes, the lack of proper precedence for logical operators in VHDL is annoying. I believe one of my colleagues has been looking at that recently.

A problem is that i don't know how to make clash stop the vhdl simulation in ghdl.

If you don't mind having to target VHDL 2008 instead of VHDL 1993 (as clash normally produces) you can write a black box which renders calls to the stop function in the new std.env package (GHDL supports this)

I'd like to generate verilog and let you know how that goes, but I don't know how to make verilog make noise while running

Verilog has some system tasks for this: $display(" ... ") works like printf in C I believe. There's also things like $dumpvars and $dumpfile("path.vcd") which can output VCD traces for loading into a waveform viewer. You can $finish; to stop a simulation early, and this doesn't require a more recent standard than 2001 (which Clash aims to generate).

PS. A puzzle is that the vhdl seems to be spending about 300 cycles doing nothing at startup
PPPPS. There are minor timing differences between running in clash and running the generated vhdl in ghdl

I'm afraid I wouldn't know about these things, sorry

[alex-mckenna]

Also, to keep issues more on track in the future. If you have general points / side questions it's more convenient to start a discussion at https://github.com/clash-lang/clash-compiler/discussions. The UI is a bit nicer there in terms of replying directly to a comment instead of just one linear string of messages like on an issue

[leonschoorl]

I can replicate it with toEnum:

import Clash.Prelude
import Clash.Explicit.Testbench

topEntity :: BitVector 2 -> Maybe A_Index
topEntity x | x == 3 = Nothing
            | otherwise = Just $ toEnum $ fromIntegral x

testBench :: Signal System Bool
testBench = done
  where
    testInput      = stimuliGenerator clk rst ( 0 :> 1 :> 2 :> 3 :> Nil)
    expectedOutput = outputVerifier clk rst (Just A_0 :> Just A_1 :> Just A_2 :> Nothing :> Nil)
    done           = expectedOutput (topEntity <$> testInput)
    clk            = tbSystemClockGen (not <$> done)
    rst            = systemResetGen


data A_Index = A_0 | A_1 | A_2
      deriving (Show,Bounded,Enum,Generic,BitPack,Eq,ShowX)

[pbreuer]

Yes, well done. I see that before the no-render-enums fix, I had intended to modify the default toEnum to make it more robust just in case the problem was the incoming argument

- deriving (Eq, Enum, BitPack, Generic, NFDataX, Show)
+ deriving (Eq, BitPack, Generic, NFDataX, Show)
+
+instance Enum A_Index
+ where toEnum x | x >= fromEnum A_ILL || x < 0 = A_ILL
+ | otherwise = unpack(truncateB(pack x))
+ fromEnum y = unpack(zeroExtend(pack y))

But I never got to test that on it own ... thank goodness. I recall tracing the template to somewhere where to/from were the other way round, being expressed in terms of the range type not the domain type, but that's about all I do recall now. Sorry.

PTB

[martijnbastiaan]

We've released v1.6.4, which includes a fix for this issue.