Dan Hecht has posted comments on this change. Change subject: Upgrade LLVM to 3.8.0 ......................................................................
Patch Set 7: (26 comments) http://gerrit.cloudera.org:8080/#/c/2486/7/be/CMakeLists.txt File be/CMakeLists.txt: Line 324: # ${LLVM_BUILTIN_LIBS} remove http://gerrit.cloudera.org:8080/#/c/2486/7/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: Line 92: if (llvm_initialized) return; do we actually call this from multiple threads? i don't see why we need these two lines. how about at least changing the second to DCHECK(!llvm_initialized)? Line 309: GetHostCPUAttrs(&cpu_attrs); do you think it would be helpful to log this? feel free to ignore. Line 530: return false; will we notice during testing if this thing returns false? Line 634: no functions are successfully codegen'd in what case would that happen? i.e. what does "successfully" mean in this context? Line 661: consider removing some blank lines Line 775: boost::lock_guard<mutex> l(jitted_functions_lock_); why do we need this lock? isn't this called only once per fragment instance and this object is per instance, right? http://gerrit.cloudera.org:8080/#/c/2486/7/be/src/codegen/llvm-codegen.h File be/src/codegen/llvm-codegen.h: Line 100: query fragment instance, right? Line 138: boost::scoped_ptr<LlvmCodeGen>* codegen); Maybe make this private. Line 287: this Not your change, but while we're here let's try to clarify a few things. 'this' is confusing, since it's not talking about this routine. Also, what's 'result_fn_ptr'? Line 289: is not valid to call GetJittedFunction() before every part of the : /// query has finished adding their IR rather than saying what, let's explain why. Line 291: those objects what objects? Line 295: /// internal in FinalizeModule() and may be removed as part of optimization. rather than talk about what happens if this routine weren't called, how about we talk about what does happen -- i.e. the input function is marked reachable and so won't be removed by dead code elimination. Line 410: std::unique_ptr<llvm::Module>* module); any reason to not make this private? Line 422: std::string module_name, std::unique_ptr<llvm::Module>* module); and this one? Line 445: skips type checking what type checking? is this referring to the decimal wrapper stuff? Line 454: he function : /// must have been registered in AddFunctionToJit(). is this actually required by GetJittedFunction() or it just happens that we'll only call this on things that were added by AddFunctionToJit()? Line 517: std::set<llvm::Function*> jitted_functions_; do we still need this? http://gerrit.cloudera.org:8080/#/c/2486/7/be/src/runtime/decimal-value.inline.h File be/src/runtime/decimal-value.inline.h: Line 97: // Avoid using on Clang: it regresses performance. how about opening a JIRA about this with whatever details you have already and referencing it here? Line 150: // library but not the GCC runtime library and regresses performance. same http://gerrit.cloudera.org:8080/#/c/2486/7/be/src/runtime/descriptors.cc File be/src/runtime/descriptors.cc: Line 668: if (slot->type().type == TYPE_CHAR) return NULL; why do we do this? Line 672: sometimes does it really sometimes pack them and sometimes not? Line 678: field_idx_ let's update the comment for field_idx_ about the padding. also, maybe we should rename this llvm_field_idx_? Line 689: // the contents are aligned without additional padding. I'm not sure what this comment is trying to say. since we've fully padded, wouldn't we get the same struct whether it was marked packed or not? Line 696: // the tuple to be disabled. why should this ever fail? Line 700: DCHECK_EQ(layout->getSizeInBytes(), byte_size()) << DebugString(); oh, i see we don't expect it to fail. do we ever exercise the return NULL case in testing? -- To view, visit http://gerrit.cloudera.org:8080/2486 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I17d7afd05ad3b472a0bfe035bfc3daada5597b2d Gerrit-PatchSet: 7 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Skye Wanderman-Milne <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
