Tim Armstrong has posted comments on this change. Change subject: IMPALA-3637,IMPALA-3636: refactor codegen constant replacement ......................................................................
Patch Set 5: (14 comments) Thanks for the review. I'll do a local perf run on targeted-perf before committing just to double-check the perf impact. 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 de Done http://gerrit.cloudera.org:8080/#/c/3401/4/be/src/codegen/constant-replacement.h File be/src/codegen/constant-replacement.h: Line 45: /// The full mangled function symbol. > I don't think this is needed for usability, since it is just the constructo Done PS4, Line 57: lic: > How do these vcalls affect codegen time? It'll be rounding error. They're only going to be called when there is actually a constant to replace (i.e. at most once per function call in the IR). LLVM's overhead for its optimisation and machine code generation is much higher per instruction. PS4, Line 75: O > Only >= 0 is checked in TryReplace. Done Line 101: } > This method can be const Done Line 108: } > if using std::vector above, please use std::vector everywhere in this file Hm, I'm surprised this compiled. I guess one of the headers has a "using" in it. Line 116: return entry->second.value; > Please use override when possible Done Line 124: /// Track the number of times the symbol as been replaced. > Can you give this type a more descriptive name? Maybe even using a std::pai SimpleNoArgConstantReplacer::MapEntry seems ok to me. I'll rename it SymbolMapEntry to be a bit more explicit. I think pair makes the above code a lot less readable. E.g. ++entry->second.second is pretty cryptic. 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: e > const int num_build_tuples, here and below Done 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 f Done Line 608: const FunctionContext::TypeDesc& type = arg_types_[arg]; > Why do the error checking here but not in the symbol= GET_* conditionals? The arg value is potentially coming from a UDF or UDA whereas the symbol values are coming from internal Impala code (actually HandledSymbols() just above). We don't expose GetArgPrecision(), etc, to non-builtin UDFs yet, but I wanted to set the precedent here of validating input. 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 { > long line Done 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 = > long line Done PS4, Line 202: > https://isocpp.org/wiki/faq/ctors#static-init-order It should be safe here: I can't think of any possible scenario where a static initializer would be doing codegen. Added a comment just to document this. -- 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: 5 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
