Michael Ho has posted comments on this change. Change subject: IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen ......................................................................
Patch Set 1: (8 comments) http://gerrit.cloudera.org:8080/#/c/5732/1/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: PS1, Line 119: Value > Function and GlobalVariable are both subclasses of GlobalObject so it would Done PS1, Line 124: getParent()->getParent() > getFunction()? Done PS1, Line 149: fn_name > Is this right? It doesn't reference any variables that are updated within t For global variables, we only count those functions not defined in Impalad binary. Line 382: for (const string& gv: gv_with_fns_) { > This is the only place this is used, so we could instead just directly stor Done http://gerrit.cloudera.org:8080/#/c/5732/1/be/src/codegen/llvm-codegen.h File be/src/codegen/llvm-codegen.h: PS1, Line 511: FindUses > FindGlobalUses? Renamed to FindGlobalUsers(); Line 516: static void FillCalleeMap(llvm::Function* fn, CalleeMap& callee_map, > The name and comment should communicate that this excludes functions define This function is removed in the new patch. http://gerrit.cloudera.org:8080/#/c/5732/1/be/src/exprs/timestamp-functions-ir.cc File be/src/exprs/timestamp-functions-ir.cc: Line 57 > It seems like we want to be able to inline the constants into the timestamp Without defining MIN_YEAR as a compilation constant, there isn't much to be inlined. This new patch sets MIN_YEAR to value 1400 and adds a DCHECK() to verify it matches Date(min_date_year).year(). Inspected the compiled code to verify these class static variables are global constants in IR. http://gerrit.cloudera.org:8080/#/c/5732/1/be/src/runtime/exec-env.h File be/src/runtime/exec-env.h: Line 115: Status InitForBeTests(); > I think we should avoid replicating the InitForFeTests() pattern - it's kin Done -- To view, visit http://gerrit.cloudera.org:8080/5732 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I40fdb035a565ae2f9c9fbf4db48a548653ef7608 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
