Tim Armstrong has posted comments on this change. Change subject: IMPALA-4164: Use inline hints instead of always-inline ......................................................................
Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/6941/2/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: Line 163: target_cpu_attr_ = fn->getFnAttribute("target-cpu").getValueAsString(); > The Jenkins VM apparently doesn't have avx2. I updated PhjBuilder to inline Ok, I guess we can defer dealing with that issue. Can you make a comment at the place where we add the function attribute mentioning that we assume that cross-compiled functions don't have attributes that are unsupported on this system. Just so that the caveat to the approach is documented. Line 645: // If there is no "noinline" attribute, add an inline hint to the function. See my other comment about whether InlineHint is required. http://gerrit.cloudera.org:8080/#/c/6941/7/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: Line 700: LOG(ERROR) << "Function corrupt: " << fn_name; It isn't limited to cross-compiled functions is it? Line 704: return true; Instead of adding the hint everywhere it seems better to adjust the inlining threshold by passing it in to createFunctionInliningPass(). It looks like the threshold for inlining hints is 325 vs 225 http://llvm.org/docs/doxygen/html/InlineCost_8cpp_source.html#l00053 I'm assuming the goal here was to inline a bit more aggressively than the default settings. -- To view, visit http://gerrit.cloudera.org:8080/6941 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2d87ae8d222b415587e7320cb9072e4a8d6615ce Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
