Jim Apple has posted comments on this change. Change subject: IMPALA-3637,IMPALA-3636: refactor codegen constant replacement ......................................................................
Patch Set 4: (14 comments) http://gerrit.cloudera.org:8080/#/c/3401/4/be/src/codegen/constant-replacement.cc File be/src/codegen/constant-replacement.cc: Line 45: if (isa<ConstantInt>(call_arg)) { if false, replacement is never initialized. Can you initialize it at its declaration site, please? http://gerrit.cloudera.org:8080/#/c/3401/4/be/src/codegen/constant-replacement.h File be/src/codegen/constant-replacement.h: Line 45: static ReplaceableSymbol OneArg(const std::string& symbol, int constant_arg_index) { I don't think this is needed for usability, since it is just the constructor PS4, Line 57: Abstract How do these vcalls affect codegen time? PS4, Line 75: 1 Only >= 0 is checked in TryReplace. Line 101: int CallsReplaced(const std::string& symbol) { This method can be const Line 108: virtual vector<ReplaceableSymbol> HandledSymbols() { if using std::vector above, please use std::vector everywhere in this file Line 116: virtual llvm::Value* ReplaceNoArgs(LlvmCodeGen* codegen, const std::string& symbol) { Please use override when possible Line 124: struct MapEntry { Can you give this type a more descriptive name? Maybe even using a std::pair would be fine http://gerrit.cloudera.org:8080/#/c/3401/4/be/src/exec/partitioned-aggregation-node.cc File be/src/exec/partitioned-aggregation-node.cc: PS4, Line 1839: 1 const int num_build_tuples, here and below http://gerrit.cloudera.org:8080/#/c/3401/4/be/src/exprs/expr.cc File be/src/exprs/expr.cc: Line 599: } else { COnsider taking this out of the else block. That way, if the DCHECK would fail in a release build, at least it still returns something deterministic, not whatever happens to be in that register. Line 608: if (arg < 0 || arg >= arg_types_.size()) return NULL; Why do the error checking here but not in the symbol= GET_* conditionals? http://gerrit.cloudera.org:8080/#/c/3401/4/be/src/udf/udf-internal.h File be/src/udf/udf-internal.h: Line 99: int IR_NO_INLINE GetArgPrecision(int arg_idx) const { return arg_types_[arg_idx].precision; } long line http://gerrit.cloudera.org:8080/#/c/3401/4/be/src/util/cpu-info.cc File be/src/util/cpu-info.cc: Line 200: const char* CpuInfoConstantReplacer::IS_SUPPORTED_SYMBOL = "_ZN6impala7CpuInfo11IsSupportedEl"; long line PS4, Line 202: static https://isocpp.org/wiki/faq/ctors#static-init-order -- To view, visit http://gerrit.cloudera.org:8080/3401 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6ba029ed8589698eb15dbfb0a20dd2a7ea752635 Gerrit-PatchSet: 4 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
