Remove custom unsafeShift{L,R} definitions #281
Conversation
These have been available from base since v4.5.
|
IIRC the question in containers was whether these had the same inlining
behavior.
…On Thu, Jul 2, 2020, 6:59 AM Simon Jakobi ***@***.***> wrote:
These have been available from base since v4.5.
------------------------------
You can view, comment on, or merge this pull request online at:
#281
Commit Summary
- Remove custom unsafeShift{L,R} definitions
File Changes
- *M* Data/HashMap/Base.hs
<https://github.com/haskell-unordered-containers/unordered-containers/pull/281/files#diff-11321ff356efb0247b49763554dc8620>
(5)
- *D* Data/HashMap/UnsafeShift.hs
<https://github.com/haskell-unordered-containers/unordered-containers/pull/281/files#diff-83105b314ce0c55d85c792cfb92fd054>
(16)
- *M* unordered-containers.cabal
<https://github.com/haskell-unordered-containers/unordered-containers/pull/281/files#diff-7107455655dd1149b7449addd4f5ce6f>
(1)
Patch Links:
-
https://github.com/haskell-unordered-containers/unordered-containers/pull/281.patch
-
https://github.com/haskell-unordered-containers/unordered-containers/pull/281.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#281>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAOOF7OXXBT2F7PN5PKWZ7TRZRSBLANCNFSM4OOYT3NA>
.
|
Can you give more detail, or just a link to that discussion? I just had a look at the changes in the generated Core for Is there a better way to inspect the inlining behaviour here? |
|
Well, if GHC generates the same unfoldings with the same inlining guidance,
I think we should be good. As I recall, some of the bitty functions in base
don't/didn't have explicit INLINE pragmas, which can affect whether they
get inlined into unfoldings.
…On Thu, Jul 2, 2020, 11:51 AM Simon Jakobi ***@***.***> wrote:
IIRC the question in containers was whether these had the same inlining
behavior.
Can you give more detail, or just a link to that discussion?
I just had a look at the changes in the generated Core for
Data.HashMap.Base with GHC 8.10.1 and the only differences I could find
were in some source locations due to the removal of the import statement.
Is there a better way to inspect the inlining behaviour here?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#281 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAOOF7JUNBCU47TDDT4VMVDRZSUJVANCNFSM4OOYT3NA>
.
|
|
I just compared the Core generated with 7.8.4 (the oldest GHC version we support) and there are quite a lot of differences. I'll have to take a closer look. Maybe we can figure out the earliest GHC version that does the desired inlining and move the custom EDIT: Relevant links: haskell/containers#569, haskell/containers#216, https://gitlab.haskell.org/ghc/ghc/-/issues/12022 |
|
No. CPP is for compatibility, not extreme performance. I don't care if there are perf regressions on 7.8. |
|
In order to get more insight into the inlining behaviour, I've extracted this bit of code: -- shift.hs
module M (mask) where
import Data.Bits
import Data.Word
bitsPerSubkey :: Int
bitsPerSubkey = 4
subkeyMask :: Word
subkeyMask = 1 `unsafeShiftL` bitsPerSubkey - 1
mask :: Word -> Int -> Int
mask w s = 1 `unsafeShiftL` index w s
{-# INLINE mask #-}
index :: Word -> Int -> Int
index w s = fromIntegral $ unsafeShiftR w s .&. subkeyMask
{-# INLINE index #-}I've compared the Core produced for this module with mask [InlPrag=INLINE (sat-args=2)] :: Word -> Int -> Int
[GblId,
Arity=2,
Caf=NoCafRefs,
- Str=DmdType <S(S),1*U(U)><S(S),1*U(U)>m,
+ Str=<S(S),1*U(U)><S(S),1*U(U)>m,
Unf=Unf{Src=InlineStable, TopLvl=True, Value=True, ConLike=True,
WorkFree=True, Expandable=True,
Guidance=ALWAYS_IF(arity=2,unsat_ok=False,boring_ok=False)
- Tmpl= \ (w [Occ=Once] :: Word) (s [Occ=Once] :: Int) ->
- Data.Bits.$fBitsInt_$cunsafeShiftL
- (GHC.Types.I# 1#)
- ($ @ 'GHC.Types.PtrRepLifted
- @ Word
- @ Int
- (\ (ds [Occ=Once!] :: Word) ->
- case ds of _ [Occ=Dead] { GHC.Types.W# x# [Occ=Once] ->
- GHC.Types.I# (GHC.Prim.word2Int# x#)
- })
- (Data.Bits.$fBitsWord_$c.&.
- (Data.Bits.$fBitsWord_$cunsafeShiftR w s) subkeyMask))}]
-mask =
- \ (w :: Word) (s :: Int) ->
- case w of _ [Occ=Dead] { GHC.Types.W# x# ->
- case s of _ [Occ=Dead] { GHC.Types.I# i# ->
- GHC.Types.I#
- (GHC.Prim.uncheckedIShiftL#
- 1#
- (GHC.Prim.word2Int#
- (GHC.Prim.and# (GHC.Prim.uncheckedShiftRL# x# i#) 15##)))
- }
- }
+ Tmpl= \ (w [Occ=Once!] :: Word) (s [Occ=Once!] :: Int) ->
+ case w of { GHC.Types.W# x# [Occ=Once] ->
+ case s of { GHC.Types.I# i# [Occ=Once] ->
+ GHC.Types.I#
+ (GHC.Prim.uncheckedIShiftL#
+ 1#
+ (GHC.Prim.word2Int#
+ (GHC.Prim.and# (GHC.Prim.uncheckedShiftRL# x# i#) 15##)))
+ }
+ }}]
+mask
+ = \ (w :: Word) (s :: Int) ->
+ case w of { GHC.Types.W# x# ->
+ case s of { GHC.Types.I# i# ->
+ GHC.Types.I#
+ (GHC.Prim.uncheckedIShiftL#
+ 1#
+ (GHC.Prim.word2Int#
+ (GHC.Prim.and# (GHC.Prim.uncheckedShiftRL# x# i#) 15##)))
+ }
+ }With 8.0.2 the unfolding still references the Based on this insight, I'd suspect that this PR would cause a performance deterioration with GHC < 8.2. |
Indeed benchmarks with GHC 8.0.2 seem to indicate a small hit, mostly on the order of 1–6%. I think the simplification might still be worth it. |
|
I was somewhat on the fence about this patch, since it kind of prioritizes maintainability over performance, but I guess I don't care enough about users of GHC 7.8, 7.10 and 8.0. |
These have been available from base since v4.5.