[Impala-ASF-CR] IMPALA-6177: Cleanup incomplete handcrafted IRs before finalizing module

2017-12-29 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8541 )

Change subject: IMPALA-6177: Cleanup incomplete handcrafted IRs before 
finalizing module
..


Patch Set 15: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If975cfb3906482b36dd6ede32ca81de6fcee1d7f
Gerrit-Change-Number: 8541
Gerrit-PatchSet: 15
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Sat, 30 Dec 2017 05:31:37 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6177: Cleanup incomplete handcrafted IRs before finalizing module

2017-12-29 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8541 )

Change subject: IMPALA-6177: Cleanup incomplete handcrafted IRs before 
finalizing module
..


Patch Set 15:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1659/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If975cfb3906482b36dd6ede32ca81de6fcee1d7f
Gerrit-Change-Number: 8541
Gerrit-PatchSet: 15
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Sat, 30 Dec 2017 01:54:16 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6177: Cleanup incomplete handcrafted IRs before finalizing module

2017-12-29 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8541 )

Change subject: IMPALA-6177: Cleanup incomplete handcrafted IRs before 
finalizing module
..


Patch Set 14: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/1657/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If975cfb3906482b36dd6ede32ca81de6fcee1d7f
Gerrit-Change-Number: 8541
Gerrit-PatchSet: 14
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Sat, 30 Dec 2017 00:34:24 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6177: Cleanup incomplete handcrafted IRs before finalizing module

2017-12-29 Thread Bikramjeet Vig (Code Review) 
Bikramjeet Vig has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8541 )

Change subject: IMPALA-6177: Cleanup incomplete handcrafted IRs before 
finalizing module
..


Patch Set 14: Code-Review+2

carrying over Michael's +2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If975cfb3906482b36dd6ede32ca81de6fcee1d7f
Gerrit-Change-Number: 8541
Gerrit-PatchSet: 14
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 29 Dec 2017 20:57:27 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6177: Cleanup incomplete handcrafted IRs before finalizing module

2017-12-22 Thread Michael Ho (Code Review) 
Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8541 )

Change subject: IMPALA-6177: Cleanup incomplete handcrafted IRs before 
finalizing module
..


Patch Set 13:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8541/13/be/src/codegen/llvm-codegen.cc@898
PS13, Line 898: ReplaceCallSites
> should i add a dcheck for here to check if the replacement function is fina
Sounds like a good idea.

However, this is also making the assumption that 'new_fn' is a handcrafted IR 
function which seems to be case for all callers now. May want to verify that 
either 'new_fn' is not a handcrafted IR function or that it is finalized 
already.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If975cfb3906482b36dd6ede32ca81de6fcee1d7f
Gerrit-Change-Number: 8541
Gerrit-PatchSet: 13
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 22 Dec 2017 20:26:55 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6177: Cleanup incomplete handcrafted IRs before finalizing module

2017-12-22 Thread Bikramjeet Vig (Code Review) 
Bikramjeet Vig has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8541 )

Change subject: IMPALA-6177: Cleanup incomplete handcrafted IRs before 
finalizing module
..


Patch Set 13:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8541/13/be/src/codegen/llvm-codegen.cc@898
PS13, Line 898: ReplaceCallSites
should i add a dcheck for here to check if the replacement function is 
finalized?
This would prevent us from running into a situation where impala would crash if 
the replacement function is not finalized and gets removed before finalizing 
the module.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If975cfb3906482b36dd6ede32ca81de6fcee1d7f
Gerrit-Change-Number: 8541
Gerrit-PatchSet: 13
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 22 Dec 2017 19:32:08 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6177: Cleanup incomplete handcrafted IRs before finalizing module

2017-12-20 Thread Bikramjeet Vig (Code Review) 
Bikramjeet Vig has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8541 )

Change subject: IMPALA-6177: Cleanup incomplete handcrafted IRs before 
finalizing module
..


Patch Set 13: Code-Review+2

(2 comments)

Carrying over Michael's +2
will run core tests on local with LLVM 5 before running GVO on this.

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

http://gerrit.cloudera.org:8080/#/c/8541/12/be/src/codegen/llvm-codegen.cc@859
PS12, Line 859: nullptr, n
> nullptr, nullptr
Done


http://gerrit.cloudera.org:8080/#/c/8541/12/be/src/codegen/llvm-codegen.cc@1082
PS12, Line 1082: fn->eraseFromParent();
> Feel free to ignore but do you think it will be sufficient to just print th
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If975cfb3906482b36dd6ede32ca81de6fcee1d7f
Gerrit-Change-Number: 8541
Gerrit-PatchSet: 13
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 20 Dec 2017 20:17:16 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6177: Cleanup incomplete handcrafted IRs before finalizing module

2017-12-20 Thread Bikramjeet Vig (Code Review) 
Hello Michael Ho, Tim Armstrong,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8541

to look at the new patch set (#13).

Change subject: IMPALA-6177: Cleanup incomplete handcrafted IRs before 
finalizing module
..

IMPALA-6177: Cleanup incomplete handcrafted IRs before finalizing module

Currently, if an error is encountered during the creation of a
handcrafted codegen method, then the resulting IR is left in an
incomplete state. This patch ensures that all such IRs are cleaned up
(method is deleted from the module) before the llvm module is finalized.

Testing:
- added a backend test to exercise the added code path.
- tested manually by executing the following query:
select * from charTable A, charTable B
where A.charColumn = B.charColumn and A.charColumn = 'foo';
and looking at the logs to verify that 'InsertRuntimeFilters' and
'FilterContextInsert' methods have been removed.

Change-Id: If975cfb3906482b36dd6ede32ca81de6fcee1d7f
---
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/hdfs-text-scanner.cc
4 files changed, 99 insertions(+), 35 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/41/8541/13
--
To view, visit http://gerrit.cloudera.org:8080/8541
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If975cfb3906482b36dd6ede32ca81de6fcee1d7f
Gerrit-Change-Number: 8541
Gerrit-PatchSet: 13
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-6177: Cleanup incomplete handcrafted IRs before finalizing module

2017-12-19 Thread Michael Ho (Code Review) 
Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8541 )

Change subject: IMPALA-6177: Cleanup incomplete handcrafted IRs before 
finalizing module
..


Patch Set 12: Code-Review+2

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/8541/12/be/src/codegen/llvm-codegen.cc@859
PS12, Line 859: NULL, NULL
nullptr, nullptr


http://gerrit.cloudera.org:8080/#/c/8541/12/be/src/codegen/llvm-codegen.cc@1082
PS12, Line 1082: fn->print(stream, nullptr, false, true);
Feel free to ignore but do you think it will be sufficient to just print the 
function name ? I am a bit worried about potential log spew if we print the 
function body when we run queries with types (e.g. char) which we will always 
not codegen for.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If975cfb3906482b36dd6ede32ca81de6fcee1d7f
Gerrit-Change-Number: 8541
Gerrit-PatchSet: 12
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 19 Dec 2017 21:58:46 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6177: Cleanup incomplete handcrafted IRs before finalizing module

2017-12-19 Thread Michael Ho (Code Review) 
Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8541 )

Change subject: IMPALA-6177: Cleanup incomplete handcrafted IRs before 
finalizing module
..


Patch Set 11:

(7 comments)

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

http://gerrit.cloudera.org:8080/#/c/8541/11/be/src/codegen/llvm-codegen-test.cc@111
PS11, Line 111: for(auto fn : codegen->handcrafted_functions_){
  :   if(fn == function) return true;
  : }
  : return false;
const auto& hf = codegen->handcrafted_functions_;
return find(hf.begin(), hf.end(), function) != hf.end();


http://gerrit.cloudera.org:8080/#/c/8541/11/be/src/codegen/llvm-codegen-test.cc@498
PS11, Line 498: NULL
nullptr


http://gerrit.cloudera.org:8080/#/c/8541/11/be/src/codegen/llvm-codegen-test.cc@506
PS11, Line 506: NULL
nullptr


http://gerrit.cloudera.org:8080/#/c/8541/11/be/src/codegen/llvm-codegen-test.cc@507
PS11, Line 507: NULL
nullptr


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

http://gerrit.cloudera.org:8080/#/c/8541/11/be/src/codegen/llvm-codegen.h@228
PS11, Line 228: NULL
nullptr


http://gerrit.cloudera.org:8080/#/c/8541/11/be/src/codegen/llvm-codegen.h@228
PS11, Line 228: NULL
nullptr


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

http://gerrit.cloudera.org:8080/#/c/8541/11/be/src/codegen/llvm-codegen.cc@182
PS11, Line 182:  NULL
nullptr



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If975cfb3906482b36dd6ede32ca81de6fcee1d7f
Gerrit-Change-Number: 8541
Gerrit-PatchSet: 11
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 19 Dec 2017 20:42:49 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6177: Cleanup incomplete handcrafted IRs before finalizing module

2017-12-14 Thread Tim Armstrong (Code Review) 
Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8541 )

Change subject: IMPALA-6177: Cleanup incomplete handcrafted IRs before 
finalizing module
..


Patch Set 10: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If975cfb3906482b36dd6ede32ca81de6fcee1d7f
Gerrit-Change-Number: 8541
Gerrit-PatchSet: 10
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 14 Dec 2017 22:38:44 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6177: Cleanup incomplete handcrafted IRs before finalizing module

2017-12-12 Thread Bikramjeet Vig (Code Review) 
Hello Michael Ho, Tim Armstrong,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8541

to look at the new patch set (#10).

Change subject: IMPALA-6177: Cleanup incomplete handcrafted IRs before 
finalizing module
..

IMPALA-6177: Cleanup incomplete handcrafted IRs before finalizing module

Currently, if an error is encountered during the creation of a
handcrafted codegen method, then the resulting IR is left in an
incomplete state. This patch ensures that all such IRs are cleaned up
(method body is truncated) before the llvm module is finalized.

Tested manually as follows:
- move the cleanup instructions before the instructions for printing
the unoptimized llvm module in LlvmCodeGen::FinalizeModule()
- run the following query:
select * from charTable A, charTable B
where A.charColumn = B.charColumn and A.charColumn = 'foo'
- inspect the dump of the unoptimized module and make sure the methods
'InsertRuntimeFilters' and 'FilterContextInsert' are not defined

Change-Id: If975cfb3906482b36dd6ede32ca81de6fcee1d7f
---
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/hdfs-text-scanner.cc
4 files changed, 58 insertions(+), 32 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/41/8541/10
--
To view, visit http://gerrit.cloudera.org:8080/8541
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If975cfb3906482b36dd6ede32ca81de6fcee1d7f
Gerrit-Change-Number: 8541
Gerrit-PatchSet: 10
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-6177: Cleanup incomplete handcrafted IRs before finalizing module

2017-12-12 Thread Tim Armstrong (Code Review) 
Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8541 )

Change subject: IMPALA-6177: Cleanup incomplete handcrafted IRs before 
finalizing module
..


Patch Set 9: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8541/8/be/src/codegen/llvm-codegen-test.cc
File be/src/codegen/llvm-codegen-test.cc:

http://gerrit.cloudera.org:8080/#/c/8541/8/be/src/codegen/llvm-codegen-test.cc@167
PS8, Line 167: }
> that would crash llvm(and hence the process) during FinalizeModule
We could use the magic IMPALA_ASSERT_DEBUG_DEATH() macro if we want to test 
that code path. It might not be worth testing if it's an invalid use of the API.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If975cfb3906482b36dd6ede32ca81de6fcee1d7f
Gerrit-Change-Number: 8541
Gerrit-PatchSet: 9
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 13 Dec 2017 00:01:24 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6177: Cleanup incomplete handcrafted IRs before finalizing module

2017-12-12 Thread Bikramjeet Vig (Code Review) 
Hello Michael Ho, Tim Armstrong,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8541

to look at the new patch set (#9).

Change subject: IMPALA-6177: Cleanup incomplete handcrafted IRs before 
finalizing module
..

IMPALA-6177: Cleanup incomplete handcrafted IRs before finalizing module

Currently, if an error is encountered during the creation of a
handcrafted codegen method, then the resulting IR is left in an
incomplete state. This patch ensures that all such IRs are cleaned up
(method body is truncated) before the llvm module is finalized.

Testing:
Tested manually by executing the following query and inspecting
the dump of the optimized llvm module.

Change-Id: If975cfb3906482b36dd6ede32ca81de6fcee1d7f
---
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/hdfs-text-scanner.cc
4 files changed, 58 insertions(+), 32 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/41/8541/9
--
To view, visit http://gerrit.cloudera.org:8080/8541
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If975cfb3906482b36dd6ede32ca81de6fcee1d7f
Gerrit-Change-Number: 8541
Gerrit-PatchSet: 9
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-6177: Cleanup incomplete handcrafted IRs before finalizing module

2017-12-12 Thread Bikramjeet Vig (Code Review) 
Bikramjeet Vig has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8541 )

Change subject: IMPALA-6177: Cleanup incomplete handcrafted IRs before 
finalizing module
..


Patch Set 8:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/8541/8/be/src/codegen/llvm-codegen-test.cc
File be/src/codegen/llvm-codegen-test.cc:

http://gerrit.cloudera.org:8080/#/c/8541/8/be/src/codegen/llvm-codegen-test.cc@167
PS8, Line 167: }
> Can you please also add a test case in which we don't finalize a handcrafte
that would crash llvm(and hence the process) during FinalizeModule


http://gerrit.cloudera.org:8080/#/c/8541/8/be/src/codegen/llvm-codegen-test.cc@327
PS8, Line 327:   EXPECT_TRUE(VerifyFunction(codegen.get(), string_test_fn));
> Does this become unnecessary given you called FinalizeFunction() in Codegen
yes, removed


http://gerrit.cloudera.org:8080/#/c/8541/8/be/src/codegen/llvm-codegen-test.cc@330
PS8, Line 330:   void* jitted_fn = NULL;
 :   AddFunctionToJit(codegen.get(), string_test_fn, _fn);
> Same here.
This is necessary as we need to explicitly add it to the list of JIT functions 
as FinalizeFunction does not do that


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

http://gerrit.cloudera.org:8080/#/c/8541/8/be/src/codegen/llvm-codegen.h@107
PS8, Line 107: build
> nit: built
Done


http://gerrit.cloudera.org:8080/#/c/8541/8/be/src/codegen/llvm-codegen.h@109
PS8, Line 109: Loaded
> Cross-compiled
Done


http://gerrit.cloudera.org:8080/#/c/8541/8/be/src/codegen/llvm-codegen.h@219
PS8, Line 219: should
> must
Done


http://gerrit.cloudera.org:8080/#/c/8541/8/be/src/codegen/llvm-codegen.h@363
PS8, Line 363:   /// FinalizeFunction() on the returned function as it is 
already materialized.
> I find this last statement potentially confusing. Most of the time, we call
Done


http://gerrit.cloudera.org:8080/#/c/8541/8/be/src/codegen/llvm-codegen.h@385
PS8, Line 385:   /// this method.
> Shouldn't there be a DCHECK in this function to enforce this ?
Done


http://gerrit.cloudera.org:8080/#/c/8541/8/be/src/codegen/llvm-codegen.h@765
PS8, Line 765: generated
> generated from scratch
Done


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

http://gerrit.cloudera.org:8080/#/c/8541/8/be/src/codegen/llvm-codegen.cc@1062
PS8, Line 1062: deleteBody()
> Why not fn->eraseFromParent() ?
switching to fn->eraseFromParent().

If we remove the unfinalized IR function altogether from the module but still 
use it to replace some call sites in a loaded cross-compiled function, the 
compilation would crash impalad.

I dont currently see anywhere in the codebase where we would use a handcrafted 
IR even if finalize function fails. So, switching to fn->eraseFromParent() 
would work. Also, I ll run core tests again with this patch using llvm 5 after 
I get a +2 but before I run the GVO merge job.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If975cfb3906482b36dd6ede32ca81de6fcee1d7f
Gerrit-Change-Number: 8541
Gerrit-PatchSet: 8
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 12 Dec 2017 23:55:43 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6177: Cleanup incomplete handcrafted IRs before finalizing module

2017-12-07 Thread Michael Ho (Code Review) 
Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8541 )

Change subject: IMPALA-6177: Cleanup incomplete handcrafted IRs before 
finalizing module
..


Patch Set 8:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/8541/8/be/src/codegen/llvm-codegen-test.cc
File be/src/codegen/llvm-codegen-test.cc:

http://gerrit.cloudera.org:8080/#/c/8541/8/be/src/codegen/llvm-codegen-test.cc@167
PS8, Line 167: }
Can you please also add a test case in which we don't finalize a handcrafted IR 
but still use it to replace some call sites in a loaded cross-compiled function 
? Not sure if we can recover from this kind of error ? May be it'll crash the 
test.


http://gerrit.cloudera.org:8080/#/c/8541/8/be/src/codegen/llvm-codegen-test.cc@327
PS8, Line 327:   EXPECT_TRUE(VerifyFunction(codegen.get(), string_test_fn));
Does this become unnecessary given you called FinalizeFunction() in 
CodegenStringTest() ?


http://gerrit.cloudera.org:8080/#/c/8541/8/be/src/codegen/llvm-codegen-test.cc@330
PS8, Line 330:   void* jitted_fn = NULL;
 :   AddFunctionToJit(codegen.get(), string_test_fn, _fn);
Same here.


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

http://gerrit.cloudera.org:8080/#/c/8541/8/be/src/codegen/llvm-codegen.h@107
PS8, Line 107: build
nit: built


http://gerrit.cloudera.org:8080/#/c/8541/8/be/src/codegen/llvm-codegen.h@109
PS8, Line 109: Loaded
Cross-compiled


http://gerrit.cloudera.org:8080/#/c/8541/8/be/src/codegen/llvm-codegen.h@219
PS8, Line 219: should
must

And if FinalizeFunction() is not called on a handcrafted-IR function, it will 
be deleted during FinalizeModule(), right ? Would be great to mention it 
somewhere.


http://gerrit.cloudera.org:8080/#/c/8541/8/be/src/codegen/llvm-codegen.h@363
PS8, Line 363:   /// FinalizeFunction() on the returned function as it is 
already materialized.
I find this last statement potentially confusing. Most of the time, we call 
CloneFunction() or GetFunction() to get a copy of the cross-compiled function 
as template to be customized for a query. And that means we still need to call 
FinalizeFunction() on the customized function. FinalizeFunction() shouldn't 
have anything to do with bitcode materialization, right ?


http://gerrit.cloudera.org:8080/#/c/8541/8/be/src/codegen/llvm-codegen.h@385
PS8, Line 385:   /// this method.
Shouldn't there be a DCHECK in this function to enforce this ?


http://gerrit.cloudera.org:8080/#/c/8541/8/be/src/codegen/llvm-codegen.h@765
PS8, Line 765: generated
generated from scratch


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

http://gerrit.cloudera.org:8080/#/c/8541/8/be/src/codegen/llvm-codegen.cc@1062
PS8, Line 1062: deleteBody()
Why not fn->eraseFromParent() ?

In theory, if there is a bug in some codegen function and we didn't finalize a 
handcrafted IR but still use it to replace some call sites in a loaded 
cross-compiled function, things may still work if just delete the function body 
but that may generate wrong result, right ? If we remove the unfinalized IR 
function altogether from the module, the compilation should fail, right ?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If975cfb3906482b36dd6ede32ca81de6fcee1d7f
Gerrit-Change-Number: 8541
Gerrit-PatchSet: 8
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 07 Dec 2017 23:28:18 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6177: Cleanup incomplete handcrafted IRs before finalizing module

2017-12-07 Thread Bikramjeet Vig (Code Review) 
Bikramjeet Vig has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8541


Change subject: IMPALA-6177: Cleanup incomplete handcrafted IRs before 
finalizing module
..

IMPALA-6177: Cleanup incomplete handcrafted IRs before finalizing module

Currently, if an error is encountered during the creation of a
handcrafted codegen method, then the resulting IR is left in an
incomplete state. This patch ensures that all such IRs are cleaned up
(method body is truncated) before the llvm module is finalized.

Testing:
Tested manually by executing the following query and inspecting
the dump of the optimized llvm module.

Change-Id: If975cfb3906482b36dd6ede32ca81de6fcee1d7f
---
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/hdfs-text-scanner.cc
4 files changed, 50 insertions(+), 24 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/41/8541/8
--
To view, visit http://gerrit.cloudera.org:8080/8541
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: If975cfb3906482b36dd6ede32ca81de6fcee1d7f
Gerrit-Change-Number: 8541
Gerrit-PatchSet: 8
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong