search

clash-lang / clash-compiler

Haskell to VHDL/Verilog/SystemVerilog compiler
https://clash-lang.org/
Other
1.44k stars 153 forks source link

Prelude.^ fails on Bit, BitVector 1, Unsigned 1 #1401

Open leonschoorl opened 4 years ago

leonschoorl commented 4 years ago 
Clash.Prelude> import qualified Prelude as P

Clash.Prelude P> 42  P.^  (0 :: Int)
1
Clash.Prelude P> 42  P.^  (1 :: Int)
42

Clash.Prelude P> 42  P.^  (0 :: Unsigned 1)
1
Clash.Prelude P> 42  P.^  (1 :: Unsigned 1)
*** Exception: divide by zero

Clash.Prelude P> 42  P.^  (0 :: BitVector 1)
1
Clash.Prelude P> 42  P.^  (1 :: BitVector 1)
*** Exception: divide by zero

Clash.Prelude P> 42  P.^  (0 :: Bit)
1
Clash.Prelude P> 42  P.^  (1 :: Bit)
^CInterrupted.  <- just loops and if you don't interrupt it will eat all you memory
leonschoorl commented 4 years ago

The divide by zero is because implementation of Prelude.^ uses Prelude.even and that doesn't work for types that can't represent the value 2, see https://github.com/clash-lang/clash-compiler/issues/959

This hang with Bit is because Bit's Integral instance is a hardcoded and doesn't throw divide by zero exceptions like the other instances do: https://github.com/clash-lang/clash-compiler/blob/d547dee8a33e2a3cddd7772bbf0df8e587f45d27/clash-prelude/src/Clash/Sized/Internal/BitVector.hs#L297-L303

leonschoorl commented 4 years ago

Ideally for us we would put the | y == 1 = x first as the first case in f in base's implementation of (^)

We should try these benchmarks to see if the order of the cases of this f are just an implementation detail or important for performance.

dramforever commented 4 years ago

Just to throw in an additional note, negate for Bit looks suspicious: It's complement rather than id

https://github.com/clash-lang/clash-compiler/blob/d547dee8a33e2a3cddd7772bbf0df8e587f45d27/clash-prelude/src/Clash/Sized/Internal/BitVector.hs#L281-L288

Is this intended? It isn't negation modulo 2 (which is id, which in turn justifies (+) = (-)), so it doesn't even match with BitVector 1. And also now 0 - x isn't - x.

ghci> negate 1 :: BitVector 1
1
ghci> negate 0 :: BitVector 1
0
ghci> negate 1 :: Bit
0
ghci> negate 0 :: Bit
1

Maybe whoever wrote that thought (- x) might be a handy shortcut, and to be honest, negating a (non-0) unsigned value would cause an overflow in the first place, so it probably wouldn't have worked in generic code anyway. But I thought I should bring it up.