impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] Reduce LLVM module's preparation time by lazily creating the IRFunction::Type to LLVM::Function* mappings.
Date Wed, 12 Oct 2016 06:55:11 GMT
Michael Ho has posted comments on this change.

Change subject: Reduce LLVM module's preparation time by lazily creating the IRFunction::Type
to LLVM::Function* mappings.
......................................................................


Patch Set 1:

(4 comments)

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

Line 206:   for (int i = IRFunction::FN_START; i < IRFunction::FN_END; ++i) {
> Maybe should be a new function like ValidateFunctionMappings()?
IMHO, this seems pretty readable  and self-contained without factoring it into a separate
function. Will add some comments though.


Line 209:     if (init_codegen->module_->getFunction(fn_name) == NULL) {
> We shouldn't hit this unless we messed up the build or the function names r
Or if the in memory copy of the bitcode is corrupted somehow. That said, it can get corrupted
any time since this check and GetFunction() will return NULL if the bitcode is corrupted somehow
so may be okay to keep it as a DCHECK.


PS1, Line 407: StringRef
> Should be able to construct the StringRef from the std::string directly wit
Done


Line 668:     StringRef fn_name(FN_MAPPINGS[ir_type].fn_name.c_str());
> This conversion shouldn't be necessary either.
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/4691
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I61ab9fa8cca5a0909bb716c3c62819da3e3b3041
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message