GH-50371: [C++][Gandiva] Fold common subexpressions before code generation#50372
GH-50371: [C++][Gandiva] Fold common subexpressions before code generation#50372likun666661 wants to merge 2 commits into
Conversation
|
|
|
@dmitry-chirkov-dremio @lriggs @akravchukdremio @xxlaykxx Could you review this? |
There was a problem hiding this comment.
Pull request overview
This PR introduces a conservative expression-layer common subexpression folding (CSE) pass in Gandiva before LLVM code generation, aiming to reduce redundant decomposition/codegen for repeated safe/pure subtrees while explicitly avoiding reuse in cases where Gandiva’s validity/bitmap side effects could affect correctness. It also adds the ability to dump unoptimized IR (pre-optimizer) to support targeted IR-based tests.
Changes:
- Add
expr_csefolding pass and apply it in bothProjector::MakeandFilter::Makebefore validation, cache-keying, and code generation. - Add decomposition-time reuse for safe nodes (fields, literals, and pure/native functions that don’t need context/holders and can’t error / use null-internal).
- Add unoptimized IR dumping plumbing and new IR-focused regression tests.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| cpp/src/gandiva/tests/projector_test.cc | Adds IR-string-based tests validating CSE behavior (optimized vs unoptimized IR) for repeated arithmetic, generated if, generated boolean, and nested between-like patterns. |
| cpp/src/gandiva/projector.h | Exposes DumpUnoptimizedIR() on Projector. |
| cpp/src/gandiva/projector.cc | Folds common subexpressions before building cache keys, validating, and building the LLVM module; wires DumpUnoptimizedIR(). |
| cpp/src/gandiva/llvm_generator.h | Exposes unoptimized_ir() passthrough. |
| cpp/src/gandiva/filter.cc | Folds common subexpressions before cache-keying, validation, and building for filters. |
| cpp/src/gandiva/expr_decomposer.h / .cc | Adds decomposition caching for reuse-safe nodes to avoid repeated decomposition work. |
| cpp/src/gandiva/expr_cse.h / .cc | Introduces the conservative expression-tree folding pass and structural interning for safe nodes. |
| cpp/src/gandiva/engine.h / .cc | Captures and exposes module IR before the optimizer pipeline when dump_ir is enabled. |
| cpp/src/gandiva/CMakeLists.txt | Adds expr_cse.cc to the build. |
|
|
||
| const std::string& Projector::DumpIR() { return llvm_generator_->ir(); } | ||
|
|
||
| const std::string& Projector::DumpUnoptimizedIR() { |
There was a problem hiding this comment.
This new API looks unsafe on cache hits. Projector::Make() now reuses cached object code for more expressions after folding, but on the cached path Engine::FinalizeModule() never populates unoptimized_module_ir_, so DumpUnoptimizedIR() will DCHECK even when dump_ir=true.
Can we either:
- make this API explicitly unavailable for cached projectors, or
- preserve/store the unoptimized IR alongside the cache entry, or
- add a guard/error path here instead of unconditionally delegating?
| auto expr = | ||
| TreeExprBuilder::MakeExpression(square, arrow::field("cse_out", arrow::int32())); | ||
|
|
||
| auto configuration = std::make_shared<Configuration>( |
There was a problem hiding this comment.
These assertions feel more tied to LLVM's final optimization/lowering choices than to the CSE pass itself. For example, expecting zero helper calls and exactly one add instruction can change across LLVM versions or target CPUs even if the folding is still correct.
Would it be safer to keep the structural checks on DumpUnoptimizedIR() and then validate behavior with runtime evaluation assertions, instead of pinning the optimized IR this tightly?
| ARROW_RETURN_IF(configuration == nullptr, | ||
| Status::Invalid("Configuration cannot be null")); | ||
|
|
||
| auto folded_condition = |
There was a problem hiding this comment.
Could we add a Filter test for this path too? The new tests here are projector-only, but filter now also depends on the folded tree for both build behavior and cache-keying.
Rationale for this change
This adds a conservative Gandiva expression-layer common subexpression folding pass before code generation. Repeated pure expression subtrees can otherwise be decomposed and lowered multiple times before LLVM sees the module.
The pass intentionally avoids cases where expression-level reuse can be unsafe, including functions that need execution context/function holders, can return errors, use result-null-internal handling, and boolean/if decomposition results with local validity bitmap side effects.
What changes are included in this PR?
expr_cseto fold safe repeated Gandiva expression subtrees before projector/filter code generation.dump_iris enabled.if, generated boolean, and nestedbetween-style patterns.Are these changes tested?
Yes.
The
ninja ... unittestrun completed with100% tests passed, 0 tests failed out of 81.AI-assisted contribution disclosure
This PR was prepared with AI assistance. I reviewed and tested the generated changes locally, including the Gandiva C++ tests and Arrow C++ pre-commit checks listed above.