impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
Date Wed, 01 Feb 2017 04:55:42 GMT
Dan Hecht has posted comments on this change.

Change subject: IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
......................................................................


Patch Set 4:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/5732/4//COMMIT_MSG
Commit Message:

Line 50: defined in the boost library.
is there some verification code we can add to ensure that this problem won't happen again
(or isn't happening elsewhere)?


http://gerrit.cloudera.org:8080/#/c/5732/4/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:

PS4, Line 326: which are also defined in the main module
this doesn't seem to match what the code is doing.  Isn't it materializing functions that
are referenced by functions of the new module?

But why is this needed? why don't they get materialized when we follow the chain of references
in MaterializeFunctionHelper()?


http://gerrit.cloudera.org:8080/#/c/5732/4/be/src/codegen/llvm-codegen.h
File be/src/codegen/llvm-codegen.h:

PS4, Line 85: their callee functions
what is a "callee" for a global variable? do you mean reference?
If so, how about:

// Map from global variables/functions to the functions that they reference.


PS4, Line 86: CalleeMap
again, callee sounds to me like only functions would be keys in this map.  FunctionRefsMap?


PS4, Line 160: f 'eager' is false, the
             :   /// functions in the module are not materialized.
this seems to contradict the class header comment that says "llvm::Function objects in the
module are materialized lazily". i.e. the behavior you are documented is the expected behavior,
no?

So are you adding 'eager' so that the default behavior can be overridden? if so, it should
say something like: 

If 'eager' is true, the functions are materalized. Otherwise they are materialized lazily
via GetFunction().

And if that's what's going on, why not call the parameter 'materialize"?

But, as a caller, how do I choose whether I'm suppose to materialize or not at this point?
The need for this seems to contradict the whole explanation about lazy materialization in
the header comment.

Finally, rather than threading through this parameter, why not just have the caller do module->MaterializeAll()
with the returned module?


PS4, Line 617:  which also need to be materialized if
             :   /// the input global variable or function is materialized
is this really specific to materialization? wouldn't it be enough to just say:

... all the functions referenced by it.

The materialization code itself can explain (or be self documenting) about why references
need to be followed when materializing.


PS4, Line 625: fns_to_materialize_
why this name change?  does this now store functions that need to be materialized for various
reasons? don't we materialize functions that aren't in this set?


http://gerrit.cloudera.org:8080/#/c/5732/4/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:

since this takes so long to execute, should we either move expr-test.cc to run only in exhaustive,
or run only codegen tests when exhaustive?


-- 
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message