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

Use object code for TH+UnboxedTuples/Sums #1382

Merged
merged 12 commits into from Feb 18, 2021
Merged

Use object code for TH+UnboxedTuples/Sums #1382

merged 12 commits into from Feb 18, 2021

Conversation

wz1000
Copy link
Collaborator

@wz1000 wz1000 commented Feb 16, 2021

  1. Modules that use TH themselves but aren't depended on by anything that uses TH don't need to be compiled
  2. Any module that needs to be compiled and uses unboxed sums/tuples needs to be compiled using object code
  3. Any module that is depended on by anything that uses object code needs to be compiled using object code.

Logic from https://gitlab.haskell.org/ghc/ghc/-/blob/963e1e9aedf0ee70d4e817640ec9845ed00ce0cf/compiler/GHC/Driver/Make.hs#L2268

@michaelpj
Copy link
Collaborator

FYI you might be interested in https://gitlab.haskell.org/ghc/ghc/-/merge_requests/4412, which will hopefully get into 9.2 and should make this work without object code. We actually use a GHC with that patch applied at work precisely so that HLS works with unboxed tuples and sums 😅

@hamishmack
Copy link

We have back ported the patch to 8.10.3 (and newer) and include it in haskell.nix. Any haskell.nix project that uses ghc 8.10 and has a shell created with shellFor { tools = { haskell-language-server = "latest"; }; } should use it correctly.

If you use nix and want to try it out while developing haskell-language-server itself, I have included a suitable .nix file in #1386.

@wz1000
Copy link
Collaborator Author

wz1000 commented Feb 16, 2021

That is good to hear, but an important question remains. Is it possible to detect the presence of this patch at compile time? We would like to merge this PR to properly support users of unpatched GHCs. However, it may have some negative impact in terms of performance, linker issues etc. for users of patched GHCs.

@wz1000
Copy link
Collaborator Author

wz1000 commented Feb 16, 2021

I suppose it might be OK if it were possible to detect it at runtime either, but a configuration option seems less than ideal

@hamishmack
Copy link

I'm not how we could detect the patch in ghc 8.10 (@angerman or @luite might have an idea). We could set a flag when building haskell-language-server if that helps. We have a good place to put it.

@hamishmack
Copy link

A cabal flag that is

@pepeiborra
Copy link
Collaborator

You could change the version number to e.g. 8.10.13, or 8.100.3

@hamishmack
Copy link

You could change the version number to e.g. 8.10.13, or 8.100.3

That might break other things that expect 8.10.3.

Perhaps haskell-language-server could check for version 9.2 (when we expect the unboxed tuple patch to lade in GHC). If it sees that version it could assume unboxed tuple support is present and that using object code is unnecessary.

Then in haskell.nix we could include a patch for haskell-language-server that would just change that threshold from 9.2 to 8.10.3 (but it might be cleaner if there was a ghc-has-unboxed-tuples-patch cabal flag it could set instead).

@wz1000
Copy link
Collaborator Author

wz1000 commented Feb 17, 2021

@michaelpj @hamishmack I've added a cabal flag called ghc-patched-unboxed-bytecode (quite a mouthful, I know). It would be great if you guys could add it to haskell.nix or whatever after adding a configuration to CI testing it. You don't need to test everything, just --pattern=TemplateHaskell should be enough.

Copy link
Collaborator

@pepeiborra pepeiborra left a comment

Choose a reason for hiding this comment

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

It's a bit bad that we are forced to do a whole project analysis in NeedsCompilation, which is used by other core rules like GetModIface. But that's already the status quo, this PR doesn't make it materially worse.

@wz1000 wz1000 added the merge me Label to trigger pull request merge label Feb 17, 2021
@wz1000
Copy link
Collaborator Author

wz1000 commented Feb 17, 2021

I don't see a way around the whole project analysis, even GHC executes this logic in the "downsweep" step which is a whole project analysis starting from the highest nodes in the module graph and proceeding down to the leaves.

@mergify mergify bot merged commit 03e833b into master Feb 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants