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 factory method:
* 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
* 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 have a
default value if nullptr is provided (a cache that does not cache at all), and
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]
