Fix literal rendering of Index values #2816
Conversation
|
This fixes the hang that Felix experienced. But I've still left this as a draft, as the overflow behaviour still isn't right, or at least not the same as in Haskell. topEntity = 6 :: Index 5Evaluating this in Haskell produces: But clash will just generate HDL with the 6 in it: assign result = 3'd6;Should it just produce the same HDL as if one would call assign result = {3 {1'bx}};or: result <= Index_topEntity_types.index_5'(0 to 2 => '-'); |
There was confusion between the size in bits and the type level argument to Index. Fixes #2813
leonschoorl
force-pushed
the
fix-index-lit-rendering-verilog
branch
from
1bc05cd
to
af3f884
Compare
leonschoorl
changed the title
Let me just note that the contract is "When Clash produces an I'll not touch the issue whether it's desired. |
|
I've now added the overflow detection too |
| @@ -1228,7 +1228,9 @@ expr_ _ (BlackBoxE pNm _ _ _ _ bbCtx _) | |||
| expr_ _ (BlackBoxE pNm _ _ _ _ bbCtx _) | |||
| | pNm == "Clash.Sized.Internal.Index.fromInteger#" | |||
| , [Literal _ (NumLit n), Literal _ i] <- extractLiterals bbCtx | |||
| = exprLitSV (Just (Index (fromInteger n),fromInteger n)) i | |||
| , Just k <- clogBase 2 n | |||
| , let k' = max 1 k | |||
There was a problem hiding this comment.
When would k be zero in this code path?
I feel like that only happens for types like Index 1 which shouldn't render in the first place?
There was a problem hiding this comment.
I was basically copying what the VHDL backend does:
clash-compiler/clash-lib/src/Clash/Backend/VHDL.hs
Lines 1767 to 1772 in 3b755b9
There was a problem hiding this comment.
But I think you're right
There was a problem hiding this comment.
That code was added in version 0.6.15 6b365dc#diff-d636015ead3ec91b11144d7f18c90be9133810ab83457b3649a91e046cfae73fR647
Which might be from a time before we elided 0-bit values?
There was a problem hiding this comment.
Yeah probably.
I had seen that, at least in simple cases clash, removes Index 1 literals because they're 0 bits wide values.
But I'm wasn't sure if there was some code path I didn't foresee. And assumed the author of that code in VHDL.hs knew what they were doing.
You want me to take out the clogBase?
leonschoorl
force-pushed
the
fix-index-lit-rendering-verilog
branch
from
af3f884
to
d7150eb
Compare
| @@ -0,0 +1,2 @@ | |||
| FIXED: Clash hanging when rendering `Index n` literals, for large values of `n` [#2813](https://github.com/clash-lang/clash-compiler/issues/2813) | |||
| FIXED: Render overflowed Index literals as errors | |||
There was a problem hiding this comment.
| FIXED: Render overflowed Index literals as errors | |
| FIXED: Render overflowed Index literals as don't-cares in HDL |
?
Maybe that's not good either, but I worry people might not understand what "render as an error" means. I'd also like to emphasise that we're talking about HDL here for clarity.
There was confusion between the size in bits and the type level argument to Index.
Causing it to use way too big numbers in the overflow calculation.
Fixes #2813
Still TODO: