impala-reviews 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-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
Date Sat, 21 Jan 2017 02:46:02 GMT
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 <kwho@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message