[Impala-CR](cdh5-trunk) IMPALA-2548: Codegen Tuple::MaterializeExprs() and use in TopN node

["Michael Ho (Code Review)" <ge...@cloudera.org>]

Michael Ho has posted comments on this change.

Change subject: IMPALA-2548: Codegen Tuple::MaterializeExprs() and use in TopN node
......................................................................


Patch Set 6:

(15 comments)

Sorry for the delay. Some more comments. Hope they make sense as I am still learning about codegen.

http://gerrit.cloudera.org:8080/#/c/1901/6/be/src/codegen/codegen-anyval.cc
File be/src/codegen/codegen-anyval.cc:

Line 152:  CodegenAnyVal(cg, builder, type, v, name);
If result_ptr != NULL, CreateCall() will create NULL. So, what's the point of creating a CodegenAnyVal object with v == NULL ?


Line 507: Value* CodegenAnyVal::ToNativeValue(MemPool* pool) 
Generated IR in the header comment will help.


Line 565: CodegenAnyVal::WriteToSlot
May help readers if you post the generated IR in the header comment.


http://gerrit.cloudera.org:8080/#/c/1901/6/be/src/codegen/codegen-anyval.h
File be/src/codegen/codegen-anyval.h:

Line 214: insert_before
nit: 'insert_before'


Line 217: WriteToSlot
Can you please consider calling it CodegenWriteToSlot() ?


http://gerrit.cloudera.org:8080/#/c/1901/6/be/src/codegen/llvm-codegen.h
File be/src/codegen/llvm-codegen.h:

Line 432:  CodegenMemcpy
Does this need to be exposed ? Does this even need to be a separate function ?


http://gerrit.cloudera.org:8080/#/c/1901/6/be/src/exec/topn-node.cc
File be/src/exec/topn-node.cc:

Line 84:   insert_batch_fn = codegen->ReplaceCallSites(insert_batch_fn, false,
There are two calls to MaterializeExprs(), one with pool != NULL and the other one with pool == NULL. Is there any guarantee that we are replacing the right one ?

An alternative would be to define two different signatures of MaterializeExprs() with/without 'MemPool* pool'. In this way, we don't need the changes to ReplaceCallSites() either.


http://gerrit.cloudera.org:8080/#/c/1901/6/be/src/runtime/tuple.cc
File be/src/runtime/tuple.cc:

Line 218: non_null_string_values++)
I thought our coding style prefers pre-increment if possible.


Line 249: trunc i64 %src to i1
Is trunc the right instruction to use here ? I believe this is generated by CodegenAnyVal::GetIsNull().

%Y = trunc i32 123 to i1; yields i1:true
%Z = trunc i32 122 to i1; yields i1:false


Line 252: // non_null:                                         ; preds = %entry
Part of this IR is generated by functions in CodegenAnyVal, right ? Can you please annotate this header comment so it's easier to follow ?


Line 297: Status Tuple::CodegenMaterializeExprs(RuntimeState* state, bool collect_string_vals,
Just curious if you have considered refactoring Tuple::MaterializeExprs() so we can generate its IR and replaced only its loop with an inline function and replace the inline function with codegen'ed version ?


Line 327: GetPtrType(ExprContext::LLVM_CLASS_NAME)->getPointerTo();
Is there a reason why we mix the use getPointerTo() and GetPtrType() ? According to the documentation, getPointerTo() is the same as This is equivalent to PointerType::get(Foo, AddrSpace).


Line 348:   // Value* pool_arg = args[4]; // unused, we hardcode 'pool's ptr instead
        :   // Value* non_null_string_values_arg = args[5]; // unused
        :   // Value* total_string_arg = args[6]; // unused
Can these unused arguments just be a single line of comment ?


Line 364:        
nit: wrong indentation ?


http://gerrit.cloudera.org:8080/#/c/1901/6/be/src/runtime/tuple.h
File be/src/runtime/tuple.h:

Line 122: std::vector<StringValue*>* non_null_string_values =
The conversion of std::vector<StringValue*> to StringValue** seems dangerous as it assumes the layout of vector entry. Why don't we also convert this function to not use vector ?


-- 
To view, visit http://gerrit.cloudera.org:8080/1901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib422a8d50303c21c6a228675157bf867e8619444
Gerrit-PatchSet: 6
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Skye Wanderman-Milne <sk...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Skye Wanderman-Milne <sk...@cloudera.com>
Gerrit-HasComments: Yes