Use unsafeShift in BitUtil for base >= 4.8

[Rufflewind]

Use unsafeShiftL/R in BitUtil for base >= 4.8 instead of explicitly using GHC primitives, and also avoid the warning about Trustworthy.

[treeowl]

The comments accompanying those indicate the purpose is to ensure inlining.
The Bits Int instance does not INLINE those methods, so they won't be
inlined in unfoldings, I don't think.
On May 4, 2016 4:35 AM, "Phil Ruffwind" [email protected] wrote:

Use unsafeShiftL/R in BitUtil for base >= 4.8 instead of explicitly using

GHC primitives, and also avoid the warning about Trustworthy.

You can view, comment on, or merge this pull request online at:

#216
Commit Summary

• Use unsafeShift in BitUtil for base >= 4.8
• Avoid Trustworthy warning in BitUtil

File Changes

• M Data/Utils/BitUtil.hs
  https://github.com/haskell/containers/pull/216/files#diff-0 (15)

Patch Links:

• https://github.com/haskell/containers/pull/216.patch
• https://github.com/haskell/containers/pull/216.diff

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#216

[treeowl]

Of course, it's possible that's not necessary, but it would be good to be
sure.
On May 4, 2016 4:38 AM, "David Feuer" [email protected] wrote:

The comments accompanying those indicate the purpose is to ensure
inlining. The Bits Int instance does not INLINE those methods, so they
won't be inlined in unfoldings, I don't think.
On May 4, 2016 4:35 AM, "Phil Ruffwind" [email protected] wrote:

Use unsafeShiftL/R in BitUtil for base >= 4.8 instead of explicitly

using GHC primitives, and also avoid the warning about Trustworthy.

You can view, comment on, or merge this pull request online at:

#216
Commit Summary

• Use unsafeShift in BitUtil for base >= 4.8
• Avoid Trustworthy warning in BitUtil

File Changes

• M Data/Utils/BitUtil.hs
  https://github.com/haskell/containers/pull/216/files#diff-0 (15)

Patch Links:

• https://github.com/haskell/containers/pull/216.patch
• https://github.com/haskell/containers/pull/216.diff

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#216

[Rufflewind]

You're right. It doesn't seem to inline those … I don't understand why shift has INLINE but unsafeShift don't :/

https://hackage.haskell.org/package/base-4.8.2.0/docs/src/GHC.Word.html

shiftRL2 :: Word -> Int -> Word
…
shiftRL2 =
  \ (ds_d1VQ :: Word) (ds1_d1VR :: Int) ->
    case ds_d1VQ of _ [Occ=Dead] { W# x_a1MJ ->
    case ds1_d1VR of _ [Occ=Dead] { I# i_a1MK ->
    GHC.Types.W# (uncheckedShiftRL# x_a1MJ i_a1MK)
    }
    }

shiftRL :: Word -> Int -> Word
…
shiftRL = Data.Bits.$fBitsWord_$cunsafeShiftR

[treeowl]

The comment in Data.Bits is not terribly informative. I would think these should all be marked INLINE or INLINE CONLIKE. I suggest you open a ticket upstream to consider this and link from here. I'll leave this ticket open pending a decision upstream.

[Rufflewind]

I suggest you open a ticket upstream to consider this and link from here.

https://ghc.haskell.org/trac/ghc/ticket/12022

[m-renaud]

There doesn't appear to be any updates on the upstream ticktet and the Bits instance for Int in base-4.10.0.0 still doesn't have unsafeShiftL and unsafeShiftR marked INLINE. I'll follow up on the trac ticket when I get a chance.

[m-renaud]

See updated comments in https://ghc.haskell.org/trac/ghc/ticket/12022. It sounds like the advice is still to let the compiler determine when to INLINE. mpickering also points out that the shiftLL function has different semantics depending on if __GLASGOW_HASKELL__ is set or not.

I don't know enough about optimization or GHC semantics at this point to comment on what we should do here.

[sjakobi]

I think this can be closed. Today we have

shiftRL = unsafeShiftR
shiftLL = unsafeShiftL