Michael Ho has posted comments on this change. Change subject: Upgrade LLVM to 3.8.0 ......................................................................
Patch Set 9: (13 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 ? Line 97: llvm::InitializeNativeTargetAsmPrinter(); : llvm::InitializeNativeTargetAsmParser(); Just curious what these are for ? Mind adding a comment about them ? Line 184: std::string module_name nit: wrong indentation. Line 225: if (CpuInfo::IsSupported(CpuInfo::SSE4_2)) { Can we move this if-else statement to InitializeLlvm() as it shouldn't change after initialization ? Line 316: vector<string> cpu_attrs; : GetHostCPUAttrs(&cpu_attrs); Can this be done once at InitializeLlvm() ? Line 535: if (is_corrupt_) LOG(ERROR) << str; Can this line be moved to the if-stmt below ? Line 649: if (optimizations_enabled_ && !FLAGS_disable_optimization_passes) { : OptimizeModule(); nit: one line ? Line 785: AddFunctionToJitInternal( Does this need to be a separate function ? Line 786: DCHECK(!is_compiled_); May be helpful to have this DCHECK at the beginning of AddFunctionToJit() instead. http://gerrit.cloudera.org:8080/#/c/2486/9/be/src/codegen/llvm-codegen.h File be/src/codegen/llvm-codegen.h: Line 421: ObjectPool*, nit: can we also have a parameter name for consistency ? 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. 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 ? http://gerrit.cloudera.org:8080/#/c/2486/9/be/src/runtime/descriptors.cc File be/src/runtime/descriptors.cc: Line 681: nit: wrong indentation. -- 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 <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
