IMPALA-5192: Don't bake MemPool* into IR Tuple::CodegenMaterializeExprs() currently bakes the MemPool* provided by its caller into the generated IR. The MemPool* usually belongs to some exec nodes which owns the codegend function and it's used for allocating string buffer. With multi-threading, IR needs to be shared across multiple fragment instances so IR can no longer contain pointers not shared across fragment instances.
This change fixes the problem above by using the MemPool* argument passed to the IR function. This also cleans up UnionNode by removing the field tuple_pool_ from it and the logic for transferring buffer from tuple_pool_ to the MemPool of the row batch. Change-Id: I09d620e48032351ab9805825a4afb6536bed2302 Reviewed-on: http://gerrit.cloudera.org:8080/6657 Reviewed-by: Michael Ho <[email protected]> Tested-by: Impala Public Jenkins Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/e78d71e6 Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/e78d71e6 Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/e78d71e6 Branch: refs/heads/master Commit: e78d71e63328397ffdb59066982c0c6e83feb3d9 Parents: e2c53a8 Author: Michael Ho <[email protected]> Authored: Wed Apr 19 19:12:01 2017 -0700 Committer: Impala Public Jenkins <[email protected]> Committed: Wed Apr 26 20:46:02 2017 +0000 ---------------------------------------------------------------------- be/src/codegen/codegen-anyval.cc | 36 ++++++++------ be/src/codegen/codegen-anyval.h | 23 +++++---- be/src/codegen/llvm-codegen.cc | 21 ++++---- be/src/codegen/llvm-codegen.h | 7 +-- be/src/exec/topn-node-ir.cc | 6 +-- be/src/exec/topn-node.cc | 4 +- be/src/exec/union-node-ir.cc | 3 +- be/src/exec/union-node.cc | 9 +--- be/src/exec/union-node.h | 7 --- be/src/runtime/tuple.cc | 92 +++++++++++++++-------------------- be/src/runtime/tuple.h | 20 ++++---- 11 files changed, 107 insertions(+), 121 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/e78d71e6/be/src/codegen/codegen-anyval.cc ---------------------------------------------------------------------- diff --git a/be/src/codegen/codegen-anyval.cc b/be/src/codegen/codegen-anyval.cc index a778812..8000dfd 100644 --- a/be/src/codegen/codegen-anyval.cc +++ b/be/src/codegen/codegen-anyval.cc @@ -511,7 +511,7 @@ void CodegenAnyVal::SetFromRawValue(Value* raw_val) { } } -Value* CodegenAnyVal::ToNativeValue(MemPool* pool) { +Value* CodegenAnyVal::ToNativeValue(Value* pool_val) { Type* raw_type = codegen_->GetType(type_); Value* raw_val = Constant::getNullValue(raw_type); switch (type_.type) { @@ -520,12 +520,13 @@ Value* CodegenAnyVal::ToNativeValue(MemPool* pool) { // Convert StringVal to StringValue Value* len = GetLen(); raw_val = builder_->CreateInsertValue(raw_val, len, 1); - if (pool == NULL) { + if (pool_val == nullptr) { // Set raw_val.ptr from this->ptr raw_val = builder_->CreateInsertValue(raw_val, GetPtr(), 0); } else { - // Allocate raw_val.ptr from 'pool' and copy this->ptr - Value* new_ptr = codegen_->CodegenAllocate(builder_, pool, len, "new_ptr"); + // Allocate raw_val.ptr from 'pool_val' and copy this->ptr + Value* new_ptr = + codegen_->CodegenMemPoolAllocate(builder_, pool_val, len, "new_ptr"); codegen_->CodegenMemcpy(builder_, new_ptr, GetPtr(), len); raw_val = builder_->CreateInsertValue(raw_val, new_ptr, 0); } @@ -560,9 +561,9 @@ Value* CodegenAnyVal::ToNativeValue(MemPool* pool) { return raw_val; } -Value* CodegenAnyVal::ToNativePtr(Value* native_ptr, MemPool* pool) { - Value* v = ToNativeValue(pool); - if (native_ptr == NULL) { +Value* CodegenAnyVal::ToNativePtr(Value* native_ptr, Value* pool_val) { + Value* v = ToNativeValue(pool_val); + if (native_ptr == nullptr) { native_ptr = codegen_->CreateEntryBlockAlloca(*builder_, v->getType()); } builder_->CreateStore(v, native_ptr); @@ -589,15 +590,17 @@ Value* CodegenAnyVal::ToNativePtr(Value* native_ptr, MemPool* pool) { // // end_write: ; preds = %null, %non_null // ; [insert point ends here] -void CodegenAnyVal::WriteToSlot(const SlotDescriptor& slot_desc, Value* tuple, - MemPool* pool, BasicBlock* insert_before) { - DCHECK(tuple->getType()->isPointerTy()); - DCHECK(tuple->getType()->getPointerElementType()->isStructTy()); +void CodegenAnyVal::WriteToSlot(const SlotDescriptor& slot_desc, Value* tuple_val, + Value* pool_val, BasicBlock* insert_before) { + DCHECK(tuple_val->getType()->isPointerTy()); + DCHECK(tuple_val->getType()->getPointerElementType()->isStructTy()); LLVMContext& context = codegen_->context(); Function* fn = builder_->GetInsertBlock()->getParent(); // Create new block that will come after conditional blocks if necessary - if (insert_before == NULL) insert_before = BasicBlock::Create(context, "end_write", fn); + if (insert_before == nullptr) { + insert_before = BasicBlock::Create(context, "end_write", fn); + } // Create new basic blocks and br instruction BasicBlock* non_null_block = BasicBlock::Create(context, "non_null", fn, insert_before); @@ -606,14 +609,15 @@ void CodegenAnyVal::WriteToSlot(const SlotDescriptor& slot_desc, Value* tuple, // Non-null block: write slot builder_->SetInsertPoint(non_null_block); - Value* slot = builder_->CreateStructGEP(NULL, tuple, slot_desc.llvm_field_idx(), - "slot"); - ToNativePtr(slot, pool); + Value* slot = + builder_->CreateStructGEP(nullptr, tuple_val, slot_desc.llvm_field_idx(), "slot"); + ToNativePtr(slot, pool_val); builder_->CreateBr(insert_before); // Null block: set null bit builder_->SetInsertPoint(null_block); - slot_desc.CodegenSetNullIndicator(codegen_, builder_, tuple, codegen_->true_value()); + slot_desc.CodegenSetNullIndicator( + codegen_, builder_, tuple_val, codegen_->true_value()); builder_->CreateBr(insert_before); // Leave builder_ after conditional blocks http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/e78d71e6/be/src/codegen/codegen-anyval.h ---------------------------------------------------------------------- diff --git a/be/src/codegen/codegen-anyval.h b/be/src/codegen/codegen-anyval.h index 13494ac..696f542 100644 --- a/be/src/codegen/codegen-anyval.h +++ b/be/src/codegen/codegen-anyval.h @@ -82,7 +82,7 @@ class CodegenAnyVal { /// 'name' optionally specifies the name of the returned value. static llvm::Value* CreateCall(LlvmCodeGen* cg, LlvmBuilder* builder, llvm::Function* fn, llvm::ArrayRef<llvm::Value*> args, const char* name = "", - llvm::Value* result_ptr = NULL); + llvm::Value* result_ptr = nullptr); /// Same as above but wraps the result in a CodegenAnyVal. static CodegenAnyVal CreateCallWrapped(LlvmCodeGen* cg, LlvmBuilder* builder, @@ -135,7 +135,7 @@ class CodegenAnyVal { // /// If 'name' is specified, it will be used when generated instructions that set value_. CodegenAnyVal(LlvmCodeGen* codegen, LlvmBuilder* builder, const ColumnType& type, - llvm::Value* value = NULL, const char* name = ""); + llvm::Value* value = nullptr, const char* name = ""); /// Returns the current type-lowered value. llvm::Value* GetLoweredValue() const { return value_; } @@ -197,16 +197,19 @@ class CodegenAnyVal { /// Converts this *Val's value to a native type, StringValue, TimestampValue, etc. /// This should only be used if this *Val is not null. /// - /// If 'pool' is non-NULL, var-len data will be copied into 'pool'. - llvm::Value* ToNativeValue(MemPool* pool = NULL); + /// If 'pool_val' is non-NULL, var-len data will be copied into 'pool_val'. + /// 'pool_val' has to be of type MemPool*. + llvm::Value* ToNativeValue(llvm::Value* pool_val = nullptr); /// Sets 'native_ptr' to this *Val's value. If non-NULL, 'native_ptr' should be a /// pointer to a native type, StringValue, TimestampValue, etc. If NULL, a pointer is /// alloca'd. In either case the pointer is returned. This should only be used if this /// *Val is not null. /// - /// If 'pool' is non-NULL, var-len data will be copied into 'pool'. - llvm::Value* ToNativePtr(llvm::Value* native_ptr = NULL, MemPool* pool = NULL); + /// If 'pool_val' is non-NULL, var-len data will be copied into 'pool_val'. + /// 'pool_val' has to be of type MemPool*. + llvm::Value* ToNativePtr( + llvm::Value* native_ptr = nullptr, llvm::Value* pool_val = nullptr); /// Writes this *Val's value to the appropriate slot in 'tuple' if non-null, or sets the /// appropriate null bit if null. This assumes null bits are initialized to 0. Analogous @@ -218,9 +221,10 @@ class CodegenAnyVal { /// 'insert_before' if specified, or a new basic block created at the end of the /// function if 'insert_before' is NULL. /// - /// If 'pool' is non-NULL, var-len data will be copied into 'pool'. + /// If 'pool_val' is non-NULL, var-len data will be copied into 'pool_val'. + /// 'pool_val' has to be of type MemPool*. void WriteToSlot(const SlotDescriptor& slot_desc, llvm::Value* tuple, - MemPool* pool = NULL, llvm::BasicBlock* insert_before = NULL); + llvm::Value* pool_val, llvm::BasicBlock* insert_before = nullptr); /// Returns the i1 result of this == other. this and other must be non-null. llvm::Value* Eq(CodegenAnyVal* other); @@ -252,7 +256,8 @@ class CodegenAnyVal { /// Ctor for created an uninitialized CodegenAnYVal that can be assigned to later. CodegenAnyVal() - : type_(INVALID_TYPE), value_(NULL), name_(NULL), codegen_(NULL), builder_(NULL) {} + : type_(INVALID_TYPE), value_(nullptr), name_(nullptr), + codegen_(nullptr), builder_(nullptr) {} private: ColumnType type_; http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/e78d71e6/be/src/codegen/llvm-codegen.cc ---------------------------------------------------------------------- diff --git a/be/src/codegen/llvm-codegen.cc b/be/src/codegen/llvm-codegen.cc index 4ad5c93..cdc0c53 100644 --- a/be/src/codegen/llvm-codegen.cc +++ b/be/src/codegen/llvm-codegen.cc @@ -1381,20 +1381,19 @@ void LlvmCodeGen::CodegenClearNullBits(LlvmBuilder* builder, Value* tuple_ptr, CodegenMemset(builder, null_bytes_ptr, 0, tuple_desc.num_null_bytes()); } -Value* LlvmCodeGen::CodegenAllocate(LlvmBuilder* builder, MemPool* pool, Value* size, - const char* name) { - DCHECK(pool != NULL); - DCHECK(size->getType()->isIntegerTy()); - DCHECK_LE(size->getType()->getIntegerBitWidth(), 64); - // Extend 'size' to i64 if necessary - if (size->getType()->getIntegerBitWidth() < 64) { - size = builder->CreateSExt(size, bigint_type()); +Value* LlvmCodeGen::CodegenMemPoolAllocate(LlvmBuilder* builder, Value* pool_val, + Value* size_val, const char* name) { + DCHECK(pool_val != nullptr); + DCHECK(size_val->getType()->isIntegerTy()); + DCHECK_LE(size_val->getType()->getIntegerBitWidth(), 64); + DCHECK_EQ(pool_val->getType(), GetPtrType(MemPool::LLVM_CLASS_NAME)); + // Extend 'size_val' to i64 if necessary + if (size_val->getType()->getIntegerBitWidth() < 64) { + size_val = builder->CreateSExt(size_val, bigint_type()); } Function* allocate_fn = GetFunction(IRFunction::MEMPOOL_ALLOCATE, false); - PointerType* pool_type = GetPtrType(MemPool::LLVM_CLASS_NAME); - Value* pool_val = CastPtrToLlvmPtr(pool_type, pool); Value* alignment = GetIntConstant(TYPE_INT, MemPool::DEFAULT_ALIGNMENT); - Value* fn_args[] = {pool_val, size, alignment}; + Value* fn_args[] = {pool_val, size_val, alignment}; return builder->CreateCall(allocate_fn, fn_args, name); } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/e78d71e6/be/src/codegen/llvm-codegen.h ---------------------------------------------------------------------- diff --git a/be/src/codegen/llvm-codegen.h b/be/src/codegen/llvm-codegen.h index 7259081..5c26c14 100644 --- a/be/src/codegen/llvm-codegen.h +++ b/be/src/codegen/llvm-codegen.h @@ -495,9 +495,10 @@ class LlvmCodeGen { void CodegenClearNullBits(LlvmBuilder* builder, llvm::Value* tuple_ptr, const TupleDescriptor& tuple_desc); - /// Codegen to call pool->Allocate(size). - llvm::Value* CodegenAllocate(LlvmBuilder* builder, MemPool* pool, llvm::Value* size, - const char* name = ""); + /// Codegen to call pool_val->Allocate(size_val). + /// 'pool_val' has to be of type MemPool*. + llvm::Value* CodegenMemPoolAllocate(LlvmBuilder* builder, llvm::Value* pool_val, + llvm::Value* size_val, const char* name = ""); /// Codegens IR to load array[idx] and returns the loaded value. 'array' should be a /// C-style array (e.g. i32*) or an IR array (e.g. [10 x i32]). This function does not http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/e78d71e6/be/src/exec/topn-node-ir.cc ---------------------------------------------------------------------- diff --git a/be/src/exec/topn-node-ir.cc b/be/src/exec/topn-node-ir.cc index 87f671f..f0eaeaa 100644 --- a/be/src/exec/topn-node-ir.cc +++ b/be/src/exec/topn-node-ir.cc @@ -27,7 +27,7 @@ void TopNNode::InsertBatch(RowBatch* batch) { // Insert if either not at the limit or it's a new TopN tuple_row void TopNNode::InsertTupleRow(TupleRow* input_row) { - Tuple* insert_tuple = NULL; + Tuple* insert_tuple = nullptr; if (priority_queue_->size() < limit_ + offset_) { insert_tuple = reinterpret_cast<Tuple*>( @@ -38,7 +38,7 @@ void TopNNode::InsertTupleRow(TupleRow* input_row) { DCHECK(!priority_queue_->empty()); Tuple* top_tuple = priority_queue_->top(); tmp_tuple_->MaterializeExprs<false, true>(input_row, *materialized_tuple_desc_, - sort_exec_exprs_.sort_tuple_slot_expr_ctxs(), NULL); + sort_exec_exprs_.sort_tuple_slot_expr_ctxs(), nullptr); if (tuple_row_less_than_->Less(tmp_tuple_, top_tuple)) { // TODO: DeepCopy() will allocate new buffers for the string data. This needs // to be fixed to use a freelist @@ -48,5 +48,5 @@ void TopNNode::InsertTupleRow(TupleRow* input_row) { } } - if (insert_tuple != NULL) priority_queue_->push(insert_tuple); + if (insert_tuple != nullptr) priority_queue_->push(insert_tuple); } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/e78d71e6/be/src/exec/topn-node.cc ---------------------------------------------------------------------- diff --git a/be/src/exec/topn-node.cc b/be/src/exec/topn-node.cc index b79191f..54bfe8f 100644 --- a/be/src/exec/topn-node.cc +++ b/be/src/exec/topn-node.cc @@ -105,12 +105,12 @@ void TopNNode::Codegen(RuntimeState* state) { codegen_status = Tuple::CodegenMaterializeExprs(codegen, false, *materialized_tuple_desc_, sort_exec_exprs_.sort_tuple_slot_expr_ctxs(), - tuple_pool_.get(), &materialize_exprs_tuple_pool_fn); + true, &materialize_exprs_tuple_pool_fn); if (codegen_status.ok()) { codegen_status = Tuple::CodegenMaterializeExprs(codegen, false, *materialized_tuple_desc_, sort_exec_exprs_.sort_tuple_slot_expr_ctxs(), - NULL, &materialize_exprs_no_pool_fn); + false, &materialize_exprs_no_pool_fn); if (codegen_status.ok()) { int replaced = codegen->ReplaceCallSites(insert_batch_fn, http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/e78d71e6/be/src/exec/union-node-ir.cc ---------------------------------------------------------------------- diff --git a/be/src/exec/union-node-ir.cc b/be/src/exec/union-node-ir.cc index 38dfc97..3d19f07 100644 --- a/be/src/exec/union-node-ir.cc +++ b/be/src/exec/union-node-ir.cc @@ -25,7 +25,8 @@ void IR_ALWAYS_INLINE UnionNode::MaterializeExprs(const std::vector<ExprContext* DCHECK(!dst_batch->AtCapacity()); Tuple* dst_tuple = reinterpret_cast<Tuple*>(tuple_buf); TupleRow* dst_row = dst_batch->GetRow(dst_batch->AddRow()); - dst_tuple->MaterializeExprs<false, false>(row, *tuple_desc_, exprs, tuple_pool_.get()); + dst_tuple->MaterializeExprs<false, false>(row, *tuple_desc_, exprs, + dst_batch->tuple_data_pool()); dst_row->SetTuple(0, dst_tuple); dst_batch->CommitLastRow(); } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/e78d71e6/be/src/exec/union-node.cc ---------------------------------------------------------------------- diff --git a/be/src/exec/union-node.cc b/be/src/exec/union-node.cc index 0da1760..cd16bea 100644 --- a/be/src/exec/union-node.cc +++ b/be/src/exec/union-node.cc @@ -37,7 +37,6 @@ UnionNode::UnionNode(ObjectPool* pool, const TPlanNode& tnode, tuple_id_(tnode.union_node.tuple_id), tuple_desc_(nullptr), first_materialized_child_idx_(tnode.union_node.first_materialized_child_idx), - tuple_pool_(nullptr), child_idx_(0), child_batch_(nullptr), child_row_idx_(0), @@ -71,7 +70,6 @@ Status UnionNode::Prepare(RuntimeState* state) { RETURN_IF_ERROR(ExecNode::Prepare(state)); tuple_desc_ = state->desc_tbl().GetTupleDescriptor(tuple_id_); DCHECK(tuple_desc_ != nullptr); - tuple_pool_.reset(new MemPool(mem_tracker())); codegend_union_materialize_batch_fns_.resize(child_expr_lists_.size()); // Prepare const expr lists. @@ -105,7 +103,7 @@ void UnionNode::Codegen(RuntimeState* state) { llvm::Function* tuple_materialize_exprs_fn; codegen_status = Tuple::CodegenMaterializeExprs(codegen, false, *tuple_desc_, - child_expr_lists_[i], tuple_pool_.get(), &tuple_materialize_exprs_fn); + child_expr_lists_[i], true, &tuple_materialize_exprs_fn); if (!codegen_status.ok()) { // Codegen may fail in some corner cases (e.g. we don't handle TYPE_CHAR). If this // happens, abort codegen for this and the remaining children. @@ -272,8 +270,6 @@ Status UnionNode::GetNext(RuntimeState* state, RowBatch* row_batch, bool* eos) { RETURN_IF_ERROR(ExecDebugAction(TExecNodePhase::GETNEXT, state)); RETURN_IF_CANCELLED(state); RETURN_IF_ERROR(QueryMaintenance(state)); - // The tuple pool should be empty between GetNext() calls. - DCHECK_EQ(tuple_pool_.get()->GetTotalChunkSizes(), 0); if (to_close_child_idx_ != -1) { // The previous child needs to be closed if passthrough was enabled for it. In the non @@ -309,8 +305,6 @@ Status UnionNode::GetNext(RuntimeState* state, RowBatch* row_batch, bool* eos) { *eos = ReachedLimit() || (!HasMorePassthrough() && !HasMoreMaterialized() && !HasMoreConst(state)); - // Attach the memory in the tuple pool (if any) to the row batch. - row_batch->tuple_data_pool()->AcquireData(tuple_pool_.get(), false); COUNTER_SET(rows_returned_counter_, num_rows_returned_); return Status::OK(); } @@ -336,6 +330,5 @@ void UnionNode::Close(RuntimeState* state) { for (const vector<ExprContext*>& exprs : child_expr_lists_) { Expr::Close(exprs, state); } - if (tuple_pool_.get() != nullptr) tuple_pool_->FreeAll(); ExecNode::Close(state); } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/e78d71e6/be/src/exec/union-node.h ---------------------------------------------------------------------- diff --git a/be/src/exec/union-node.h b/be/src/exec/union-node.h index b1715f9..311ae17 100644 --- a/be/src/exec/union-node.h +++ b/be/src/exec/union-node.h @@ -65,13 +65,6 @@ class UnionNode : public ExecNode { /// materialized. const int first_materialized_child_idx_; - /// Used by MaterializeExprs() to materialize var-len slots. The ownership of the memory - /// in this pool should be transferred to the row batch at the end of each GetNext() - /// call. The memory can't be attached to the row batch in MaterializeExprs() because - /// the pointer to the mem pool is hard coded in the codegen'ed MaterializeExprs(). - /// TODO (IMPALA-5192): Remove this when no longer necessary in the future. - boost::scoped_ptr<MemPool> tuple_pool_; - /// Const exprs materialized by this node. These exprs don't refer to any children. /// Only materialized by the first fragment instance to avoid duplication. std::vector<std::vector<ExprContext*>> const_expr_lists_; http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/e78d71e6/be/src/runtime/tuple.cc ---------------------------------------------------------------------- diff --git a/be/src/runtime/tuple.cc b/be/src/runtime/tuple.cc index dc83922..67a238d 100644 --- a/be/src/runtime/tuple.cc +++ b/be/src/runtime/tuple.cc @@ -236,7 +236,7 @@ void Tuple::MaterializeExprs( // writes. If 'pool' is non-NULL, string data is copied into it. Note that the generated // function ignores its 'pool' arg; instead we hardcode the pointer in the IR. // -// Example IR for materializing an int column and a string column with non-NULL 'pool': +// Example IR for materializing a string column with non-NULL 'pool': // // ; Function Attrs: alwaysinline // define void @MaterializeExprs(%"class.impala::Tuple"* %opaque_tuple, @@ -244,72 +244,59 @@ void Tuple::MaterializeExprs( // %"class.impala::ExprContext"** %materialize_expr_ctxs, // %"class.impala::MemPool"* %pool, // %"struct.impala::StringValue"** %non_null_string_values, -// i32* %total_string_lengths) #20 { +// i32* %total_string_lengths, i32* %num_non_null_string_values) #34 { // entry: // %tuple = bitcast %"class.impala::Tuple"* %opaque_tuple to -// { i8, i32, %"struct.impala::StringValue" }* -// %0 = bitcast { i8, i32, %"struct.impala::StringValue" }* %tuple to i8* -// call void @llvm.memset.p0i8.i64(i8* %0, i8 0, i64 1, i32 0, i1 false) -// %1 = getelementptr %"class.impala::ExprContext"** %materialize_expr_ctxs, i32 0 -// %expr_ctx = load %"class.impala::ExprContext"** %1 -// %src = call i64 @GetSlotRef4(%"class.impala::ExprContext"* %expr_ctx, -// %"class.impala::TupleRow"* %row) +// <{ %"struct.impala::StringValue", i8 }>* +// %int8_ptr = bitcast <{ %"struct.impala::StringValue", i8 }>* %tuple to i8* +// %null_bytes_ptr = getelementptr inbounds i8, i8* %int8_ptr, i32 16 +// call void @llvm.memset.p0i8.i64(i8* %null_bytes_ptr, i8 0, i64 1, i32 0, i1 false) +// %0 = getelementptr %"class.impala::ExprContext"*, +// %"class.impala::ExprContext"** %materialize_expr_ctxs, i32 0 +// %expr_ctx = load %"class.impala::ExprContext"*, %"class.impala::ExprContext"** %0 +// %src = call { i64, i8* } @"impala::StringFunctions::UpperWrapper"( +// %"class.impala::ExprContext"* %expr_ctx, %"class.impala::TupleRow"* %row) +// %1 = extractvalue { i64, i8* } %src, 0 // ; ----- generated by CodegenAnyVal::WriteToSlot() ---------------------------------- -// %is_null = trunc i64 %src to i1 +// %is_null = trunc i64 %1 to i1 // br i1 %is_null, label %null, label %non_null // // non_null: ; preds = %entry -// %slot = getelementptr inbounds { i8, i32, %"struct.impala::StringValue" }* %tuple, -// i32 0, i32 1 -// %2 = ashr i64 %src, 32 -// %3 = trunc i64 %2 to i32 -// store i32 %3, i32* %slot +// %slot = getelementptr inbounds <{ %"struct.impala::StringValue", i8 }>, +// <{ %"struct.impala::StringValue", i8 }>* %tuple, i32 0, i32 0 +// %2 = extractvalue { i64, i8* } %src, 0 +// %3 = ashr i64 %2, 32 +// %4 = trunc i64 %3 to i32 +// %5 = insertvalue %"struct.impala::StringValue" zeroinitializer, i32 %4, 1 +// %6 = sext i32 %4 to i64 +// %new_ptr = call i8* @_ZN6impala7MemPool8AllocateILb0EEEPhli( +// %"class.impala::MemPool"* %pool, i64 %6, i32 8) +// %src1 = extractvalue { i64, i8* } %src, 1 +// call void @llvm.memcpy.p0i8.p0i8.i32( +// i8* %new_ptr, i8* %src1, i32 %4, i32 0, i1 false) +// %7 = insertvalue %"struct.impala::StringValue" %5, i8* %new_ptr, 0 +// store %"struct.impala::StringValue" %7, %"struct.impala::StringValue"* %slot // br label %end_write // // null: ; preds = %entry -// call void @SetNull6({ i8, i32, %"struct.impala::StringValue" }* %tuple) +// %8 = bitcast <{ %"struct.impala::StringValue", i8 }>* %tuple to i8* +// %null_byte_ptr = getelementptr inbounds i8, i8* %8, i32 16 +// %null_byte = load i8, i8* %null_byte_ptr +// %null_bit_set = or i8 %null_byte, 1 +// store i8 %null_bit_set, i8* %null_byte_ptr // br label %end_write // // end_write: ; preds = %null, %non_null // ; ----- end CodegenAnyVal::WriteToSlot() ------------------------------------------- -// %4 = getelementptr %"class.impala::ExprContext"** %materialize_expr_ctxs, i32 1 -// %expr_ctx1 = load %"class.impala::ExprContext"** %4 -// %src2 = call { i64, i8* } @GetSlotRef5(%"class.impala::ExprContext"* %expr_ctx1, -// %"class.impala::TupleRow"* %row) -// ; ----- generated by CodegenAnyVal::WriteToSlot() ---------------------------------- -// %5 = extractvalue { i64, i8* } %src2, 0 -// %is_null5 = trunc i64 %5 to i1 -// br i1 %is_null5, label %null4, label %non_null3 -// -// non_null3: ; preds = %end_write -// %slot7 = getelementptr inbounds { i8, i32, %"struct.impala::StringValue" }* %tuple, -// i32 0, i32 2 -// %6 = extractvalue { i64, i8* } %src2, 0 -// %7 = ashr i64 %6, 32 -// %8 = trunc i64 %7 to i32 -// %9 = insertvalue %"struct.impala::StringValue" zeroinitializer, i32 %8, 1 -// %new_ptr = call i8* @_ZN6impala7MemPool8AllocateILb0EEEPhi( -// %"class.impala::MemPool"* inttoptr (i64 159661008 to %"class.impala::MemPool"*), -// i32 %8) -// %src8 = extractvalue { i64, i8* } %src2, 1 -// call void @llvm.memcpy.p0i8.p0i8.i32(i8* %new_ptr, i8* %src8, i32 %8, i32 0, -// i1 false) -// %10 = insertvalue %"struct.impala::StringValue" %9, i8* %new_ptr, 0 -// store %"struct.impala::StringValue" %10, %"struct.impala::StringValue"* %slot7 -// br label %end_write6 -// -// null4: ; preds = %end_write -// call void @SetNull7({ i8, i32, %"struct.impala::StringValue" }* %tuple) -// br label %end_write6 -// -// end_write6: ; preds = %null4, %non_null3 -// ; ----- end CodegenAnyVal::WriteToSlot() ------------------------------------------- // ret void // } Status Tuple::CodegenMaterializeExprs(LlvmCodeGen* codegen, bool collect_string_vals, const TupleDescriptor& desc, const vector<ExprContext*>& materialize_expr_ctxs, - MemPool* pool, Function** fn) { - DCHECK(!collect_string_vals) << "CodegenMaterializeExprs: collect_string_vals NYI"; + bool use_mem_pool, Function** fn) { + // Only support 'collect_string_vals' == false for now. + if (collect_string_vals) { + return Status("CodegenMaterializeExprs() collect_string_vals == true NYI"); + } SCOPED_TIMER(codegen->codegen_timer()); LLVMContext& context = codegen->context(); @@ -356,7 +343,8 @@ Status Tuple::CodegenMaterializeExprs(LlvmCodeGen* codegen, bool collect_string_ Value* row_arg = args[1]; // Value* desc_arg = args[2]; // unused Value* expr_ctxs_arg = args[3]; - // Value* pool_arg = args[4]; // unused + Value* pool_arg = args[4]; + // The followings arguments are unused as 'collect_string_vals' is false. // Value* non_null_string_values_arg = args[5]; // unused // Value* total_string_lengths_arg = args[6]; // unused // Value* num_non_null_string_values_arg = args[7]; // unused @@ -386,7 +374,7 @@ Status Tuple::CodegenMaterializeExprs(LlvmCodeGen* codegen, bool collect_string_ materialize_expr_fns[i], expr_args, "src"); // Write expr result 'src' to slot - src.WriteToSlot(*slot_desc, tuple, pool); + src.WriteToSlot(*slot_desc, tuple, use_mem_pool ? pool_arg : nullptr); } builder.CreateRetVoid(); // TODO: if pool != NULL, OptimizeFunctionWithExprs() is inlining the Allocate() http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/e78d71e6/be/src/runtime/tuple.h ---------------------------------------------------------------------- diff --git a/be/src/runtime/tuple.h b/be/src/runtime/tuple.h index 82efc24..cf278ae 100644 --- a/be/src/runtime/tuple.h +++ b/be/src/runtime/tuple.h @@ -118,16 +118,17 @@ class Tuple { /// /// If non-NULL, 'pool' is used to allocate var-length data, otherwise var-length data /// isn't copied. (Memory for this tuple itself must already be allocated.) 'NULL_POOL' - /// should be true if 'pool' is NULL and false otherwise. The template parameter serves + /// must be true if 'pool' is NULL and false otherwise. The template parameter serves /// only to differentiate the NULL vs. non-NULL pool cases when we replace the function /// calls during codegen; the parameter means there are two different function symbols. + /// Callers of CodegenMaterializeExprs must set 'use_mem_pool' to true to generate the + /// IR function for the case 'pool' is non-NULL and false for the NULL pool case. /// /// If 'COLLECT_STRING_VALS' is true, the materialized non-NULL string value slots and /// the total length of the string slots are returned in 'non_null_string_values' and /// 'total_string_lengths'. 'non_null_string_values' and 'total_string_lengths' must be /// non-NULL in this case. 'non_null_string_values' does not need to be empty; its /// original contents will be overwritten. - /// TODO: this function does not collect other var-len types such as collections. template <bool COLLECT_STRING_VALS, bool NULL_POOL> inline void IR_ALWAYS_INLINE MaterializeExprs(TupleRow* row, @@ -160,16 +161,17 @@ class Tuple { static const char* MATERIALIZE_EXPRS_NULL_POOL_SYMBOL; /// Generates an IR version of MaterializeExprs(), returned in 'fn'. Currently only - /// 'collect_string_vals' = false is implemented. + /// 'collect_string_vals' = false is implemented and some arguments passed to the IR + /// function are unused. /// - /// 'pool' may be NULL, in which case no pool-related code is generated. Otherwise - /// 'pool's address is used directly in the IR. Note that this requires generating - /// separate functions for the non-NULL and NULL cases, i.e., the 'pool' argument of the - /// generated function is ignored. There are two different MaterializeExprs symbols to - /// differentiate these cases when we replace the function calls during codegen. + /// If 'use_mem_pool' is true, any varlen data will be copied into the MemPool specified + /// in the 'pool' argument of the generated function. Otherwise, the varlen data won't + /// be copied. There are two different MaterializeExprs symbols to differentiate between + /// these cases when we replace the function calls during codegen. Please see comment + /// of MaterializeExprs() for details. static Status CodegenMaterializeExprs(LlvmCodeGen* codegen, bool collect_string_vals, const TupleDescriptor& desc, const vector<ExprContext*>& materialize_expr_ctxs, - MemPool* pool, llvm::Function** fn); + bool use_mem_pool, llvm::Function** fn); /// Turn null indicator bit on. For non-nullable slots, the mask will be 0 and /// this is a no-op (but we don't have to branch to check is slots are nullable).
