clash-lang / clash-compiler

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

Differences between `shiftInAt0` and `shiftInAtN` #2836

Open kleinreact opened 1 month ago

kleinreact commented 1 month ago

I noticed some unexpected differences between shiftInAt0 and shiftInAtN:

We either should give both operations some symmetric behavior or we should explain, why we like them to be different on purpose.


rotateLeftS' :: forall d n a. KnownNat n => Vec n a -> SNat d -> Vec n a
rotateLeftS' input SNat = output
 where
  (output, feedback) = shiftInAtN @d @n @_ input feedback

rotateRightS' :: forall d n a. KnownNat n => Vec n a -> SNat d -> Vec n a
rotateRightS' input SNat = output
 where
  (output, feedback) = shiftInAt0 @n @_ @d input feedback

someInput :: Vec 10 Bit
someInput = unpack 0b1110011000

returns :: Vec 10 Bit
returns = rotateLeftS' someInput d2

diverges :: Vec 10 Bit
diverges = rotateRightS' someInput d2
christiaanb commented 1 week ago
Right, currently, Function Argument 1 Argument 2
shiftInAt0 lazy strict
shiftInAtN strict lazy

Which is why:

rotateRightS' :: forall d n a. KnownNat n => Vec n a -> SNat d -> Vec n a
rotateRightS' input SNat = output
 where
  (output, feedback) = shiftInAt0 @n @_ @d input feedback

diverges, as it uses its recursive result in the strict argument position. To make it not diverge you have to do:

rotateRightS' :: forall d n a. KnownNat n => Vec n a -> SNat d -> Vec n a
rotateRightS' input SNat = output
 where
  (output, feedback) = shiftInAt0 @n @_ @d input (lazyV feedback)

The reason for all of this is my/our desire to built functions within the Vector module out of existing functions in the Vector module; as opposed to adding a new recursive function. Though this is mostly coming from a position where:

  1. In the past, we would have to add a new primitive for every recursive function (this is no longer the case).
  2. In the present, in order to have fast compiles, it's better to have a primitive for a recursive function; but I want to avoid adding more primitives.

We can make the type arguments have the same order, so n m a. But otherwise I wouldn't want to make any changes in order to achieve symmetry. I would probably update the documentation to highlight the strictness of the arguments. I could be convinced otherwise though.

kleinreact commented 1 week ago

I'm fine with keeping the operations asymmetric with respect to their argument strictness, as long as this is clear to the user. It's just something that might be overseen easily otherwise.

We can make the type arguments have the same order, so n m a. But otherwise I wouldn't want to make any changes in order to achieve symmetry. I would probably update the documentation to highlight the strictness of the arguments.

:+1: