-
Notifications
You must be signed in to change notification settings - Fork 605
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
MCJIT initial support for PowerPC64 #3200
Conversation
This adds a MCJIT initial support for PowerPC64.
engine_->freeMachineCodeForFunction(function_); | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that this change currently leaks memory when applied? You should be able to use valgrind
to check for memory leaks here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dbussink, thank you for the feedback, I will do that.
Please rebase this branch based on the current branch, instead of merging. That way we don't end up with needless "Merge pull request ...." commits all over the place. As an aside, if we're going to support mcjit we shouldn't add extra configure flags and conditional macros, it should instead be the JIT library (we're not going to run both the old and new setup in parallel). |
@yorickpeterse, OK, I understand, thanks for the comment... In this case, is it OK if I change the macro from RBX_LLVM_MCJIT_ENABLED to something like RBX_LLVM_API_VER >= 306? |
@gustavotemple Why not change it for all? Is there only a certain LLVM version that has the necessary MCJIT support? It would be great if we can switch to MCJIT for everything, but I tried in the past and it has a bunch of issues. The biggest one is that we use a custom memory allocator so we can prevent memory growth over time of long living LLVM context objects. I couldn't find a way at that point to fix that, but maybe newer LLVM versions allow for solving this problem now. |
@gustavotemple thanks for working on this. I want to move completely to the new API and stabilize with LLVM 3.5+, and I want to do this completely, not partially. I'm allocating time to work on it in the next couple weeks so I'm going to close this. |
@brixen, OK, thank you |
@dbussink, sorry for my late reply. |
This adds a MCJIT initial support for PowerPC64.