impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4164: Use inline hints instead of always-inline
Date Fri, 26 May 2017 23:31:30 GMT
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 <kwho@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message