Skip to content
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

Add missing {int,word}2{Float,Double} rules on 64 bit architectures #203

Closed
sheaf opened this issue Sep 5, 2023 · 11 comments
Closed

Add missing {int,word}2{Float,Double} rules on 64 bit architectures #203

sheaf opened this issue Sep 5, 2023 · 11 comments
Labels
approved Approved by CLC vote base-4.19 Implemented in base-4.19 (GHC 9.8)

Comments

@sheaf
Copy link
Contributor

sheaf commented Sep 5, 2023

Context

We currently have rules to convert between Word# and Float#/Double# in the GHC.Float module (in base):

"Int# -> Integer -> Float#"
  forall x. integerToFloat# (IS x) = int2Float# x

"Int# -> Integer -> Double#"
  forall x. integerToDouble# (IS x) = int2Double# x

"Word# -> Integer -> Float#"
  forall x. integerToFloat# (integerFromWord# x) = word2Float# x

"Word# -> Integer -> Double#"
  forall x. integerToDouble# (integerFromWord# x) = word2Double# x

"Word# -> Natural -> Float#"
  forall x. naturalToFloat# (NS x) = word2Float# x

"Word# -> Natural -> Double#"
  forall x. naturalToDouble# (NS x) = word2Double# x #-}

However, since GHC 9.4, Word64 wraps Word64# instead of Word#. This means that these rules no longer fire when using Word64. This was the cause of regressions in the splitmix package reported in GHC ticket #23907.

Proposal

Add the following rules to GHC.Float to handle Word64# and Int64# on 64-bit architectures:

"Int64# -> Integer -> Float#"
  forall x. integerToFloat# (integerFromInt64# x) = int2Float# (int64ToInt# x)

"Int64# -> Integer -> Double#"
  forall x. integerToDouble# (integerFromInt64# x) = int2Double# (int64ToInt# x)

"Word64# -> Integer -> Float#"
  forall x. integerToFloat# (integerFromWord64# x) = word2Float# (word64ToWord# x)

"Word64# -> Integer -> Double#"
  forall x. integerToDouble# (integerFromWord64# x) = word2Double# (word64ToWord# x)

This proposal is implemented in GHC MR !11170.

Moreover, we propose to backport this change to GHC 9.6 and GHC 9.8.

Addendum 1

We do not propoe adding rules going through Natural at this stage, as this would require defining a new function naturalFromWord64#, and adding several other rules mirroring those for integerFromWord64#.

Addendum 2

Note that similar rules should also be implemented on 32 bit architectures, but that would require introducing new primops (see GHC ticket #23908), so we don't propose doing that for the moment.

@Bodigrim
Copy link
Collaborator

Bodigrim commented Sep 6, 2023

Given the importance of splitmix for random and QuickCheck and the intention to backport the change, if approved, to GHC 9.8, I'll wait a few days more and trigger a vote. Looks pretty unambiguous to me.

@sheaf
Copy link
Contributor Author

sheaf commented Sep 7, 2023

I've clarified in the proposal that we would like to backport this to GHC 9.6 and 9.8 to limit the impact of the performance regression as much as possible (given that we do not currently plan to make any more 9.4 releases).

@sheaf
Copy link
Contributor Author

sheaf commented Sep 12, 2023

@Bodigrim Would it be possible to move forward with a vote? As I said above, we would really prefer for this change to make it into 9.8.1.

@tomjaguarpaw
Copy link
Member

If we're voting, then it'll be +1 from me

@Bodigrim
Copy link
Collaborator

Dear CLC members, let's vote on the proposal to add missing RULES to fix the regression caused by changing data Word64 = Word64 Word# to data Word64 = Word64 Word64#.

@chshersh @mixphix @angerman @parsonsmatt @hasufell


+1 from me.

@parsonsmatt
Copy link

+1

4 similar comments
@chshersh
Copy link
Member

+1

@hasufell
Copy link
Member

+1

@angerman
Copy link

+1

@mixphix
Copy link
Collaborator

mixphix commented Sep 13, 2023

+1

@Bodigrim
Copy link
Collaborator

Thanks all, approved unanimously.

@Bodigrim Bodigrim added the approved Approved by CLC vote label Sep 13, 2023
pull bot pushed a commit to sysfce2/ghc that referenced this issue Sep 15, 2023
leafcathead pushed a commit to leafcathead/ghc that referenced this issue Dec 6, 2023
KDr2 pushed a commit to KDr2/ghc that referenced this issue Jan 9, 2024
csabahruska pushed a commit to grin-compiler/ghc-wpc that referenced this issue Feb 14, 2024
@Bodigrim Bodigrim added the base-4.19 Implemented in base-4.19 (GHC 9.8) label Jun 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Approved by CLC vote base-4.19 Implemented in base-4.19 (GHC 9.8)
Projects
None yet
Development

No branches or pull requests

8 participants