Michael Ho has posted comments on this change. Change subject: IMPALA-3637,IMPALA-3636: refactor codegen constant replacement ......................................................................
Patch Set 2: (16 comments) The change looks good. Mostly comments are about clarification in functions' comments. http://gerrit.cloudera.org:8080/#/c/3843/2/be/src/codegen/constant-replacement.h File be/src/codegen/constant-replacement.h: PS2, Line 47: we often want to ignore non-constant arguments. I.e. NO_ARG Can this be shortened to: Note that NO_ARG doesn't imply ... PS2, Line 75: replace be replaced PS2, Line 88: 10 a LLVM ConstantInt 10 ? Line 93: /// will then be called with name = "some_function" and arg = 24. If the argument is not It may help to point out that ReplaceOneArg() is usually used if replacement constant value is derived from the input constant arg. PS2, Line 104: constant value LLVM constant PS2, Line 106: ReplaceNoArgs Should ReplaceNoArgs() and ReplaceOneArgs() be protected ? PS2, Line 112: = NO_ARG != NO_ARG ? PS2, Line 113: value integral value PS2, Line 114: constant value LLVM constant. PS2, Line 116: Only integral constant args for now. Currently only support integral constant args. Line 150: std::vector<ReplaceableFunction> result; Do we expect this to be usually called once ? It appears that we could have cached this in a set when AddReplacement() is called instead of computing it every time ? http://gerrit.cloudera.org:8080/#/c/3843/2/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: PS2, Line 798: fn_map.find(callee->getName()) fn_map.find(callee->getName()) may be evaluated once in this statement: if (callee != NULL) { unordered_map<string, ReplaceableFunction>::const_iterator fn_map_entry = fn_map.find(callee->getName()); if (fn_map_entry == fn_map.end()) continue; ... .... } PS2, Line 1014: !fn.isDeclaration() !fn.isDeclaration() && !fn.isIntrinsic() ? http://gerrit.cloudera.org:8080/#/c/3843/2/be/src/exprs/decimal-operators-ir.cc File be/src/exprs/decimal-operators-ir.cc: PS2, Line 644: int return_precision = \ : context->impl()->GetReturnPrecision(); \ one line ? http://gerrit.cloudera.org:8080/#/c/3843/2/be/src/exprs/expr-constant-replacement.cc File be/src/exprs/expr-constant-replacement.cc: Line 41: const vector<FunctionContext::TypeDesc>& arg_types) : nit: indentation http://gerrit.cloudera.org:8080/#/c/3843/2/be/src/runtime/types.h File be/src/runtime/types.h: PS2, Line 238: inline int IR_ALWAYS_INLINE ALWAYS_INLINE int -- To view, visit http://gerrit.cloudera.org:8080/3843 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6ba029ed8589698eb15dbfb0a20dd2a7ea752635 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
