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 Mon, 22 May 2017 18:10:24 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4164: Use inline hints instead of always-inline
......................................................................


Patch Set 2:

(2 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();
This solves the inlining problem but the outcome is a bit counterintuitive - it's taking the
target attributes from the IR module (which is the lowest-common-denominator set of features
that each function was originally compiled for) and copying them to all the functions. 

I think this makes an incorrect assumption that all functions in the module have the same
target attributes, which wouldn't be true for, say, AVX2-specific functions. So if one of
those functions was in the module the outcome would depend on the (semi-arbitrary) order of
the functions in the IR module.

At a minimum it needs some explanation, but I think it would make more sense to ensure that
cross-compiled functions target the current machine, rather than the lowest-common-denominator.
If I remember correctly, if the function has no attributes set then it defaults to the execution
engine's, which match the current machine.

This would also potentially causes problems if we ever wanted to create functions with instructions
like AVX2.


Line 166:       LOG(INFO) << "target_features_attr: " << target_features_attr_;
Should this logging be left in?


-- 
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: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message