Mailing-List: contact dev-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@impala.incubator.apache.org
Message-Id: <201607140439.u6E4dZRT018126@ip-10-146-233-104.ec2.internal>
Date: Thu, 14 Jul 2016 04:39:35 +0000
From: "Dan Hecht (Code Review)" <gerrit@cloudera.org>
To: Michael Ho <kwho@cloudera.com>, impala-cr@cloudera.com,
        dev@impala.incubator.apache.org
CC: Mostafa Mokhtar <mmokhtar@cloudera.com>,
        Tim Armstrong <tarmstrong@cloudera.com>
Reply-To: dhecht@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-CR=5D=28cdh5-trunk=29_IMPALA-3674=3A_Lazy_materialization_of_LLVM_module_bitcode=2E=0A?=
In-Reply-To: <gerrit.1464213733000.I6ed7862fc5e86005ecea83fa2ceb489e737d66b2@gerrit.cloudera.org>
References: <gerrit.1464213733000.I6ed7862fc5e86005ecea83fa2ceb489e737d66b2@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.2
archived-at: Thu, 14 Jul 2016 04:39:43 -0000

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