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

Switch to ORCv2 code generation #485

Open
elliottslaughter opened this issue Feb 23, 2021 · 10 comments
Open

Switch to ORCv2 code generation #485

elliottslaughter opened this issue Feb 23, 2021 · 10 comments

Comments

@elliottslaughter
Copy link
Member

elliottslaughter commented Feb 23, 2021

ORCv1 (which we use via the MCJIT wrapper) has been deprecated for quite a while. At some point it will finally be removed and we'll be forced to switch (e.g., to LLJIT).

I don't have time to do this at the moment, but if someone wants to take a look, we could switch proactively rather than waiting for our build to break before doing it.

Deprecation warning looks like:

src/tcompiler.cpp:449:14: warning: 'setUseOrcMCJITReplacement' is deprecated: ORCv1 utilities (including OrcMCJITReplacement) are deprecated. Please use ORCv2/LLJIT instead (see docs/ORCv2.rst) [-Wdeprecated-declarations]
            .setUseOrcMCJITReplacement(true);
             ^
@ErikMcClure
Copy link
Contributor

I feel obligated to warn you that ORCv2 is what I attempted to use for inNative and it required several hacks inside LLVM itself to actually load DLLs properly, and I haven't attempted to upstream those (mostly because those probably aren't the "correct" fix and I just don't want to deal with LLVM right now). While LLVM certainly likes to claim that ORCv2 is what you "should" use, it risks breaking terra on windows in ways that won't be possible to fix until LLVM fixes how it processes symbols on Windows and COFF loading. This is on LLVM 10, which is the latest version supported by terra.

@elliottslaughter
Copy link
Member Author

Is the LLVM project at least aware of these issues? It would be nice to make sure they're aware of them so hopefully they're not too eager to remove ORCv1 while they still remain.

@elliottslaughter
Copy link
Member Author

Well, it looks like we're going to finally be forced to look into this. LLVM 12 has removed setUseOrcMCJITReplacement:

src/tcompiler.cpp:393:14: error: no member named 'setUseOrcMCJITReplacement' in 'llvm::EngineBuilder'
            .setUseOrcMCJITReplacement(true);
             ^

@ErikMcClure
Copy link
Contributor

I will have to attempt to update my compiler to LLVM 12 and see if they actually fixed ORCv2 on windows, but I don't know when I'm going to have time to do this. I checked the LLVM 12 code on github and there haven't been any significant changes to the areas where we implemented our workarounds, so I suspect the issues are still there. This won't affect Terra's compilation step, but it will potentially make it so Terra cannot link to a DLL correctly with the JIT.

@elliottslaughter
Copy link
Member Author

I made a comment in #503 (comment) about the removal of ORCv1 in LLVM 12. Apparently ORCv1 was removed, but MCJIT is still around. Which is frankly bizarre. But apparently we're getting some mileage out of going back to MCJIT, which (assuming it works) may defer the need to think about this a while longer.

@elliottslaughter
Copy link
Member Author

We went ahead with the MCJIT path for #503. It would still be a good idea to investigate ORCv2 at some point because presumably MCJIT will get removed eventually, even if it outlived ORCv1.

@elliottslaughter
Copy link
Member Author

This appears to be linked to two issues:

So there now seems to be concrete motivation for fixing this, since it is holding back some of our platforms on the newest LLVM versions.

@elliottslaughter
Copy link
Member Author

This also appears to be the issue behind the divergence between hardware and emulated test results on AArch64. (I suspect it would help PPC64le too, but I haven't had time to check there.)

@reinar
Copy link

reinar commented Apr 12, 2024

Just to revive the discussion with some context: after introduction of JITLink, which has full COFF support since LLVM 15 (pretty nice talk by one of the contributors), ORCv2 migration seems to be worth it, as Windows concerns described above should be resolved and in general COFF linking couldn't be more native.

Maybe this will help to remove a lot of general flakiness across platforms, however I just started to read the code and have little idea on the effort involved.

@elliottslaughter
Copy link
Member Author

Hi @reinar,

Thanks for bringing this to our attention. OCRv2 is generally something we're interested in. As usual we're waiting for someone who has the time to actually do it. If it falls to me, it will probably happen eventually, but "eventually" might be a while since my time here is very limited and there are several things that are still higher priority on my to-do list.

Contributions are always welcome!

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

No branches or pull requests

3 participants