Tim Armstrong has posted comments on this change. Change subject: Upgrade LLVM to 3.8.0 ......................................................................
Patch Set 7: (27 comments) http://gerrit.cloudera.org:8080/#/c/2486/7/be/CMakeLists.txt File be/CMakeLists.txt: Line 307: ${LLVM_SYSTEM_LIBS} I ran into an issue on the centos build that was related to linking zlib (decompressor-test failed). Instead of trying to be smart and getting the system libs from LLVM, I switched to manually adding LLVM's dependencies to this list. Line 324: # ${LLVM_BUILTIN_LIBS} > remove Done 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 the I don't think so. Done. Line 309: GetHostCPUAttrs(&cpu_attrs); > do you think it would be helpful to log this? feel free to ignore. I added code to InitialiseLlvm() to log these. Seems useful. I0413 14:16:04.894820 18223 llvm-codegen.cc:109] CPU class for runtime code generation: haswell I0413 14:16:04.894876 18223 llvm-codegen.cc:113] CPU flags for runtime code generation: -sse4a,-avx512bw,+cx16,-tbm,+xsave,-fma4,-avx512vl,-prfchw,+bmi2,-adx,-xsavec,+fsgsbase,+avx,-avx512cd,-avx512pf,-rtm,+popcnt,+fma,+bmi,+aes,+rdrnd,-xsaves,+sse4.1,+sse4.2,+avx2,-avx512er,+sse,+lzcnt,+pclmul,-avx512f,+f16c,+ssse3,+mmx,-pku,+cmov,-xop,-rdseed,+movbe,-hle,+xsaveopt,-sha,+sse2,+sse3,-avx512dq Line 530: return false; > will we notice during testing if this thing returns false? It has hit a DCHECK in scalar-fn-call.cc for me before. Line 634: no functions are successfully codegen'd > in what case would that happen? i.e. what does "successfully" mean in this I think if we hit an unsupported case, e.g. CHAR() partway through the codegen and don't actually produce a function. Line 661: > consider removing some blank lines Done 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 I traced this back and realised that we can get rid of jitted_functions_, jitted_functions_lock_ and inline GetJittedFunction(). It was relevant when different exec nodes would call GetJittedFunction() to get their codegened function pointers. 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? Done Line 138: boost::scoped_ptr<LlvmCodeGen>* codegen); > Maybe make this private. Done Line 287: this > Not your change, but while we're here let's try to clarify a few things. 't Done 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. I removed that function just now, so no need. Line 291: those objects > what objects? Removed. 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 abo Done Line 410: std::unique_ptr<llvm::Module>* module); > any reason to not make this private? Done Line 422: std::string module_name, std::unique_ptr<llvm::Module>* module); > and this one? Done Line 445: skips type checking > what type checking? is this referring to the decimal wrapper stuff? Done Line 454: he function : /// must have been registered in AddFunctionToJit(). > is this actually required by GetJittedFunction() or it just happens that we Deleted function. Line 517: std::set<llvm::Function*> jitted_functions_; > do we still need this? No. Deleted it. 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 https://issues.cloudera.org/browse/IMPALA-3345 Line 150: // library but not the GCC runtime library and regresses performance. > same Done 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? It doesn't look like GetType() is implemented correctly for CHAR (it returns a StringVal struct). I added a DCHECK to GetType() and a comment here to explain. Line 672: sometimes > does it really sometimes pack them and sometimes not? Yes, in some cases I see it set the packed flag and sometimes it doesn't. I think the story is that Clang can't always rely on LLVM's alignment rules to implement C's packing rules so sometimes falls back to manually aligning structs. Line 678: field_idx_ > let's update the comment for field_idx_ about the padding. also, maybe we Done, including rename. 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, Yeah, that's what it was trying to say (it shouldn't matter whether it's marked packed or not). Line 696: // the tuple to be disabled. > why should this ever fail? I don't think it should. 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 c It shouldn't be possible unless the frontend messed something up. I'm going to delete some of this check code and just add add appropriate DCHECKs. -- 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 <tarmstr...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Skye Wanderman-Milne <s...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
