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-775,IMPALA-3374: Upgrade LLVM to 3.8.0
Date Tue, 19 Apr 2016 00:48:54 GMT
Dan Hecht has posted comments on this change.

Change subject: IMPALA-775,IMPALA-3374: Upgrade LLVM to 3.8.0
......................................................................


Patch Set 11: Code-Review+2

(4 comments)

Looks good to me. Please let Michael finish his review as well.

http://gerrit.cloudera.org:8080/#/c/2486/11/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:

Line 545:     return false;
now that the DCHECK is gone, will we still notice during testing when this thing returning
false?  Maybe we just rely on perf testing for that, since as you pointed out, a malformed
UDF IR file could trigger this path (and so we can't dcheck).


http://gerrit.cloudera.org:8080/#/c/2486/11/be/src/codegen/llvm-codegen.h
File be/src/codegen/llvm-codegen.h:

Line 292: VerifyFunction
can this be private?


http://gerrit.cloudera.org:8080/#/c/2486/11/be/src/exprs/scalar-fn-call.cc
File be/src/exprs/scalar-fn-call.cc:

Line 391:     return Status("Function codegen failed verification: see error log for details.");
maybe put the udf name in the error?


http://gerrit.cloudera.org:8080/#/c/2486/11/bin/impala-config.sh
File bin/impala-config.sh:

Line 255:   # Debug builds should use the release+asserts build to get additional coverage.
briefly explain why not using llvm debug build.


-- 
To view, visit http://gerrit.cloudera.org:8080/2486
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I17d7afd05ad3b472a0bfe035bfc3daada5597b2d
Gerrit-PatchSet: 11
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Skye Wanderman-Milne <skye@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message