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 <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Skye Wanderman-Milne <[email protected]> Gerrit-HasComments: Yes
