Michael Ho has posted comments on this change. Change subject: IMPALA-3906: Materialize implicitly referenced IR functions ......................................................................
Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/3740/2/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: PS2, Line 144: Also ignore other global : // variables as InitializeLlvm() will go through all of them. > Not sure what this is saying. What does "other" mean here? Rephrased the comments. It means other global variables in the same module. The reason they should be avoided is that we will parse all global variable one-by-one any way so there is no need to parse other global variables referenced by the current global variable. PS2, Line 320: fn.isMaterializable() > at this point, what functions in new_module are !isMaterializable()? when m This is to avoid built-in functions and declarations. LLVM functions sometimes have bad names. Line 337: } > can there be global variables in the module that need to be inspected for f Good point. Code is updated to also parse the global variables in the source module. http://gerrit.cloudera.org:8080/#/c/3740/2/be/src/codegen/llvm-codegen.h File be/src/codegen/llvm-codegen.h: PS2, Line 546: . > ... that aren't defined in the Impalad native code (they may have been inli Done PS2, Line 546: implicitly > what's implicit? aren't they explicitly referenced by the global variables? It's implicit wrt to GetFunction() but yes, it's explicitly referenced by the global variables. PS2, Line 548: so LLVM cannot resolve references to them if they are not materialized. : /// These functions are always materialized each time the module is loaded. > These functions are always materialized each time the module is loaded to e Done PS2, Line 550: gv_ref_functions_ > since these are a subset of global var referenced functions that we need in Done -- To view, visit http://gerrit.cloudera.org:8080/3740 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3653f55a1aa063b21cb3a5040f502b4c0ecf82e8 Gerrit-PatchSet: 2 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
