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

Llvm 12 #14

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Llvm 12 #14

wants to merge 8 commits into from

Conversation

jrp2014
Copy link

@jrp2014 jrp2014 commented May 8, 2021

Hi, some unsolicited changes. These mainly update the cabal file, add a few explicit export lists and explicit stock derivings.

I'll have a further look. I suspect that some of the fields in data types could be made strict to further improve performance / reduce the chance of space leaks, eg.

This works with LLVM-12 and a cabal.project file to pull in the LLVM-12 branches of llvm-hs, pure and pretty.

packages: ./llvm-hs-pretty
          ./llvm-hs/llvm-hs-pure
          ./llvm-hs/llvm-hs
          ./mcc

Many thanks for making this available.

@jmorag
Copy link
Owner

jmorag commented May 10, 2021

Thanks for this PR! Very glad you enjoyed the tutorial :) Just a couple questions:

  • CI looks broken now. I see that you added some github workflow files. Does that mean we can replace travis with github actions and turn it green again?
  • I wasn't aware that you could indicate stock deriving without turning on DerivingStrategies. I'm not necessarily opposed, but since this is supposed to be "beginner friendly" Haskell, whatever that means, it might be preferable to leave them off.
  • A bunch of the diffs look like they were done by a formatter. Which one did you use? I'm not particularly attached to any, but now that you've set up github actions, it might be nice to just use https://github.com/mrkkrp/ormolu-action to eliminate such diffs in the future.
  • Did you measure/notice a performance difference with StrictData? I'm happy to turn it on, but it would be nice to have some concrete evidence that it helps. I guess I should finish the benchmark suite 😅 if I want such things, though if you're interested in taking that on, that would also be a fantastic contribution!

@jmorag
Copy link
Owner

jmorag commented May 10, 2021

Resolves #13

@jmorag jmorag linked an issue May 10, 2021 that may be closed by this pull request
@jrp2014
Copy link
Author

jrp2014 commented May 10, 2021

Thanks for your rsponses:

  • The CI won't work at present as it will pull down llvm-hs from hackage, which is not yet at LLVM 12. I haven't spent the time customizing the workflow as the result would be fragile, but it should start to work when hackage is updated.
  • Adding the stock shuts up a compiler warning, suggesting that we should be starting to use deriving strategies. It could encourage beginners to explore the topic, if nothing else.
  • I had not intended to reformat the sources (probably just muscle reflex). I tend to use brittany or ormolu. Adding the ormolu action would be a nice touch.
  • I found it quite hard to measure performance (by running the test suit against the wall clock) as the results seem to vary wildly from run to run, suggesting that the cost of firing off a process to run a test is the dominant timing factor. In any case, the data strictness should reduce the risk of memory leaks, although ghc is pretty good at avoiding them. I did get as far as trying to add ghc-options: -threaded -rtsopts -with-rtsopts=-N to the mcc build, but that messes up the test suite runs, for reasons unknown (the tests cannot find the rts file, suggesting that the current working directory is not preserved). I have not played with other variations. You could try to run your original against this version to see what difference it made. I'll have a further think about the benchmarking. It's not hard, although it'd be good to have some bigger examples to stretch the compiler and include facilities for extra optimization passes.

If you were ever thinking of adding to this project, you could see if you could implement a repl and use the Orc JIT compiler. The API has, however, changed in llvm 12, so you may want to wait for that. Here's an example https://lukelau.me/kaleidoscope/

@jmorag
Copy link
Owner

jmorag commented May 10, 2021

Does the CI workflow try to install LLVM 12 currently? I skimmed the workflow file but I didn't find anything to indicate that it does. Could you try removing the travis CI so we can see what happens with the github actions run? Then we can integrate the ormolu action and reformat.

@jmorag
Copy link
Owner

jmorag commented May 10, 2021

Regarding the rts options failing, it's probably because the tests call clang for linking and that doesn't play well with tasty's built in test parallelization. I'll take a look later.

@jrp2014
Copy link
Author

jrp2014 commented May 10, 2021

No, the workflow actions only use the hackage versions for now. These are "out of the box" workflows. You could try them on the llvm-9 branch to validate them, I suppose.

It may be worth using typed-process rather than process for invoking clang. I've struggled with process on Windows as it's hard under windows to get return codes back to the grandparent process.

@jmorag
Copy link
Owner

jmorag commented May 10, 2021 via email

@jrp2014
Copy link
Author

jrp2014 commented May 10, 2021

No, I don't think that it does. Not sure what the standard VMs provide.

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.

test-linkedlist fails
2 participants