Dan Hecht has posted comments on this change. Change subject: IMPALA-3674: Lazy materialization of LLVM module bitcode. ......................................................................
Patch Set 9: (2 comments) http://gerrit.cloudera.org:8080/#/c/3220/8/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: Line 742: module_->getFunctionList().push_back(fn_clone); > Don't think it's a good idea to assume callers have already called GetFunct Why? Doesn't impala code assume that any llvm Function object it gets a hold of is already materialized (so that it can, for example, traverse the IR and do things like replace calls)? Or is there some code that wouldn't care if it's materialized? It seems best, if it's possible, to keep the materizalization detail entirely within this module so that no other code has to think about it. And that would mean that all Function * that other code operations on must be materialized. Or is there some case I'm not considering? Also, couldn't we just make it a requirement that the only way the rest of impala can get a Function is via GetFunction() to enforce this? Again, let me know if I'm missing something, but it still seems to me this should be a DCHECK that fn is materialized already. http://gerrit.cloudera.org:8080/#/c/3220/8/be/src/codegen/llvm-codegen.h File be/src/codegen/llvm-codegen.h: Line 231: > This is still called in ScalarFnCall::GetUdf(). Would it make sense to use Codegen::FnPrototype there? It would be nice to not expose module(). If FnPrototype doesn't work, we can leave this for now. -- 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: 9 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
