Michael Ho has posted comments on this change. Change subject: IMPALA-3674: Lazy materialization of LLVM module bitcode. ......................................................................
Patch Set 11: (4 comments) http://gerrit.cloudera.org:8080/#/c/3220/11/be/src/codegen/llvm-codegen-test.cc File be/src/codegen/llvm-codegen-test.cc: Line 162: const char* loop_name = "_Z8TestLoopi"; > just make these "const string" and then you can get rid of the string() on Done http://gerrit.cloudera.org:8080/#/c/3220/11/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: Line 536: } > is this function dead code? Yes, looks like it's dead code. Removed. http://gerrit.cloudera.org:8080/#/c/3220/11/be/src/codegen/llvm-codegen.h File be/src/codegen/llvm-codegen.h: Line 199: /// externally defined native function. > thanks, that's a lot clearer. but then I wonder why can't we just use 'bui It appears that Expr::CreateIrFunctionPrototype() will call this function with builder == NULL to create the prototype for hand-crafted expr IR. It appears that some callers (e.g. case expr) of that function has context specific logic for determining the entry point for the IR function so the default behavior of setting the IR's entry point when builder != NULL may not be applicable for those callers. However, we still want to print the IR of those hand-crafted functions if requested. Given we already overloaded 'builder != NULL' for the purpose stated above, it may be clearer to add an extra parameter to indicate whether to print the IR or not in GetIR(). http://gerrit.cloudera.org:8080/#/c/3220/11/be/src/exprs/scalar-fn-call.cc File be/src/exprs/scalar-fn-call.cc: PS11, Line 460: len > shouldn't this say varargs, not "var len"? Done -- To view, visit http://gerrit.cloudera.org:8080/3220 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6ed7862fc5e86005ecea83fa2ceb489e737d66b2 Gerrit-PatchSet: 11 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: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
