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 <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
