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

GH-45132: [C++][Gandiva] Revert LVVM JIT upgrade #45114

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

lriggs
Copy link
Contributor

@lriggs lriggs commented Dec 27, 2024

Rationale for this change

#37848 upgraded the JIT compiler for LLVM/Gandiva code which presented linking errors with newer version of LLVM. Some Gandiva tests were disabled, and here at Dremio I am running into the same linking problem when trying to build with an updated Arrow library. LLVM devs are aware of the linking problem, which seems to be an issue with the package exports, but there is no fix yet. In the meantime I'm proposing reverting the JIT upgrade since it was a maintenance item and not strictly necessary at this time.

More discussion in apache/arrow-java#63.

What changes are included in this PR?

Reverting #37848

Are these changes tested?

Covered by existing tests.

Are there any user-facing changes?

No.

@lriggs lriggs requested a review from wjones127 as a code owner December 27, 2024 23:14
Copy link

⚠️ GitHub issue #55 has been automatically assigned in GitHub to PR creator.

Copy link

⚠️ GitHub issue #43981 has no components, please add labels for components.

@kou
Copy link
Member

kou commented Dec 28, 2024

Could you open a new issue for this?
GH-43981 doesn't exist in apache/arrow now.

Copy link

⚠️ GitHub issue #55 has been automatically assigned in GitHub to PR creator.

Copy link

⚠️ GitHub issue #43981 has no components, please add labels for components.

@kou kou changed the title GH-43981 [Gandiva] Revert LVVM JIT upgrade. GH-45132: [C++][Gandiva] Revert LVVM JIT upgrade Dec 31, 2024
Copy link

⚠️ GitHub issue #45132 has been automatically assigned in GitHub to PR creator.

@kou
Copy link
Member

kou commented Dec 31, 2024

We have AddAbsoluteSymbol() helper:

void AddAbsoluteSymbol(llvm::orc::LLJIT& lljit, const std::string& name,
void* function_ptr) {
llvm::orc::MangleAndInterner mangle(lljit.getExecutionSession(), lljit.getDataLayout());
// https://github.com/llvm/llvm-project/commit/8b1771bd9f304be39d4dcbdcccedb6d3bcd18200#diff-77984a824d9182e5c67a481740f3bc5da78d5bd4cf6e1716a083ddb30a4a4931
// LLVM 17 introduced ExecutorSymbolDef and move most of ORC APIs to ExecutorAddr
#if LLVM_VERSION_MAJOR >= 17
llvm::orc::ExecutorSymbolDef symbol(
llvm::orc::ExecutorAddr(reinterpret_cast<uint64_t>(function_ptr)),
llvm::JITSymbolFlags::Exported);
#else
llvm::JITEvaluatedSymbol symbol(reinterpret_cast<llvm::JITTargetAddress>(function_ptr),
llvm::JITSymbolFlags::Exported);
#endif
auto error = lljit.getMainJITDylib().define(
llvm::orc::absoluteSymbols({{mangle(name), symbol}}));
llvm::cantFail(std::move(error));
}

Can we register llvm_orc_registerEHFrameSectionWrapper() and needed symbols manually instead?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants