impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-3637,IMPALA-3636: refactor codegen constant replacement
Date Sat, 03 Sep 2016 01:30:30 GMT
Michael Ho has posted comments on this change.

Change subject: IMPALA-3637,IMPALA-3636: refactor codegen constant replacement

Patch Set 2:


The change looks good. Mostly comments are about clarification in functions' comments.
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 ?
File be/src/codegen/

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() ?
File be/src/exprs/

PS2, Line 644:     int return_precision = \
             :         context->impl()->GetReturnPrecision(); \
one line ?
File be/src/exprs/

Line 41:       const vector<FunctionContext::TypeDesc>& arg_types) :
nit: indentation
File be/src/runtime/types.h:

PS2, Line 238: inline int IR_ALWAYS_INLINE

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I6ba029ed8589698eb15dbfb0a20dd2a7ea752635
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <>
Gerrit-Reviewer: Jim Apple <>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message