niyue commented on code in PR #38116:
URL: https://github.com/apache/arrow/pull/38116#discussion_r1356013485
##########
cpp/src/gandiva/engine.cc:
##########
@@ -152,6 +153,7 @@ Status Engine::LoadFunctionIRs() {
if (!functions_loaded_) {
ARROW_RETURN_NOT_OK(LoadPreCompiledIR());
ARROW_RETURN_NOT_OK(DecimalIR::AddFunctions(this));
+ ARROW_RETURN_NOT_OK(LoadExternalPreCompiledIR());
Review Comment:
> How about adding FunctionRegistry::Add(NativeFunction func, const
std::string& bitcode_path) and using it and Engine::AddPreCompiledIR() in
LLVMGenerator::Add()?
That seems reasonable, users typically have to do both (registering the
function meta and providing function bitcode) for such purpose, so it is easier
to make them call single API to achieve it.
But there are two issues we may consider:
1) One issue is `FunctionRegistry::Add(NativeFunction func, const
std::string& bitcode_path)` this API makes function metadata to be 1-1 mapped
to the bitcode, however, for a given bitcode file, it may contain bitcodes for
multiple functions. But it provides additional information where a function's
bitcode can be found if we have such an API, which may help to avoid linking
unnecessary modules if a function is never used in the expression (see
`Performance` section in this issue description) One option may be an API like
`FunctionRegistry::Add(std::vector<NativeFunction> funcs, const std::string&
bitcode_path)`, what do you think?
2) the other issue is this PR only provides the capability to register a
pre-compiled function via LLVM IR/bitcode, but I think it is possible to use
the `AddFunction` API and some shared library mechanism to support non
pre-compiled functions as well, so it may not be mandatory to register a
function metadata with bitcode (in the future)
I will take a further look to see how this could be improved. At the same
time, @js8544 do you have any feedback on the APIs here? Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
