impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-3674: Lazy materialization of LLVM module bitcode.
Date Thu, 14 Jul 2016 04:39:35 GMT
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 <kwho@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mmokhtar@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message