Skip to content

Remove a 'unsafeCoerce' from 'Unsafe.coerce' #330

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 11, 2021

Conversation

utdemir
Copy link
Contributor

@utdemir utdemir commented Jun 11, 2021

Previously, the linear Unsafe.coerce was implemented using two unsafeCoerce's, one to get a function a -> b, and another one to cast that function to be a linear one.

After the unsafe equality proofs change, we have a unsafeEqualityProof function to cast two types to be the same without looking at the value, so we can linearly return it, removing the need for the outer unsafeCoerce.

This removes one allocation from the generated STG code for Data.Array.Unlifted.get, improving the performance slightly.

Previously, the linear `Unsafe.coerce` was implemented using two
`unsafeCoerce`'s, one to get a function `a -> b`, and another one to
cast that function to be a linear one.

After the unsafe equality proofs change[1], we have a
`unsafeEqualityProof` function to cast two types to be the same without
looking at the value, so we can linearly return it, removing the need
for the outer `unsafeCoerce`.

This removes one allocation from the generated STG code for
'Data.Array.Unlifted.get', improving the performance slightly.

[1]: https://gitlab.haskell.org/ghc/ghc/-/commit/74ad75e87317196c600dfabc61aee1b87d95c214
@utdemir utdemir requested a review from aspiwack June 11, 2021 01:46
coerce a =
case NonLinear.unsafeEqualityProof @a @b of
NonLinear.UnsafeRefl -> a
{-# INLINE coerce #-}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This INLINE actually does not have an effect on my test case, since GHC already was inlining it even without. But I thought it's better to be explicit.

Copy link
Member

@aspiwack aspiwack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat.

Does it help with the benchmarks? [edit: you've already answered this question, sorry, I'm evidently distracted]

@utdemir
Copy link
Contributor Author

utdemir commented Jun 11, 2021

Does it help with the benchmarks? [edit: you've already answered this question, sorry, I'm evidently distracted]

I didn't give too much detail :). It does help the benchmarks but only a little bit. I saw that there ends up being one less allocation on Data.Array.Unlifted.Linear.get, but on my microbenchmark it only ended up being about %5 faster. The only reason I can think of is that, because there still is a unsafeCoerce at the same place, we only save one allocation but it doesn't cascade into other optimisations.

@utdemir utdemir merged commit 566a804 into master Jun 11, 2021
@utdemir utdemir deleted the ud/better-unsafe-coerce branch June 11, 2021 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants