Dan Hecht 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? PS2, Line 320: fn.isMaterializable() at this point, what functions in new_module are !isMaterializable()? when might the materialization happen already? Line 337: } can there be global variables in the module that need to be inspected for function references? 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 inlined by gcc). PS2, Line 546: implicitly what's implicit? aren't they 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 ensure that LLVM can resolve references to them. PS2, Line 550: gv_ref_functions_ since these are a subset of global var referenced functions that we need in IR as opposed to all gv-ref'ed funcs, maybe call them 'gv_ref_ir_funcs_' or similar? -- 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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
