Michael Ho 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 Done PS8, Line 174: sse > SSE4.2 Done PS8, Line 175: instructions are run or not > I think we should remove the parenthesis portion of this comment because we Done. I think Tim's new patch for constant replacement is removing the checks at runtime. Line 547: return Status(err_msg.str()); > Might be cleaner to use Substitute() but okay to ignore if you like this be Done PS8, Line 552: !fn->empty() > functions don't always have at least a 'return' instruction? or is that imp Good point. The iterator should be able to handle empty functions too. Line 583: DCHECK(!fn->empty()); > why is line 552 needed, if this is true? Good point. Removed. Line 742: Status status = MaterializeFunctionAndCallee(fn); > why is this needed? aren't all functions now retrieved by GetFunction() and Don't think it's a good idea to assume callers have already called GetFunction(). PS8, Line 784: personality > what is a personality function? Quote from the LLVM doc: Because different programming languages have different behaviors when handling exceptions, the exception handling ABI provides a mechanism for supplying personalities. An exception handling personality is defined by way of a personality function (e.g. __gxx_personality_v0 in C++), which receives the context of the exception, an exception structure containing the exception object type and value, and a reference to the exception table for the current function. The personality function for the current compile unit is specified in a common exception frame. 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 Done Line 231: llvm::Module* module() { return module_; } > can this be private now that we have GetFunction(string&)? This is still called in ScalarFnCall::GetUdf(). PS8, Line 326: fully materialized > materialized recursively (or make it consistent with the comment above in Done PS8, Line 327: isn't supported > The function isn't cloned. Done PS8, Line 435: fully > delete (let's just use the terminology that a function is "materialized" an Done PS8, Line 441: fully > delete Done PS8, Line 449: (i.e. function bodies aren't parsed) > delete Done 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: Done PS8, Line 513: MaterializeFunctionAndCallee > make it plural "callees". Or we can go back to MaterializeFunction() with Switched to MaterializeFunction(). PS8, Line 515: by calling materializeAll() on 'module' > Rather than saying what the implementation does, let's just clearly define Done PS8, Line 515: Materialize the given module > what does it mean for a module to be materialized? That it has function ob It means all the functions are materialized and the module's materializer is deleted. PS8, Line 519: fully > delete Done PS8, Line 520: are > "must be" Done 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. Yes, that's right. Done 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 ag Yes. Comments updated. -- 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
