Tim Armstrong has posted comments on this change. Change subject: IMPALA-3637,IMPALA-3636: refactor codegen constant replacement ......................................................................
Patch Set 2: (16 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: Done PS2, Line 75: replace > be replaced Done PS2, Line 88: 10 > a LLVM ConstantInt 10 ? Done 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 replaceme Done PS2, Line 104: constant value > LLVM constant Done PS2, Line 106: ReplaceNoArgs > Should ReplaceNoArgs() and ReplaceOneArgs() be protected ? I think if they're protected then it would be invalid to call ReplaceNoArgs() unless you had a pointer to the subclass. PS2, Line 112: = NO_ARG > != NO_ARG ? Done PS2, Line 113: value > integral value Done PS2, Line 114: constant value > LLVM constant. Done PS2, Line 116: Only integral constant args for now. > Currently only support integral constant args. Done Line 150: std::vector<ReplaceableFunction> result; > Do we expect this to be usually called once ? It appears that we could have It should only be called once per function, so shouldn't be too expensive. I think it's probably only worth optimising this if it starts showing up on profiles. 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: Done PS2, Line 1014: !fn.isDeclaration() > !fn.isDeclaration() && !fn.isIntrinsic() ? I think isIntrinsic() implies isDeclaration(), no? isDeclaration() looks like it's true if the function has no body, and I don't think an intrinsic should have a body. 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 ? Done 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 Done 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 Changed it to int ALWAYS_INLINE. This looks like the order we use in most of the codebase. -- 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
