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.

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

Keep casts of the form x ~ Integer #1928

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions changelog/2021-09-14T20_09_01+02_00_fix1927
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
FIXED: Don't error out on `counter ~ (Index numStages)` like equalities [#1927](https://github.com/clash-lang/clash-compiler/issues/1927)
2 changes: 2 additions & 0 deletions clash-ghc/src-ghc/Clash/GHC/GHC2Core.hs
Original file line number Diff line number Diff line change
Expand Up @@ -659,6 +659,8 @@ isSizedCast (TyConApp tc1 _) (TyConApp tc2 _) = do
,tc2 `hasKey` naturalTyConKey &&
tc1Nm == "Clash.Sized.Internal.Unsigned.Unsigned"
])
-- XXX: a work-around for issue #1927, the real fix is to handle all casts
isSizedCast (TyVarTy {}) (TyConApp tc2 _) = return (tc2 `hasKey` integerTyConKey)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this also need a case for Integer ~ x?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not for issue #1927 specifically.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, but would you trigger the same issue by changing counter ~ (Index numStages) to (Index numStages) ~ counter in the test? If so I think we should add the extra case to avoid weird (albeit unlikely) situations where someone's design fails unexpectedly unless they change the argument order to (~)

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 haven't tried. I'm somewhat uncomfortable with this PR to begin with. I'd rather properly support all casts.

Copy link
Contributor

Choose a reason for hiding this comment

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

True, supporting all casts would be the better option. How far was #1064 from being finished?

Copy link
Member Author

Choose a reason for hiding this comment

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

It went in the right direction. But I would want to redo some aspects, especially with regards to Tick. So squashCollectApp was there in terms of handling casts I think, but Ticks still need more care: depending on the type of info (source location, HDL naming, etc.) the ticks either should or shouldn't be distributed towards the arguments as we "squash" the entire application.

isSizedCast _ _ = return False

hasPrimCo :: Coercion -> C2C (Maybe Type)
Expand Down
1 change: 1 addition & 0 deletions tests/Main.hs
Original file line number Diff line number Diff line change
Expand Up @@ -533,6 +533,7 @@ runClashTest = defaultMain $ clashTestRoot
, clashLibTest "T779" def{hdlTargets=[Verilog]}
, outputTest "T1881" def{hdlSim=False}
, runTest "T1921" def{hdlTargets=[Verilog], hdlSim=False}
, runTest "T1927" def{hdlTargets=[Verilog], hdlSim=False}
] <>
if compiledWith == Cabal then
-- This tests fails without environment files present, which are only
Expand Down
35 changes: 35 additions & 0 deletions tests/shouldwork/Issues/T1927.hs
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
{-# LANGUAGE AllowAmbiguousTypes #-}
module T1927 where

import Clash.Prelude
import Control.Monad

topEntity
:: Signal System (Vec 1 (Signed 1))
-> Signal System (Maybe (Vec 1 (Signed 1, Signed 1)))
-> Clock System -> Reset System -> Enable System
-> Signal System (Maybe (Vec 1 (Signed 1, Signed 1)))
topEntity angles xs c r e = exposeClockResetEnable (machine @8 angles xs) c r e

machine
:: forall numStages pairs coord angle dom counter
. HiddenClockResetEnable dom
=> (1 <= numStages)
=> (KnownNat numStages, KnownNat pairs)
=> (Default coord, NFDataX coord)
=> (Default angle, NFDataX angle)
=> counter ~ (Index numStages)
=> Signal dom (Vec pairs angle)
-> Signal dom (Maybe (Vec pairs (coord, coord)))
-> Signal dom (Maybe (Vec pairs (coord, coord)))
machine thetas coords = mealy transition def $ liftA2 (liftM2 zip) coords (return <$> thetas)
where
transition :: (counter, Vec pairs ((coord, coord), angle))
-> Maybe (Vec pairs ((coord, coord), angle))
-> ( (counter, Vec pairs ((coord, coord), angle))
, Maybe (Vec pairs (coord, coord))
)
transition (k, xyzs) maybeInputs = case (k == 0, maybeInputs)
of (True, Nothing) -> ((0, xyzs), Just $ map fst xyzs)
(True, Just xyzs0) -> ((0, xyzs0), Just $ map fst xyzs)
(False, _) -> ((satSucc SatWrap k, xyzs), Nothing)