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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
