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