You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@impala.apache.org by "Michael Ho (Code Review)" <ge...@cloudera.org> on 2016/07/25 16:41:56 UTC
[Impala-CR](cdh5-trunk) IMPALA-3906: Materialize implicitly referenced IR functions
Michael Ho has uploaded a new change for review.
http://gerrit.cloudera.org:8080/3740
Change subject: IMPALA-3906: Materialize implicitly referenced IR functions
......................................................................
IMPALA-3906: Materialize implicitly referenced IR functions
With lazy materialization of IR functions in the LLVM module,
there is an assumption that functions not referenced by IR
functions used in the query don't have to be materialized
and can have their linkage types changed to being externally
defined. These unmaterialized functions may be referenced by
global variables in the LLVM module and LLVM resolves these
references to their definitions in the native Impalad binary.
These global variables are mostly arrays containing references
to other global constants or boost and Impala functions included
in the cross-compiled code.
When compiling Impalad with optimization, gcc may actually
inline some of the functions which the global variables in
the LLVM modules reference and LLVM may have linking error
if these referenced IR functions are not materialized as it
can no longer find their definitions in the Impalad binary.
This patch fixes the problem by parsing all the global variables
in the LLVM module during Impalad's initialization and recording
all the referenced functions which aren't defined in the Impalad
binary and make sure they are always materialized.
A second problem which this patch fixes is that linking in
external LLVM module (e.g. UDF IR created by a user) may
implicitly materialize some functions in the external module.
Normally, we would expect functions to be materialized
through the GetFunction() interface and LinkModule() seems
to be an exception. This patch updates LinkModule() to also
parse the functions from the external module just like we do
for unmaterialized functions in GetFunction().
Change-Id: I3653f55a1aa063b21cb3a5040f502b4c0ecf82e8
---
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/util/symbols-util-test.cc
3 files changed, 121 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/40/3740/1
--
To view, visit http://gerrit.cloudera.org:8080/3740
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I3653f55a1aa063b21cb3a5040f502b4c0ecf82e8
Gerrit-PatchSet: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
[Impala-CR](cdh5-trunk) IMPALA-3906: Materialize implicitly referenced IR functions
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has uploaded a new patch set (#2).
Change subject: IMPALA-3906: Materialize implicitly referenced IR functions
......................................................................
IMPALA-3906: Materialize implicitly referenced IR functions
With lazy materialization of IR functions in the LLVM module,
there is an assumption that functions not referenced by IR
functions used in the query don't have to be materialized
and can have their linkage types changed to being externally
defined. These unmaterialized functions may be referenced by
global variables in the LLVM module and LLVM resolves these
references to their definitions in the native Impalad binary.
These global variables are mostly arrays containing references
to other global constants or boost and Impala functions included
in the cross-compiled code.
When compiling Impalad with optimization, gcc may actually
inline some of the functions which the global variables in
the LLVM modules reference and LLVM may have linking error
if these referenced IR functions are not materialized as it
can no longer find their definitions in the Impalad binary.
This patch fixes the problem by parsing all the global variables
in the LLVM module during Impalad's initialization and recording
all the referenced functions which aren't defined in the Impalad
binary and make sure they are always materialized.
A second problem which this patch fixes is that linking in
external LLVM module (e.g. UDF IR created by a user) may
implicitly materialize some functions in the external module.
Normally, we would expect functions to be materialized
through the GetFunction() interface and LinkModule() seems
to be an exception. This patch updates LinkModule() to also
parse the functions from the external module just like we do
for unmaterialized functions in GetFunction().
Change-Id: I3653f55a1aa063b21cb3a5040f502b4c0ecf82e8
---
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/util/symbols-util-test.cc
3 files changed, 127 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/40/3740/2
--
To view, visit http://gerrit.cloudera.org:8080/3740
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3653f55a1aa063b21cb3a5040f502b4c0ecf82e8
Gerrit-PatchSet: 2
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-CR](cdh5-trunk) IMPALA-3906: Materialize implicitly referenced IR functions
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-3906: Materialize implicitly referenced IR functions
......................................................................
Patch Set 1:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/3740/1/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
Line 106: bool LlvmCodeGen::IsDefinedInImpalad(const Function* fn) {
Maybe the argument should just be the function name?
Line 109: Status status = LibCache::instance()->GetSoFunctionPtr("", fn_name, &fn_ptr, NULL, true);
Long line.
PS1, Line 122: dyn_cast
I think you want cast<> instead of dyn_cast<> here and below since you don't want a NULL back if the types dont' match.
Line 138: } else if (isa<ConstantVector>(val) || isa<ConstantDataVector>(val)) {
Maybe DCHECK that this is true? I think we want to know if another type of GlobalValue crops up in our cross-compiled IR.
Line 147: Function* fn = fn_list[i];
I don't think we need the index, so we can simplify to:
for (Function* fn: fn_list) {
}
Line 328: for (int i = 0; i < fn_names.size(); ++i) {
for (const string& fn_name: fn_names) {
Line 372: codegen->MaterializeFunction(&fn);
Do you have an idea of how many additional functions we're materialising? Is there a noticeable overhead?
http://gerrit.cloudera.org:8080/#/c/3740/1/be/src/codegen/llvm-codegen.h
File be/src/codegen/llvm-codegen.h:
PS1, Line 550: set
unordered_set? I believe std::set is a balanced binary tree that isn't very efficient for this purpose.
--
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: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3906: Materialize implicitly referenced IR functions
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has abandoned this change.
Change subject: IMPALA-3906: Materialize implicitly referenced IR functions
......................................................................
Abandoned
Review is moved over to https://gerrit.cloudera.org/#/c/3792/
--
To view, visit http://gerrit.cloudera.org:8080/3740
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: I3653f55a1aa063b21cb3a5040f502b4c0ecf82e8
Gerrit-PatchSet: 4
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-CR](cdh5-trunk) IMPALA-3906: Materialize implicitly referenced IR functions
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-3906: Materialize implicitly referenced IR functions
......................................................................
Patch Set 1:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/3740/1/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
Line 106: bool LlvmCodeGen::IsDefinedInImpalad(const Function* fn) {
> Maybe the argument should just be the function name?
Done
Line 109: Status status = LibCache::instance()->GetSoFunctionPtr("", fn_name, &fn_ptr, NULL, true);
> Long line.
Done
PS1, Line 122: dyn_cast
> I think you want cast<> instead of dyn_cast<> here and below since you don'
Done
Line 138: } else if (isa<ConstantVector>(val) || isa<ConstantDataVector>(val)) {
> Maybe DCHECK that this is true? I think we want to know if another type of
I added a DCHECK in a new else clause after this one. Please see if it makes sense to you.
Line 147: Function* fn = fn_list[i];
> I don't think we need the index, so we can simplify to:
Done
PS1, Line 326: "picked"
> "merged"
Done
Line 328: for (int i = 0; i < fn_names.size(); ++i) {
> for (const string& fn_name: fn_names) {
Done
Line 372: codegen->MaterializeFunction(&fn);
> Do you have an idea of how many additional functions we're materialising? I
It's adding 12 functions. There is slightly more overhead based on rough estimate from the profile.They will be removed in a patch for IMPALA-1112.
_ZN5boost9date_time10date_facetINS_9gregorian4dateEcSt19ostreambuf_iteratorIcSt11char_traitsIcEEE23set_iso_extended_formatEv
_ZN5boost9date_time10date_facetINS_9gregorian4dateEcSt19ostreambuf_iteratorIcSt11char_traitsIcEEE14set_iso_formatEv
_ZN5boost9date_time10date_facetINS_9gregorian4dateEcSt19ostreambuf_iteratorIcSt11char_traitsIcEEED0Ev
_ZN5boost9date_time10date_facetINS_9gregorian4dateEcSt19ostreambuf_iteratorIcSt11char_traitsIcEEED2Ev
_ZN5boost6detail15sp_counted_baseD0Ev
_ZNK5boost9date_time10date_facetINS_9gregorian4dateEcSt19ostreambuf_iteratorIcSt11char_traitsIcEEE9do_put_tmES7_RSt8ios_basecRK2tmSs
_ZNK5boost9date_time10date_facetINS_9gregorian4dateEcSt19ostreambuf_iteratorIcSt11char_traitsIcEEE14do_put_specialES7_RSt8ios_basecNS0_14special_valuesE
_ZN5boost6detail15sp_counted_baseD2Ev
_ZN5boost9date_time10time_facetINS_10posix_time5ptimeEcSt19ostreambuf_iteratorIcSt11char_traitsIcEEE14set_iso_formatEv
_ZN5boost9date_time10time_facetINS_10posix_time5ptimeEcSt19ostreambuf_iteratorIcSt11char_traitsIcEEE23set_iso_extended_formatEv
_ZN5boost9date_time10time_facetINS_10posix_time5ptimeEcSt19ostreambuf_iteratorIcSt11char_traitsIcEEED0Ev
_ZN5boost9date_time10time_facetINS_10posix_time5ptimeEcSt19ostreambuf_iteratorIcSt11char_traitsIcEEED2Ev
http://gerrit.cloudera.org:8080/#/c/3740/1/be/src/codegen/llvm-codegen.h
File be/src/codegen/llvm-codegen.h:
PS1, Line 550: set
> unordered_set? I believe std::set is a balanced binary tree that isn't very
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: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3906: Materialize implicitly referenced IR functions
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-3906: Materialize implicitly referenced IR functions
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3740/2/be/src/codegen/llvm-codegen.h
File be/src/codegen/llvm-codegen.h:
Line 436: static bool IsDefinedInImpalad(const std::string fn);
> Missing & to pass by reference
Oops. 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 <kw...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3906: Materialize implicitly referenced IR functions
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3740
to look at the new patch set (#4).
Change subject: IMPALA-3906: Materialize implicitly referenced IR functions
......................................................................
IMPALA-3906: Materialize implicitly referenced IR functions
With lazy materialization of IR functions in the LLVM module,
there is an assumption that functions not referenced by IR
functions used in the query don't have to be materialized
and can have their linkage types changed to being externally
defined. These unmaterialized functions may be referenced by
global variables in the LLVM module and LLVM resolves these
references to their definitions in the native Impalad binary.
These global variables are mostly arrays containing references
to other global constants or boost and Impala functions included
in the cross-compiled code.
When compiling Impalad with optimization, gcc may actually
inline some of the functions which the global variables in
the LLVM modules reference and LLVM may have linking error
if these referenced IR functions are not materialized as it
can no longer find their definitions in the Impalad binary.
This patch fixes the problem by parsing all the global variables
in the LLVM module during Impalad's initialization and recording
all the referenced functions which aren't defined in the Impalad
binary and make sure they are always materialized.
A second problem which this patch fixes is that linking in
external LLVM module (e.g. UDF IR created by a user) may
implicitly materialize some functions in the external module.
Normally, we would expect functions to be materialized
through the GetFunction() interface and LinkModule() seems
to be an exception. This patch updates LinkModule() to also
parse the functions from the external module just like we do
for unmaterialized functions in GetFunction().
Change-Id: I3653f55a1aa063b21cb3a5040f502b4c0ecf82e8
---
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/util/symbols-util-test.cc
3 files changed, 153 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/40/3740/4
--
To view, visit http://gerrit.cloudera.org:8080/3740
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3653f55a1aa063b21cb3a5040f502b4c0ecf82e8
Gerrit-PatchSet: 4
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-CR](cdh5-trunk) IMPALA-3906: Materialize implicitly referenced IR functions
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
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 <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3906: Materialize implicitly referenced IR functions
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-3906: Materialize implicitly referenced IR functions
......................................................................
Patch Set 2: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3740/2/be/src/codegen/llvm-codegen.h
File be/src/codegen/llvm-codegen.h:
Line 436: static bool IsDefinedInImpalad(const std::string fn);
Missing & to pass by reference
--
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 <kw...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3906: Materialize implicitly referenced IR functions
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-3906: Materialize implicitly referenced IR functions
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3740/1/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
PS1, Line 326: "picked"
"merged"
--
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: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3906: Materialize implicitly referenced IR functions
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3906: Materialize implicitly referenced IR functions
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3740
to look at the new patch set (#3).
Change subject: IMPALA-3906: Materialize implicitly referenced IR functions
......................................................................
IMPALA-3906: Materialize implicitly referenced IR functions
With lazy materialization of IR functions in the LLVM module,
there is an assumption that functions not referenced by IR
functions used in the query don't have to be materialized
and can have their linkage types changed to being externally
defined. These unmaterialized functions may be referenced by
global variables in the LLVM module and LLVM resolves these
references to their definitions in the native Impalad binary.
These global variables are mostly arrays containing references
to other global constants or boost and Impala functions included
in the cross-compiled code.
When compiling Impalad with optimization, gcc may actually
inline some of the functions which the global variables in
the LLVM modules reference and LLVM may have linking error
if these referenced IR functions are not materialized as it
can no longer find their definitions in the Impalad binary.
This patch fixes the problem by parsing all the global variables
in the LLVM module during Impalad's initialization and recording
all the referenced functions which aren't defined in the Impalad
binary and make sure they are always materialized.
A second problem which this patch fixes is that linking in
external LLVM module (e.g. UDF IR created by a user) may
implicitly materialize some functions in the external module.
Normally, we would expect functions to be materialized
through the GetFunction() interface and LinkModule() seems
to be an exception. This patch updates LinkModule() to also
parse the functions from the external module just like we do
for unmaterialized functions in GetFunction().
Change-Id: I3653f55a1aa063b21cb3a5040f502b4c0ecf82e8
---
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/util/symbols-util-test.cc
3 files changed, 127 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/40/3740/3
--
To view, visit http://gerrit.cloudera.org:8080/3740
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3653f55a1aa063b21cb3a5040f502b4c0ecf82e8
Gerrit-PatchSet: 3
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-CR](cdh5-trunk) IMPALA-3906: Materialize implicitly referenced IR functions
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-3906: Materialize implicitly referenced IR functions
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/3740/4/be/src/codegen/llvm-codegen.h
File be/src/codegen/llvm-codegen.h:
PS4, Line 553: referencd
> referenced
Done
PS4, Line 554: so LLVM
: /// cannot resolve references to them if they are not materialized.
> this now seems redundant with the last sentence. delete.
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: 4
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3906: Materialize implicitly referenced IR functions
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-3906: Materialize implicitly referenced IR functions
......................................................................
Patch Set 4: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/3740/4/be/src/codegen/llvm-codegen.h
File be/src/codegen/llvm-codegen.h:
PS4, Line 553: referencd
referenced
PS4, Line 554: so LLVM
: /// cannot resolve references to them if they are not materialized.
this now seems redundant with the last sentence. delete.
--
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: 4
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes