#2 PackWords uses wrong divisor

Open
opened 3 years ago by Bodigrim · 2 comments
Bodigrim commented 3 years ago

I think intoWords / outOfWords should divide by 2⁶⁴ instead of 64.

 {-# INLINE intoWords #-}
 intoWords :: forall (n :: Nat) .
   (KnownNat n, 1 <= n) =>
   Finite n -> VU.Vector Word
 intoWords = evalState (VU.replicateM (wordLength @(Finite n)) go) . fromIntegral @_ @Natural
tural
   where go = do remaining <- get
-                let (d, r) = quotRem remaining bitsPerWord
+                let (d, r) = quotRem remaining (1 `shiftL` bitsPerWord)
                 put d >> pure (fromIntegral r)

 {-# INLINE outOfWords #-}
 outOfWords :: forall (n :: Nat) .
   (KnownNat n) =>
   VU.Vector Word -> Finite n
 outOfWords v = evalState (VU.foldM' go 0 v) 1
   where go old w = do power <- get
                       let placeValue = power * fromIntegral w
-                      modify (* bitsPerWord)
+                      modify (* fromInteger (1 `shiftL` bitsPerWord))
                       return (old + placeValue)
I think `intoWords` / `outOfWords` should divide by 2⁶⁴ instead of 64. ```diff {-# INLINE intoWords #-} intoWords :: forall (n :: Nat) . (KnownNat n, 1 <= n) => Finite n -> VU.Vector Word intoWords = evalState (VU.replicateM (wordLength @(Finite n)) go) . fromIntegral @_ @Natural tural where go = do remaining <- get - let (d, r) = quotRem remaining bitsPerWord + let (d, r) = quotRem remaining (1 `shiftL` bitsPerWord) put d >> pure (fromIntegral r) {-# INLINE outOfWords #-} outOfWords :: forall (n :: Nat) . (KnownNat n) => VU.Vector Word -> Finite n outOfWords v = evalState (VU.foldM' go 0 v) 1 where go old w = do power <- get let placeValue = power * fromIntegral w - modify (* bitsPerWord) + modify (* fromInteger (1 `shiftL` bitsPerWord)) return (old + placeValue) ```
Koz Ross commented 3 years ago
Owner

Yeah, I think you're right. Does the same issue apply to PackWords and PackBytes?

Yeah, I think you're right. Does the same issue apply to `PackWords` and `PackBytes`?
Bodigrim commented 3 years ago
Poster

PackBytes looks alright to me.

Also, performance-wise it's better to shift by bitsPerWord rather than divide by 1 `shiftL` bitsPerWord.

`PackBytes` looks alright to me. Also, performance-wise it's better to shift by `bitsPerWord` rather than divide by ``1 `shiftL` bitsPerWord``.
Sign in to join this conversation.
No Milestone
No assignee
2 Participants
Loading...
Cancel
Save
There is no content yet.