niyue commented on code in PR #40031:
URL: https://github.com/apache/arrow/pull/40031#discussion_r1485615504
##########
cpp/src/gandiva/engine.h:
##########
@@ -57,12 +58,11 @@ class GANDIVA_EXPORT Engine {
///
/// \param[in] config the engine configuration
/// \param[in] cached flag to mark if the module is already compiled and
cached
- /// \param[in] object_cache an optional object_cache used for building the
module
+ /// \param[in] object_cache an object_cache used for building the module
/// \return arrow::Result containing the created engine
static Result<std::unique_ptr<Engine>> Make(
const std::shared_ptr<Configuration>& config, bool cached,
- std::optional<std::reference_wrapper<GandivaObjectCache>> object_cache =
- std::nullopt);
+ llvm::ObjectCache* object_cache);
Review Comment:
There are two reasons I make this change for this constructor:
* I realize `GandivaObjectCache` is a higher level concept, to construct it,
`ExpressionCacheKey` needs to be used, and the caller needs to be aware of
concepts like `Schema`/`Configuration`/`Expression`, and `Engine` is lower
level and is not aware of these concepts at all. So I switch to use a lower
level API (`llvm::ObjectCache`) instead
* Previously this `object_cache` is optional for the engine, however, in
this PR, I tries to avoid constructing too many `TargetMachine` for better
performance, a `TargetIRAnalysis` is passed to `Engine`'s constructor (the
Engine only uses `TargetIRAnalysis` during optimizing code, but doesn't use
`TargetMachine` directly). For some unknown reason (it seems to be a bug of
LLVM for me), the `TargetIRAnalysis` obtained from a `TargetMachine` requires
the `TargetMachine` instance to be alive when running optimization
(`OptimizeModuleWithNewPassManager`), so I change this object_cache to be
mandatory (previously the only place that constructs `Engine` without
`object_cache` were several unit tests in `engine_llvm_test.cc` and
`llvm_generator_test.cc`), and the corresponding `target_machine` will be moved
into `TMOwningSimpleCompiler` and be alive:
```
jit_builder.setCompileFunctionCreator(
[&object_cache, &target_machine](llvm::orc::JITTargetMachineBuilder
JTMB)
->
llvm::Expected<std::unique_ptr<llvm::orc::IRCompileLayer::IRCompiler>> {
// after compilation, the object code will be stored into the given
object
// cache
return std::make_unique<llvm::orc::TMOwningSimpleCompiler>(
std::move(target_machine), object_cache);
});
```
--
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]
