impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <ger...@cloudera.org>
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:

(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 <tarmstrong@cloudera.com>
Gerrit-Reviewer: Jim Apple <jbapple@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message