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

Fix for indexInt8OffAddr# potentially returning sized type in next GHC #760

Merged

Conversation

Ericson2314
Copy link
Contributor

@Ericson2314 Ericson2314 commented Dec 22, 2020

Here's how we fix this without CPP:

In old GHC:

I8# :: Int# -> Int8
indexInt8OffAddr# :: Addr# -> Int# -> Int#

In upcoming GHC 9.2:

I8# :: Int8# -> Int8
indexInt8OffAddr# :: Addr# -> Int# -> Int8#

So the "GLB" interface is:

exists alpha.
I8# :: alpha -> Int8
indexInt8OffAddr# :: Addr# -> Int# -> alpha

We we write a program against that, eliminating the black-box alpha with I8# and then converting to Int.

See https://gitlab.haskell.org/ghc/ghc/-/merge_requests/4492

@Ericson2314 Ericson2314 changed the title Fix for indexInt8OffAddr# returning sized type in next GHC Fix for indexInt8OffAddr# potentially returning sized type in next GHC Dec 22, 2020
@treeowl
Copy link
Contributor

treeowl commented Dec 22, 2020

I think we'd better put this on hold until upstream merges your MR.

@Ericson2314
Copy link
Contributor Author

Ericson2314 commented Dec 23, 2020

@treeowl So the preference from the GHC side is actually that upstream merges first to guarantee that:

  1. No commit on master of GHC will depend on submodule commits not gc rooted by master branches.
  2. There are no outstanding PRs blocking the next release of GHC.

But I am quite sympathetic that both those arguments go both ways, and no one wants to take the leap of faith referring to hypothetical changes in the other project.

I think renewing the social contract between GHC and it's dependencies is a larger conversation that should happen on a mailing list, but am I thinking of a "two-phase commit" approach where the library maintainer says "yes, I will merge this PR (not squash or rebase) if this GHC PR is merged", or even better where GitHub and GitLab better, grant the capability of executing that to to a GHC maintainer or automate it somehow. Then no one has to take the leap of faith, and now future release becomes potentially blocked.

Symmetry is restored, and in fact both @bgamari and have yet to be convinced by my proposal, so we have meta-symmetry too :).

@treeowl
Copy link
Contributor

treeowl commented Dec 23, 2020

@Ericson2314, if I make this change and your GHC MR isn't merged, then we'll be in exactly the same incompatibility boat, except everyone will think I'm braindead. Yes, we need a transition plan, and no, merging this PR as is strikes me as a rather bad one.

@Ericson2314
Copy link
Contributor Author

Ericson2314 commented Dec 23, 2020

@treeowl yes that is exactly the situation I am hoping to avoid for both GHC and libraries like containers.

The capability to grant wouldn't be for CanMerge 760, but for IsMerged GHC4492 => CanMerge 760. This is what makes it a two-phase commit. Likewise the promise or automation would be IsMerged GHC4492 => IsMerged 760.

@Ericson2314 Ericson2314 force-pushed the wip/ghc-9.2-sized-array-primops branch from 09c61ec to fc22725 Compare December 23, 2020 23:05
@Ericson2314 Ericson2314 force-pushed the wip/ghc-9.2-sized-array-primops branch from fc22725 to 7a4f89c Compare January 25, 2021 18:55
@Ericson2314
Copy link
Contributor Author

@treeowl I realized we can do this without CPP. At least the GHCs I've looked at, there are plenty of rewrite rules and the like to make this new version optimize to the same thing.

@treeowl
Copy link
Contributor

treeowl commented Jan 25, 2021 via email

@Ericson2314
Copy link
Contributor Author

Ericson2314 commented Jan 25, 2021

@treeowl If it's fine with you, I soon as well just stay with this version. The intent: "we read a Int8 from the array and then expand it into an Int" is clearer now.

Thanks for making time to give it the proper evaluation.

@treeowl
Copy link
Contributor

treeowl commented Jan 26, 2021

You have build failures. I also don't actually understand how this works, but I guess that's not important.

@Ericson2314 Ericson2314 force-pushed the wip/ghc-9.2-sized-array-primops branch 2 times, most recently from 9b5b4a9 to 7f9d92a Compare January 26, 2021 15:16
@Ericson2314
Copy link
Contributor Author

OK this should fix it. Looking at the source, I don't even understand how GHC.Exts.I8# is exported in newer base, but I do see how GHC.Int.I8# is.

I also don't actually understand how this works

Sorry. I updated the commit message and PR description to explain.

Here's how we fix this without CPP:

In old GHC:

  I8# :: Int# -> Int8
  indexInt8OffAddr# :: Addr# -> Int# -> Int#

In upcoming GHC 9.2:

  I8# :: Int8# -> Int8
  indexInt8OffAddr# :: Addr# -> Int# -> Int8#

So the "GLB" interface is:

  exists alpha.
  I8# :: alpha -> Int8
  indexInt8OffAddr# :: Addr# -> Int# -> alpha

We we write a program against that, eliminating the black-box `alpha`
with `I8#` and then converting to `Int`.
@Ericson2314 Ericson2314 force-pushed the wip/ghc-9.2-sized-array-primops branch from 7f9d92a to 8d9de76 Compare January 26, 2021 21:00
@Ericson2314
Copy link
Contributor Author

Added some CPP on the import to avoid unused import warnings.

@Ericson2314
Copy link
Contributor Author

@treeowl does the new description make sense to you? Any other things need clarification?

@treeowl treeowl merged commit 7fb91ca into haskell:master Feb 2, 2021
@treeowl
Copy link
Contributor

treeowl commented Feb 2, 2021

Looks good. Thanks!

@Ericson2314 Ericson2314 deleted the wip/ghc-9.2-sized-array-primops branch February 2, 2021 23:11
@Ericson2314
Copy link
Contributor Author

Thanks!!

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