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

Refactor jitCompile #401

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

Conversation

nic-donaldson
Copy link
Contributor

This PR refactors the jitCompile function!
Please take a look and let me know what you think.

The jitCompile function is responsible for a few things:

  • compile init.ll on first call
  • take LLVM IR for xtlang and prepend it with bitcode.ll, inline.ll, and any global declarations that are necessary (presumably for linking??), then compile
  • blow up when given bad IR

In my opinion it was pretty hard to tell what was going on, especially the dance around setting up the static variables which held different things on the first, second, and third calls to the function. This refactoring mostly removes those variables (sInlineString, sBitcode) and replaces each use with the corresponding file (inline.ll, bitcode.ll, bitcode.ll as bitcode).

There's also a little bit of cleanup, some long lines were split and I indented a few things. As part of this I want to add this .clang-format file, which is a pretty standard and opinionated way of formatting C++ code these days. The formatting throughout Extempore's C++ is not very consistent, so I figure this is a good thing to add as many editors these days use the .clang-format file to decide how to format new or modified code.

It looks like this code interacts with the extempore-as-dynamic-library feature, and I have not tested that.

This fits into the LLVM upgrade by making it easier to isolate LLVM from this file.

@benswift
Copy link
Collaborator

Hey @nic-donaldson thanks very much, will have a look asap.

I guess in the LLVM timeline we're even up to v12 now - do you know if the OrcJIT has changed much from 11 -> 12?

@nic-donaldson
Copy link
Contributor Author

do you know if the OrcJIT has changed much from 11 -> 12?

I don't know! I don't see anything in the release notes, but the LLVM release notes can sometimes be a little incomplete.

@benswift
Copy link
Collaborator

benswift commented May 5, 2021

It looks like this code interacts with the extempore-as-dynamic-library feature, and I have not tested that.

Hey @digego, I don't know much about that stuff, so you're probably better placed to answer it than me. I'm happy to put together a test for the --dll native compile stuff if you can point me in the right direction of how to test it properly.

I just tried examples/core/native_dll.xtmon my (macOS Big Sur) box and it got to the clang part---the bitcode and object files were created ok, but it bombed out because it couldn't find libextempore (I'm not sure where that's supposed to be).

Any ideas on how best to test the nativecomp stuff cross-platform (and therefore check whether this patchset is safe)?

@digego
Copy link
Owner

digego commented May 5, 2021 via email

@digego
Copy link
Owner

digego commented May 5, 2021 via email

@digego
Copy link
Owner

digego commented May 6, 2021 via email

@nic-donaldson
Copy link
Contributor Author

Hey @digego I rebased this branch to include those changes. For me adding -DEXT_DYLIB=On compiles but does not link. I suspect that's probably a NixOS thing and not an extempore thing though.

I noticed a few #ifdef DYLIB lines were not changed to EXT_DYLIB in SchemeProcess.cpp. If I change all of those to EXT_DYLIB it no longer compiles unless I pull the #ifdef which includes the cmrc.hpp header out of the #ifdef _WIN32 block that it was in. I'm assuming that's how it's meant to be so I've added a commit which does that.

@benswift
Copy link
Collaborator

hey @nic-donaldson I'm sorry about the radio silence for so long on this - I'm still keen to see it land (because LLVM 3.8 is waaaay old at this point) and will maybe have some time to look at it soon. The only confounding factor at the moment is that I'm in lockdown with only my macOS laptop, so I'll be relying on GH Actions and the test suite for cross-platform testing (although we don't currently have an end-to-end test for the dylib stuff yet).

@nic-donaldson
Copy link
Contributor Author

@benswift that's good to hear! I only have a Linux machine I can use at the moment. A while I ago I used a Windows VM on AWS to sort out the LLVM 11 build situation on my branch; it worked but it wasn't fun.

@benswift
Copy link
Collaborator

Yeah, I know what you mean. We can also put the call out the community (e.g. @digego ) to test stuff on Windows.

I'll keep you posted - still dealing with lockdowns here (and teaching online) but I'll get to it as soon as I can.

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