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 Fri, 03 Feb 2017 04:52:04 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 5:

(6 comments)

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

PS4, Line 326: n both modules. For the latter case, it's
> They are not necessarily just function declarations in the new module. They
When you say "to be safe, we need to call MaterializeFunctionHelper with the maps of the main
module and the new module", where do we create the maps of the new module?

The new comment is clearer, thanks. But I'm still confused as to why it's okay to not materialize
the function from the new module.


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

Line 173:   RETURN_IF_ERROR(init_codegen->MaterializeModule());
seems fine, but why is this needed now but not before? oh i guess it's required to get the
uses list used by FindGlobalUsers()?


PS5, Line 185: functions
functions / global-variables 
(unless you change the code due to the next comment)


Line 201:         fns_to_always_materialize_.insert(fn_name);
if we put it in fns_to_always_materialize_ (and therefore will be materialized), why does
it also go into fn_refs_map_?  i.e. should we simplify fn_refs_map_ to just be map from function
to functions?  doesn't seem like we should care about references from global vars when traversing
the call graph. (And then the name CalleeMap would have made sense but new name is fine too,
or CallGraph which is what it would be, right?).


PS5, Line 328: Note that
             :   // linking will cause functions defined only in the new module to be materialized.
does linkModule() actually materialize functions? or is this saying that once the new module
is linked in, the new definitions will be chosen when doing module_->materialize(fn)? or
something else?


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

PS5, Line 146: be
by


-- 
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: 5
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