Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix literal rendering of Index values #2816

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix literal rendering of Index values #2816

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1b40def
Fix (System)Verilog literal rendering of Index values
leonschoorl Sep 30, 2024
d7150eb
Render overflowed Index literals as errors
leonschoorl Oct 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelog/2024-10-01T14_41_36+02_00_fix_index_literal_rendering
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

8 changes: 5 additions & 3 deletions clash-lib/src/Clash/Backend/SystemVerilog.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{-|
Copyright : (C) 2015-2016, University of Twente,
2017-2018, Google Inc.,
2021-2023, QBayLogic B.V.,
2021-2024, QBayLogic B.V.,
2022 , Google Inc.
License : BSD2 (see the file LICENSE)
Maintainer : QBayLogic B.V. <[email protected]>
Expand Down Expand Up @@ -65,7 +65,7 @@ import Clash.Netlist.Types hiding (intWidth, usages,
import Clash.Netlist.Util
import Clash.Signal.Internal (ActiveEdge (..))
import Clash.Util
(SrcSpan, noSrcSpan, curLoc, makeCached, indexNote)
(SrcSpan, clogBase, noSrcSpan, curLoc, makeCached, indexNote)
import Clash.Util.Graph (reverseTopSort)

-- | State for the 'Clash.Backend.SystemVerilog.SystemVerilogM' monad:
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was basically copying what the VHDL backend does:

clash-compiler/clash-lib/src/Clash/Backend/VHDL.hs

Lines 1767 to 1772 in 3b755b9

expr_ _ (BlackBoxE pNm _ _ _ _ bbCtx _)
| pNm == "Clash.Sized.Internal.Index.fromInteger#"
, [Literal _ (NumLit n), Literal _ i] <- extractLiterals bbCtx
, Just k <- clogBase 2 n
, let k' = max 1 k
= exprLit (Just (Unsigned k',k')) i

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I think you're right

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That code was added in version 0.6.15 6b365dc#diff-d636015ead3ec91b11144d7f18c90be9133810ab83457b3649a91e046cfae73fR647

Which might be from a time before we elided 0-bit values?

Copy link
Member Author

@leonschoorl leonschoorl Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

= exprLitSV (Just (Index (fromInteger n),k')) i

expr_ b (BlackBoxE _ libs imps inc bs bbCtx b') =
parenIf (b || b') (Ap (renderBlackBox libs imps inc bs bbCtx <*> pure 0))
Expand Down
5 changes: 4 additions & 1 deletion clash-lib/src/Clash/Backend/VHDL.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1769,7 +1769,7 @@ expr_ _ (BlackBoxE pNm _ _ _ _ bbCtx _)
, [Literal _ (NumLit n), Literal _ i] <- extractLiterals bbCtx
, Just k <- clogBase 2 n
, let k' = max 1 k
= exprLit (Just (Unsigned k',k')) i
= exprLit (Just (Index n,k')) i

expr_ _ (BlackBoxE pNm _ _ _ _ bbCtx _)
| pNm == "Clash.Sized.Internal.Index.maxBound#"
Expand Down Expand Up @@ -1867,6 +1867,9 @@ exprLit (Just (hty,sz)) (NumLit i) = case hty of
| i < 0 -> "unsigned" <> parens ("std_logic_vector" <> parens ("to_signed" <> parens(integer i <> "," <> int n)))
| i < 2^(31 :: Integer) -> "to_unsigned" <> parens (integer i <> "," <> int n)
| otherwise -> "unsigned'" <> parens lit
Index n
| 0 <= i && i < n -> exprLit (Just (Unsigned sz, sz)) (NumLit i) -- reuse Unsigned implementation above
| otherwise -> hdlTypeErrValue hty
Signed n
| i < 2^(31 :: Integer) && i > (-2^(31 :: Integer)) -> "to_signed" <> parens (integer i <> "," <> int n)
| otherwise -> "signed'" <> parens lit
Expand Down
12 changes: 8 additions & 4 deletions clash-lib/src/Clash/Backend/Verilog.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ import Clash.Netlist.Types as N hiding (intWidth, usages,
import Clash.Netlist.Util
import Clash.Signal.Internal (ActiveEdge (..))
import Clash.Util
(SrcSpan, noSrcSpan, curLoc, indexNote, makeCached)
(SrcSpan, clogBase, noSrcSpan, curLoc, indexNote, makeCached)

-- | State for the 'Clash.Backend.Verilog.VerilogM' monad:
data VerilogState =
Expand Down Expand Up @@ -1151,7 +1151,9 @@ expr_ _ (BlackBoxE pNm _ _ _ _ bbCtx _)
expr_ _ (BlackBoxE pNm _ _ _ _ bbCtx _)
| pNm == "Clash.Sized.Internal.Index.fromInteger#"
, [Literal _ (NumLit n), Literal _ i] <- extractLiterals bbCtx
= exprLit undefValue (Just (Index (fromInteger n),fromInteger n)) i
, Just k <- clogBase 2 n
, let k' = max 1 k
= exprLitV (Just (Index (fromInteger n),k')) i

expr_ b (BlackBoxE _ libs imps inc bs bbCtx b') = do
parenIf (b || b') (Ap (renderBlackBox libs imps inc bs bbCtx <*> pure 0))
Expand Down Expand Up @@ -1216,14 +1218,16 @@ rtreeChain _ = Nothing
exprLitV :: Maybe (HWType,Size) -> Literal -> VerilogM Doc
exprLitV = exprLit undefValue

exprLit :: Lens' s (Maybe (Maybe Int)) -> Maybe (HWType,Size) -> Literal -> Ap (State s) Doc
exprLit :: Backend s => Lens' s (Maybe (Maybe Int)) -> Maybe (HWType,Size) -> Literal -> Ap (State s) Doc
exprLit _ Nothing (NumLit i) = integer i

exprLit k (Just (hty,sz)) (NumLit i0) = case hty of
Unsigned _
| i < 0 -> string "-" <> int sz <> string "'d" <> integer (abs i)
| otherwise -> int sz <> string "'d" <> integer i
Index _ -> int (typeSize hty) <> string "'d" <> integer i
Index n
| 0 <= i0 && i0 < n -> int (typeSize hty) <> string "'d" <> integer i0
| otherwise -> hdlTypeErrValue hty
Signed _
| i < 0 -> string "-" <> int sz <> string "'sd" <> integer (abs i)
| otherwise -> int sz <> string "'sd" <> integer i
Expand Down
7 changes: 5 additions & 2 deletions tests/shouldfail/Verification/SymbiYosys.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,13 @@ import Clash.Verification (RenderAs (YosysFormal), assert, checkI,
topEntity :: Clock System -> Reset System -> Enable System -> Signal System (Bool, Bool, Bool)
topEntity = exposeClockResetEnable go

go :: HiddenClockResetEnable dom => Signal dom (Bool, Bool, Bool)
go :: forall dom. HiddenClockResetEnable dom => Signal dom (Bool, Bool, Bool)
go =
let -- oops, 'b' is never lit
c = register (0 :: Index 15) (countSucc <$> c)
cI :: Signal dom (Index 15)
cI = register (0 :: Index 15) (countSucc <$> cI)
c :: Signal dom (Unsigned 4)
c = fmap bitCoerce cI
r = (< 10) <$> c
g = ((>= 10) .&&. (< 15)) <$> c
b = (>= 15) <$> c
Expand Down