-
Notifications
You must be signed in to change notification settings - Fork 155
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
Add support for Haskell types: Word(N), Int(N), and Char #108
Conversation
Where 'N' is defined by the architecture on which the compiler is going to run. Previously we always treated Int as a signed 32-bit number. With this change we will have a better match between simulation and eventual circuit, as most computers these days have 64-bit integers.
as long as literals fall within VHDLs integer range. Otherwise they must be printed as bitstrings.
Needed when the integer value that needs to be converted is larger than 2^31.
Add support for Haskell types: Word(N), Int(N), and Char
Do we support |
|
Oh, now that I think about it, the integer/natural use-cases I cared about we didn't synthesize anyways, (such as underlying type for |
On a different note, I am concerned about the final commit, that means two different developers on different arch could synthesize same code differently. Since clash is conceptually a cross-compiler, I am not sure inheriting host-arch stuff like that is a good idea. At the very least, might be good to ensure the prelude doesn't use any word/int as an implementation detail that would trigger such behavior. |
For example of both: https://github.com/clash-lang/clash-compiler/blob/master/clash-systemverilog/primitives/CLaSH.Prelude.RAM.json#L7-L8 ought to be unsigned, and definitely shouldn't be host-arch-dependent as a wrapper function hides the use of |
I will take a look at the primitives/prelude. Until now, I've just been kinda hoping that people don't use |
Also, the reason I made the translation of |
Re post 1: that all makes sense, sounds like we should be using |
Re post 2: Maybe the thing to do is just not support |
To elaborate, there was some talk with Rust on what the default integer type should be (rust-lang/rfcs#212 and maybe other places). One thing I learned is that 32-bit math is actually faster / just as fast on most x86-64 chips. IMO the only reason to use |
Well, yes, I don't want to use fromEnum :: Enum a => a -> Int |
Then I would do the warning so users can audit when these show up. Somewhat relatedly, how does
Does using index_word32 :: KnownNat n => Vec n a -> Word32 -> a
index_word32 xs i@(W32# n0) = sub xs n0
where
sub :: Vec m a -> Word32# -> a
sub Nil _ = undefined
sub (y `Cons` (!ys)) n = if isTrue# (n ==# 0#)
then y
else sub ys (n -# 1#)
{-# NOINLINE index_word32 #-}
(!!) :: (KnownNat n, Enum i) => Vec n a -> i -> a
xs !! i = if
| i' < 0 = error "CLaSH.Sized.Vector.(!!): negative index"
| i' > fromIntegral (maxBound :: Word32) :: Int = error "CLaSH.Sized.Vector.(!!): indices over 2^32-1 unsupported due to VHDL restrictions"
| i' > maxIndex xs = error (P.concat [ "CLaSH.Sized.Vector.(!!): index "
, show i
, " is larger than maximum index "
, show (maxIndex xs)
])
| otherwise = index_word32 xs
where i' = fromEnum i
{-# INLINE (!!) #-} |
So for the above code, I have no choice but to give a warning then... because It uses fromEnum :: Enum a => a -> Int I'm not saying I'm happy with the current situation. But it's a compromise. And like I said, the only time the |
I will add a flag, with which you can set the number of bits that are used to encode 'Int' and 'Integer', where it defaults to the "native" width of the machine the compiler is build for. And I'll update the documentation to highlight this aspect of the translation of standard Haskell data types. |
Good point about the would-be Furthermore, my experience just using Verilog and VHDL for synthesis led me to miss that for simulation purposes, Verilog and VHDL do something sane with out-of-bounds indexing, so there is no need for us to handle negative numbers instead of the output language. Seeing all that, I think the flag you propose is the best we can do. Thanks! |
* The default is the system's default word with * Integer is now treated as a Signed N-bit, instead of a HDL native integer This is a follow-up to the discussion in #108
Fix some broken links to the GHC manual
Add support for:
Int8
Int16
Int32
Int64
Word
Word8
Word16
Word32
Word64
Char