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

Support GHC 9.4 #69

Closed
wants to merge 12 commits into from
Closed

Conversation

parsonsmatt
Copy link
Contributor

No description provided.

@erikd
Copy link
Owner

erikd commented Aug 23, 2022

@parsonsmatt Is is possible to remove the use of ViewPatterns? I would also like to drastically reduce the use of CCP conditional compilations. I have an example fo this in the erikd/32-bit branch where I define functions conditionally on whether Word is 32 or 64 bit. Pretty sure something similar could be done for this.

If you do not get to it in the next couple of days, I will have a go at doing it.

@parsonsmatt
Copy link
Contributor Author

Once I've got the work codebase on GHC 9.4 then I'll have time to come back around and apply fixups. I'd be thrilled to have less CPP for this, but I'm unsure how other approaches would potentially affect performance.

Removing ViewPatterns is certainly possible but would make the CPP worse in most places.

Copy link
Owner

@erikd erikd left a comment

Choose a reason for hiding this comment

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

Looks good but needs to be squashed down to a single commit.

Even then I would like to modify this to reduce the amount of CPP. But first I need to get ghc-9.4.

@parsonsmatt
Copy link
Contributor Author

ghcup has 9.4.2 available now.

Would you be willing to do a squash+merge on GitHub side of things? I'm not great at squashing on the CLI.

@erikd
Copy link
Owner

erikd commented Aug 29, 2022

@parsonsmatt Never mind, I will sort this out (I am really good at squashing things from the CLI).

As for ghcup, I build my own Debian packages (started this before ghcup was first released) and the build system has changed and I am still working through how to get it working again.

@parsonsmatt
Copy link
Contributor Author

@erikd I could try and squash it up if you'd like. If you have any suggestions on reducing CPP then I can take a stab at it.

@spacekitteh
Copy link

Any update?

@erikd
Copy link
Owner

erikd commented Nov 21, 2022

Sorry, no. At work I am still using ghc-8.10 and trying to upgrade to ghc-9.2, but still have not got as far as ghc-9.4.

@spacekitteh
Copy link

Alas :(

@parsonsmatt
Copy link
Contributor Author

I'd be happy to help push this forward. You requested earlier that I remove ViewPatterns and reduce CPP - if I do those things, will you merge+release?

I saw a trick to #define the conversion function, which would probably significant reduce CPP use (at the cost of, well, macro-ing things in). kowainik/typerep-map#124

@erikd
Copy link
Owner

erikd commented Nov 23, 2022

@parsonsmatt Absolutely. Would merge and release the solution was a little nicer than the original. Had even meant to do it myself, but then life got in the way.

@@ -65,6 +64,14 @@ import Data.Primitive.Types (Prim (..), defaultSetByteArray#, defaultSetOffAddr#

import Data.Hashable (Hashable,hashWithSalt)

import Data.WideWord.Compat
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@erikd I've consolidated the compat stuff to minimize noise here. Thoughts?

Copy link
Owner

Choose a reason for hiding this comment

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

I like it. That is a significant improvement.

Copy link
Owner

Choose a reason for hiding this comment

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

The only thing I would still like to address is the CPP and ViewPatterns use in things like say Data.WideWord.Word256.signum256. I am sure there is a neater way to do that (in say that Compat module).

Copy link
Owner

Choose a reason for hiding this comment

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

Maybe something like:

signum256 :: Word256 -> Word256
signum256 (Word256 a b c d) =
  if isZeroWord64 a && isZeroWord64 b && isZeroWord64 c && isZeroWord64 d
    then zeroWord256
    else oneWord256

With the isZeroWord64 being defined in the Compat module.

Copy link
Owner

Choose a reason for hiding this comment

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

Also wondering if some of the other CPPisms like this:

#if MIN_VERSION_base(4,17,0)
  Word256 (W64# (or64# a3 b3)) (W64# (or64# a2 b2))
          (W64# (or64# a1 b1)) (W64# (or64# a0 b0))
#else
  Word256 (W64# (or# a3 b3)) (W64# (or# a2 b2))
          (W64# (or# a1 b1)) (W64# (or# a0 b0))
#endif

could be lifted into the Compat module.

Copy link
Owner

Choose a reason for hiding this comment

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

This is all made more difficult because ghc-9.4 insists on being built with the Hadrian build system which complete breaks the way I used to manage installing multiple GHC versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, totally - I only did the Int128 to start since I didn't want to do a bunch of work if it wasn't to your tastes 😅 I'll port the rest over.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also tbh ghcup does work very well, you may consider adopting it? i was pretty skeptical but it is legitimately very good

Copy link
Owner

Choose a reason for hiding this comment

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

Can ghcup produce a package I can install on several machines? Can I specify where it installs stuff like /usr/lib/ghc-X.Y ?

Comment on lines 69 to 73
#if MIN_VERSION_base(4,17,0)
#define ONE (wordToWord64# 1##)
#else
#define ONE (1##)
#endif
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 is very silly! I cannot define:

one# :: Word64#
one# = wordToWord64# 1##

Copy link
Contributor Author

@parsonsmatt parsonsmatt left a comment

Choose a reason for hiding this comment

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

OK, reduced the CPP and ViewPattern use throughout

@erikd
Copy link
Owner

erikd commented Nov 29, 2022

Thanks @parsonsmatt . I have a local fix for the CI failures so I will finish this off in the next couple of days.

I will almost certainly pull all these changes into my own branch, raise a new PR and then close this one.

@erikd
Copy link
Owner

erikd commented Nov 30, 2022

Closed in favor of #72 (which is getting closer and closer).

@erikd erikd closed this Nov 30, 2022
@erikd
Copy link
Owner

erikd commented Nov 30, 2022

@parsonsmatt @spacekitteh New version on Hackage.

@parsonsmatt
Copy link
Contributor Author

Thank you 🙏🏻 🙌🏻

@erikd
Copy link
Owner

erikd commented Dec 1, 2022

Hopefully the GHC people do not break it again too soon.

@parsonsmatt
Copy link
Contributor Author

parsonsmatt commented Dec 1, 2022

i am altering the deal the public api. pray i do not alter it further

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.

3 participants