Michael Ho has posted comments on this change. Change subject: IMPALA-3637,IMPALA-3636: refactor codegen constant replacement ......................................................................
Patch Set 1: (12 comments) http://gerrit.cloudera.org:8080/#/c/3843/1/be/src/codegen/constant-replacement.cc File be/src/codegen/constant-replacement.cc: PS1, Line 37: Value Do you think Constant* will be more appropriate ? Value* may be a bit generic. http://gerrit.cloudera.org:8080/#/c/3843/1/be/src/codegen/constant-replacement.h File be/src/codegen/constant-replacement.h: Line 1: // Copyright 2016 Cloudera Inc. This header needs updating. http://gerrit.cloudera.org:8080/#/c/3843/1/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: PS1, Line 829: /// nit: "//". It should be fine to declare this in the header file too. Not much harm in exposing it but up to you. PS1, Line 898: /// nit: "//" PS1, Line 900: fn_pass_manager->add(new ConstantReplacementPass(this)); Did you measure how costly this extra optimization pass is ? http://gerrit.cloudera.org:8080/#/c/3843/1/be/src/codegen/llvm-codegen.h File be/src/codegen/llvm-codegen.h: PS1, Line 376: llvm::Value* GetBoolConstant nice. http://gerrit.cloudera.org:8080/#/c/3843/1/be/src/exec/hash-table-constant-replacer.cc File be/src/exec/hash-table-constant-replacer.cc: Line 1: // Copyright 2016 Cloudera Inc. This header needs updating. http://gerrit.cloudera.org:8080/#/c/3843/1/be/src/exprs/decimal-operators-ir.cc File be/src/exprs/decimal-operators-ir.cc: PS1, Line 271: context->impl()->GetReturnPrecision() An observation when I went through the patch: if we can somehow generate FunctionContext as an LLVM constant, a lot of these call sites replacement won't be needed. That said, I am not advocating we should switch to that approach (at least not now). It's just an alternative. http://gerrit.cloudera.org:8080/#/c/3843/1/be/src/exprs/expr-constant-replacement.cc File be/src/exprs/expr-constant-replacement.cc: Line 1: // Copyright 2016 Cloudera Inc. This header needs updating. PS1, Line 41: ArgType It seems unnecessary to have a separate class for this. May be we can forgo the need of a separate class called ExprConstantReplacement and put everything in this file under the class ExprConstantReplacer ? Line 82: const vector<FunctionContext::TypeDesc>& arg_types, LlvmCodeGen* codegen, nit: wrong indentation. Line 84: ArgTypeConstantReplacer replacer(return_type, arg_types); Referring to the previous comment about combining this with the LLVM constant pass: On the first glance, the code seems to still have two different patterns for doing constant replacement. One is this function and the other one is the LLVM constant pass. However, upon closer look, they are actually different. The replacement values used in this function depend on 'return_type' which may differ from functions to functions. On the other hand, the LLVM constant pass you added is for CpuInfo() whose replacement values will be the same for all functions given the same input. In some sense, this function is a local replacement pass while the LLVM pass is closer to a global replacement pass. It may be helpful to document the difference here as having these two patterns at the same time may be a bit confusing to decide which one to use when. Not sure if it's possible to DCHECK that this type of constant replacer is not used in the LLVM constant replacement pass as it doesn't seem to make sense ? I guess same thing can be said about hash_table_constant_replacer. -- 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: 1 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
