Dan Hecht has posted comments on this change. Change subject: IMPALA-3674: Lazy materialization of LLVM module bitcode. ......................................................................
Patch Set 8: (23 comments) http://gerrit.cloudera.org:8080/#/c/3220/8/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: PS8, Line 173: sse SSE4.2 PS8, Line 174: sse SSE4.2 PS8, Line 175: instructions are run or not I think we should remove the parenthesis portion of this comment because we compile with -msse42 so these instructions may very well be run. Anyway, don't we eliminate the runtime SSE4.2 checks in codegen, or are we not doing that yet? (We should if we're not already but not in this change of course.) Line 547: return Status(err_msg.str()); Might be cleaner to use Substitute() but okay to ignore if you like this better. PS8, Line 552: !fn->empty() functions don't always have at least a 'return' instruction? or is that implicit in IR? And is this not redundant with iter != inst_end(fn) check below, or does inst_begin() fail if empty() is true? Line 583: DCHECK(!fn->empty()); why is line 552 needed, if this is true? Line 742: Status status = MaterializeFunctionAndCallee(fn); why is this needed? aren't all functions now retrieved by GetFunction() and therefore already materialized? PS8, Line 784: personality what is a personality function? http://gerrit.cloudera.org:8080/#/c/3220/8/be/src/codegen/llvm-codegen.h File be/src/codegen/llvm-codegen.h: Line 109: /// TODO: confirm that the multi-threaded usage is correct let's add a comment explaining materialization of functions. something like: llvm::Function objects can be materialized or not. Materialized functions have instruction IR already attached to them (and all IR functions reachable by this function must be materialized. Functions that aren't yet materialized do not have instruction IR, and will need to be materialized before code generation. Materialization occurs lazily as to save the cost of parsing the IR for functions that will be eliminated as dead code prior to code generation. Line 231: llvm::Module* module() { return module_; } can this be private now that we have GetFunction(string&)? PS8, Line 326: fully materialized materialized recursively (or make it consistent with the comment above in some other way). PS8, Line 327: isn't supported The function isn't cloned. PS8, Line 435: fully delete (let's just use the terminology that a function is "materialized" and "not materialized"). PS8, Line 441: fully delete PS8, Line 449: (i.e. function bodies aren't parsed) delete PS8, Line 509: Invokes MaterializeFunctionHelper() : /// which calls LLVM to parse the bitcode of the function and creates LLVM data : /// structures. Also recursively materializes callees of this function. Let's just simplify to: Calls into MaterializeFunctionHelper() to do the work. (since this function doesn't actually recurse like the comment implies). PS8, Line 513: MaterializeFunctionAndCallee make it plural "callees". Or we can go back to MaterializeFunction() with the addition of that class comment suggested above that clarifies the terminology "materialized" so that a materialized function implies all callees in IR are also materialized. PS8, Line 515: by calling materializeAll() on 'module' Rather than saying what the implementation does, let's just clearly define what a "materialized module" is. PS8, Line 515: Materialize the given module what does it mean for a module to be materialized? That it has function objects, or that the functions themselves are also materialized? PS8, Line 519: fully delete PS8, Line 520: are "must be" i.e. they aren't dead because they are not materialized. they are not materialized because they are dead code, right? PS8, Line 522: they may as well be dead code as IR the native version can be used instead, and the IR version is dead code. Is that right? PS8, Line 524: DCE may complain if the above are not done doesn't that also have to happen so that the native functions are linked against? or would the linking happen even without external linkage? -- 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: 8 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
