Tim Armstrong has posted comments on this change. Change subject: IMPALA-775,IMPALA-3374: Upgrade LLVM to 3.8.0 ......................................................................
Patch Set 9: (15 comments) http://gerrit.cloudera.org:8080/#/c/2486/9/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: Line 587: > This is replaced by the inlining pass in the optimization phase, right ? Yes, the default inlining pass will always inline functions with that attribute. There isn't a major point to doing this inlining here without the subexpr-elimination pass, which was disabled. Line 97: llvm::InitializeNativeTargetAsmPrinter(); : llvm::InitializeNativeTargetAsmParser(); > Just curious what these are for ? Mind adding a comment about them ? Done Line 184: std::string module_name > nit: wrong indentation. Done Line 225: if (CpuInfo::IsSupported(CpuInfo::SSE4_2)) { > Can we move this if-else statement to InitializeLlvm() as it shouldn't chan The codegen test relies on being able to change the cpu features during tests, so it's not an easy change. Line 316: vector<string> cpu_attrs; : GetHostCPUAttrs(&cpu_attrs); > Can this be done once at InitializeLlvm() ? Done Line 535: if (is_corrupt_) LOG(ERROR) << str; > Can this line be moved to the if-stmt below ? I don't think easily, since str is filled in by verifyFunction. Line 649: if (optimizations_enabled_ && !FLAGS_disable_optimization_passes) { : OptimizeModule(); > nit: one line ? Done Line 785: AddFunctionToJitInternal( > Does this need to be a separate function ? It's used by the test code, this seemed preferable to having the test code accessing the member variables directly. Line 786: DCHECK(!is_compiled_); > May be helpful to have this DCHECK at the beginning of AddFunctionToJit() i This is called from the tests, so we'd have to duplicate it in both places - not sure if it's worth it. http://gerrit.cloudera.org:8080/#/c/2486/9/be/src/codegen/llvm-codegen.h File be/src/codegen/llvm-codegen.h: Line 283: > by? Done Line 421: ObjectPool*, > nit: can we also have a parameter name for consistency ? Done Line 423: boost::scoped_ptr<LlvmCodeGen>* codegen); > not your change, but it's not very clear what the difference between these I changed the naming so that the ones that construct a codegen object are called Create* and the ones that construct the module only are called LoadModuleFrom*. Also fixed the weird static-method-with-codegen-argument pattern. The interface used by lib-cache is a little weird (there's no inherent need to construct a codegen object to get symbols from a file) but I think it's almost not worth the effort to avoid that, since it would require extra boilerplate setting up LLVMContexts. http://gerrit.cloudera.org:8080/#/c/2486/9/be/src/exprs/scalar-fn-call.cc File be/src/exprs/scalar-fn-call.cc: Line 294: builder.CreateStructGEP(NULL > I wonder if providing type information may help with alias analysis. NULL here means "infer the pointer type from the type of the argument", so it shouldn't make a difference. My understanding is that LLVM doesn't use the pointer type at all in alias analysis, only tbaa annotations. http://gerrit.cloudera.org:8080/#/c/2486/9/be/src/runtime/decimal-value.inline.h File be/src/runtime/decimal-value.inline.h: Line 148: __builtin_add_overflow > __builtin_mul_overflow ? Done http://gerrit.cloudera.org:8080/#/c/2486/9/be/src/runtime/descriptors.cc File be/src/runtime/descriptors.cc: Line 681: > nit: wrong indentation. Done -- 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: 9 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
