Tim Armstrong has posted comments on this change. Change subject: IMPALA-3637,IMPALA-3636: refactor codegen constant replacement ......................................................................
Patch Set 2: (11 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: DCHEC > Do you think Constant* will be more appropriate ? Value* may be a bit gener Good idea. After looking at the LLVM class hierarchy, everything we could possibly want to replace is under Constant. http://gerrit.cloudera.org:8080/#/c/3843/1/be/src/codegen/constant-replacement.h File be/src/codegen/constant-replacement.h: Line 1: // Licensed to the Apache Software Foundation (ASF) under one > This header needs updating. Updated them all. Verified using "Git show" and grepping for Cloudera http://gerrit.cloudera.org:8080/#/c/3843/1/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: PS1, Line 829: D > nit: "//". It should be fine to declare this in the header file too. Not mu Done. I think it's easier not to, since then I'll need to include additional LLVM header files. PS1, Line 898: } > nit: "//" Done PS1, Line 900: if (is_corrupt_) return Status("Module is corrupt."); > Did you measure how costly this extra optimization pass is ? I just tried instrumenting this code for TPC-H Q18 (a moderately complex query). The time taken is negligible I0805 16:49:01.824277 31068 llvm-codegen.cc:904] Const replacement took 2us I0805 16:49:01.824304 31068 llvm-codegen.cc:908] Dead inst elim took 13us I0805 16:49:01.865865 31082 llvm-codegen.cc:904] Const replacement took 2us I0805 16:49:01.865890 31082 llvm-codegen.cc:908] Dead inst elim took 15us I0805 16:49:01.891055 31057 llvm-codegen.cc:904] Const replacement took 2us I0805 16:49:01.891083 31057 llvm-codegen.cc:908] Dead inst elim took 12us I did the usual benchmarks a while back. 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: // Licensed to the Apache Software Foundation (ASF) under one > This header needs updating. Done 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 Fu Thats a good point. 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: // Licensed to the Apache Software Foundation (ASF) under one > This header needs updating. Done PS1, Line 41: const v > It seems unnecessary to have a separate class for this. May be we can forgo Done Line 82: } > nit: wrong indentation. Done Line 84 > Referring to the previous comment about combining this with the LLVM consta Right, the way I look at it is that ConstantReplacer is a general utility, and LlvmCodegen internally wraps it into a pass as a convenient way to apply it to all the functions. I think we actually already implicitly enforce the property you mention since you need to provide FunctionContext::TypeDesc, etc when constructing ArgTypeConstantReplacer. If someone was trying to use this in a global context, I think they'd have to go to some lengths to construct dummy inputs. We could add an additional enum/boolean flag to specify whether it's valid globally, but I'm not sure if that provides much of a safeguard. -- 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
