[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20399 ) Change subject: IMPALA-5081: Add codegen_opt_level query option .. Patch Set 14: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 14 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu Gerrit-Comment-Date: Mon, 23 Oct 2023 21:11:46 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/20399 ) Change subject: IMPALA-5081: Add codegen_opt_level query option .. IMPALA-5081: Add codegen_opt_level query option Adds the 'codegen_opt_level' query option to select LLVM optimization level for generated code. Retains the prior behavior - O2 - as default. If optimization level is changed for an entry already in cache, the cache entry will be used unless the new optimization level is higher than the cached level. Adds additional counters for NumOptimizedFunctions and NumOptimizedInstructions, which allow observing some impacts from codegen optimization. These additional counters, and tracking opt level for cached entries, increases the size of each cached entry. Adds unit tests for all optimizition levels checking - that small functions are inlined at higher levels (as a way to verify that optimization level has an effect) - codegen cache entries are updated when optimizing the same fragment at a higher level, and not updated the rest of the time Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Reviewed-on: http://gerrit.cloudera.org:8080/20399 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M be/src/codegen/CMakeLists.txt M be/src/codegen/llvm-codegen-cache-test.cc M be/src/codegen/llvm-codegen-cache.cc M be/src/codegen/llvm-codegen-cache.h 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/service/query-options.cc M be/src/service/query-options.h M common/thrift/ImpalaService.thrift M common/thrift/Query.thrift A testdata/llvm/test-opt.cc 12 files changed, 306 insertions(+), 39 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 15 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20399 ) Change subject: IMPALA-5081: Add codegen_opt_level query option .. Patch Set 13: (1 comment) http://gerrit.cloudera.org:8080/#/c/20399/11//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20399/11//COMMIT_MSG@18 PS11, Line 18: codegen optimization. These additional counters, and tracking opt level > Yeah, we should avoid the exponential effect of adding new parameters to th I did a manual exhaustive run adding just O0 and O3 that passed. It took 1h52m for test_queries::TestQueries. -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 13 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu Gerrit-Comment-Date: Mon, 23 Oct 2023 16:51:07 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20399 ) Change subject: IMPALA-5081: Add codegen_opt_level query option .. Patch Set 14: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 14 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu Gerrit-Comment-Date: Mon, 23 Oct 2023 16:51:21 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20399 ) Change subject: IMPALA-5081: Add codegen_opt_level query option .. Patch Set 14: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9828/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 14 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu Gerrit-Comment-Date: Mon, 23 Oct 2023 16:51:22 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/20399 ) Change subject: IMPALA-5081: Add codegen_opt_level query option .. Patch Set 13: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/20399/11//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20399/11//COMMIT_MSG@18 PS11, Line 18: codegen optimization. These additional counters, and tracking opt level > test_queries.py::TestQueries in exhaustive mode takes 36 minutes. Adding te Yeah, we should avoid the exponential effect of adding new parameters to the vector, e.g. it doesn't seem that useful to run tests for each file format X each optimization level. -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 13 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu Gerrit-Comment-Date: Fri, 20 Oct 2023 08:01:13 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20399 ) Change subject: IMPALA-5081: Add codegen_opt_level query option .. Patch Set 13: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/14209/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 13 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu Gerrit-Comment-Date: Thu, 19 Oct 2023 19:27:32 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Hello Daniel Becker, Yida Wu, Noemi Pap-Takacs, Csaba Ringhofer, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20399 to look at the new patch set (#13). Change subject: IMPALA-5081: Add codegen_opt_level query option .. IMPALA-5081: Add codegen_opt_level query option Adds the 'codegen_opt_level' query option to select LLVM optimization level for generated code. Retains the prior behavior - O2 - as default. If optimization level is changed for an entry already in cache, the cache entry will be used unless the new optimization level is higher than the cached level. Adds additional counters for NumOptimizedFunctions and NumOptimizedInstructions, which allow observing some impacts from codegen optimization. These additional counters, and tracking opt level for cached entries, increases the size of each cached entry. Adds unit tests for all optimizition levels checking - that small functions are inlined at higher levels (as a way to verify that optimization level has an effect) - codegen cache entries are updated when optimizing the same fragment at a higher level, and not updated the rest of the time Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd --- M be/src/codegen/CMakeLists.txt M be/src/codegen/llvm-codegen-cache-test.cc M be/src/codegen/llvm-codegen-cache.cc M be/src/codegen/llvm-codegen-cache.h 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/service/query-options.cc M be/src/service/query-options.h M common/thrift/ImpalaService.thrift M common/thrift/Query.thrift A testdata/llvm/test-opt.cc 12 files changed, 306 insertions(+), 39 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/99/20399/13 -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 13 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20399 ) Change subject: IMPALA-5081: Add codegen_opt_level query option .. Patch Set 12: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/14208/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 12 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu Gerrit-Comment-Date: Thu, 19 Oct 2023 17:49:07 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Michael Smith has posted comments on this change. (
http://gerrit.cloudera.org:8080/20399 )
Change subject: IMPALA-5081: Add codegen_opt_level query option
..
Patch Set 11:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/20399/11//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20399/11//COMMIT_MSG@16
PS11, Line 16: that levels besides
> this is not clear to me - maybe "with levels besides"?
I can see how that's hard to read. I've tried to clarify it.
http://gerrit.cloudera.org:8080/#/c/20399/11//COMMIT_MSG@18
PS11, Line 18:
> I don't expect any issues, but it would be nice to use the new optimization
test_queries.py::TestQueries in exhaustive mode takes 36 minutes. Adding
testing of all 5 optimization levels would make that ~3 hours. It would be
useful to have more coverage somehow.
http://gerrit.cloudera.org:8080/#/c/20399/11/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/20399/11/be/src/codegen/llvm-codegen.cc@1341
PS11, Line 1341: COUNTER_SET(num_opt_functions_,
: counter.GetCount(InstructionCounter::TOTAL_FUNCTIONS));
: COUNTER_SET(num_opt_instructions_,
counter.GetCount(InstructionCounter::TOTAL_INSTS));
: } else {
: COUNTER_SET(num_opt_functions_,
: counter.GetCount(InstructionCounter::TOTAL_FUNCTIONS));
: COUNTER_SET(num_opt_instructions_,
counter.GetCount(InstructionCounter::TOTAL_INSTS));
: }
> the COUNTER_SET part seems to be the same - shouldn't we do it after the if
Oh yup, done.
--
To view, visit http://gerrit.cloudera.org:8080/20399
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd
Gerrit-Change-Number: 20399
Gerrit-PatchSet: 11
Gerrit-Owner: Michael Smith
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Daniel Becker
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Smith
Gerrit-Reviewer: Noemi Pap-Takacs
Gerrit-Reviewer: Yida Wu
Gerrit-Comment-Date: Thu, 19 Oct 2023 17:18:22 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Hello Daniel Becker, Yida Wu, Noemi Pap-Takacs, Csaba Ringhofer, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20399 to look at the new patch set (#12). Change subject: IMPALA-5081: Add codegen_opt_level query option .. IMPALA-5081: Add codegen_opt_level query option Adds the 'codegen_opt_level' query option to select LLVM optimization level for generated code. Retains the prior behavior - O2 - as default. If optimization level is changed for an entry already in cache, the cache entry will be used unless the new optimization level is higher than the cached level. Adds additional counters for NumOptimizedFunctions and NumOptimizedInstructions, which allow observing some impacts from codegen optimization. These additional counters, and tracking opt level for cached entries, increases the size of each cached entry. Adds unit tests for all optimizition levels checking - that small functions are inlined at higher levels (as a way to verify that optimization level has an effect) - codegen cache entries are updated when optimizing the same fragment at a higher level, and not updated the rest of the time Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd --- M be/src/codegen/CMakeLists.txt M be/src/codegen/llvm-codegen-cache-test.cc M be/src/codegen/llvm-codegen-cache.cc M be/src/codegen/llvm-codegen-cache.h 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/service/query-options.cc M be/src/service/query-options.h M common/thrift/ImpalaService.thrift M common/thrift/Query.thrift A testdata/llvm/test-opt.cc 12 files changed, 306 insertions(+), 39 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/99/20399/12 -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 12 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Csaba Ringhofer has posted comments on this change. (
http://gerrit.cloudera.org:8080/20399 )
Change subject: IMPALA-5081: Add codegen_opt_level query option
..
Patch Set 11: Code-Review+1
(3 comments)
looks great!
http://gerrit.cloudera.org:8080/#/c/20399/11//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20399/11//COMMIT_MSG@16
PS11, Line 16: that levels besides
this is not clear to me - maybe "with levels besides"?
http://gerrit.cloudera.org:8080/#/c/20399/11//COMMIT_MSG@18
PS11, Line 18:
I don't expect any issues, but it would be nice to use the new optimization
levels in actual query tests, e.g. by adding a dimension to test_queries.py
similarly to ASYNC_CODEGEN
https://github.com/apache/impala/blob/75797e8ec146d6118fabff27397e2e8dbcd2ebdd/tests/query_test/test_queries.py#L54
I am ok with not doing this in the current review.
http://gerrit.cloudera.org:8080/#/c/20399/11/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/20399/11/be/src/codegen/llvm-codegen.cc@1341
PS11, Line 1341: COUNTER_SET(num_opt_functions_,
: counter.GetCount(InstructionCounter::TOTAL_FUNCTIONS));
: COUNTER_SET(num_opt_instructions_,
counter.GetCount(InstructionCounter::TOTAL_INSTS));
: } else {
: COUNTER_SET(num_opt_functions_,
: counter.GetCount(InstructionCounter::TOTAL_FUNCTIONS));
: COUNTER_SET(num_opt_instructions_,
counter.GetCount(InstructionCounter::TOTAL_INSTS));
: }
the COUNTER_SET part seems to be the same - shouldn't we do it after the if
block?
--
To view, visit http://gerrit.cloudera.org:8080/20399
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd
Gerrit-Change-Number: 20399
Gerrit-PatchSet: 11
Gerrit-Owner: Michael Smith
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Daniel Becker
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Smith
Gerrit-Reviewer: Noemi Pap-Takacs
Gerrit-Reviewer: Yida Wu
Gerrit-Comment-Date: Thu, 19 Oct 2023 12:23:11 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/20399 ) Change subject: IMPALA-5081: Add codegen_opt_level query option .. Patch Set 11: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 11 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu Gerrit-Comment-Date: Wed, 18 Oct 2023 10:17:49 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20399 ) Change subject: IMPALA-5081: Add codegen_opt_level query option .. Patch Set 11: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/14194/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 11 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu Gerrit-Comment-Date: Tue, 17 Oct 2023 17:00:21 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20399 ) Change subject: IMPALA-5081: Add codegen_opt_level query option .. Patch Set 11: (1 comment) http://gerrit.cloudera.org:8080/#/c/20399/10/be/src/service/query-options.cc File be/src/service/query-options.cc: http://gerrit.cloudera.org:8080/#/c/20399/10/be/src/service/query-options.cc@1147 PS10, Line 1147: break; > A 'break' is missing from here, be careful with the next rebase. Ack -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 11 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu Gerrit-Comment-Date: Tue, 17 Oct 2023 16:30:57 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Hello Daniel Becker, Yida Wu, Noemi Pap-Takacs, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20399 to look at the new patch set (#11). Change subject: IMPALA-5081: Add codegen_opt_level query option .. IMPALA-5081: Add codegen_opt_level query option Adds the 'codegen_opt_level' query option to select LLVM optimization level for generated code. Retains the prior behavior - O2 - as default. If optimization level is changed for an entry already in cache, the cache entry will be used unless the new optimization level is higher than the cached level. Adds unit tests that levels besides O0 inline and optimize a test function. Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd --- M be/src/codegen/CMakeLists.txt M be/src/codegen/llvm-codegen-cache-test.cc M be/src/codegen/llvm-codegen-cache.cc M be/src/codegen/llvm-codegen-cache.h 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/service/query-options.cc M be/src/service/query-options.h M common/thrift/ImpalaService.thrift M common/thrift/Query.thrift A testdata/llvm/test-opt.cc 12 files changed, 309 insertions(+), 38 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/99/20399/11 -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 11 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/20399 ) Change subject: IMPALA-5081: Add codegen_opt_level query option .. Patch Set 10: Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/20399/10/be/src/service/query-options.cc File be/src/service/query-options.cc: http://gerrit.cloudera.org:8080/#/c/20399/10/be/src/service/query-options.cc@1147 PS10, Line 1147: } A 'break' is missing from here, be careful with the next rebase. -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 10 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu Gerrit-Comment-Date: Tue, 17 Oct 2023 08:30:54 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20399 ) Change subject: IMPALA-5081: Add codegen_opt_level query option .. Patch Set 10: (1 comment) Both were rebases to resolve merge conflicts as other query options were added. http://gerrit.cloudera.org:8080/#/c/20399/10/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/20399/10/common/thrift/ImpalaService.thrift@845 PS10, Line 845: Enables predicate subsetting for Iceberg plan nodes. If enabled, expressions : // evaluated by Iceberg are not pushed down the scanner node. > Could you explain how this relates to Codegen optimization level? I could n This was from a rebase. -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 10 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu Gerrit-Comment-Date: Mon, 02 Oct 2023 17:41:59 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Noemi Pap-Takacs has posted comments on this change. ( http://gerrit.cloudera.org:8080/20399 ) Change subject: IMPALA-5081: Add codegen_opt_level query option .. Patch Set 10: (1 comment) Could you help me and explain the impact of the last two patch sets on the change? Thanks! http://gerrit.cloudera.org:8080/#/c/20399/10/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/20399/10/common/thrift/ImpalaService.thrift@845 PS10, Line 845: Enables predicate subsetting for Iceberg plan nodes. If enabled, expressions : // evaluated by Iceberg are not pushed down the scanner node. Could you explain how this relates to Codegen optimization level? I could not find any comments here or on the associated Jira ticket about Iceberg predicate pushdown. -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 10 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu Gerrit-Comment-Date: Mon, 02 Oct 2023 09:22:26 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20399 ) Change subject: IMPALA-5081: Add codegen_opt_level query option .. Patch Set 10: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/14106/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 10 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu Gerrit-Comment-Date: Thu, 28 Sep 2023 18:07:41 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20399 ) Change subject: IMPALA-5081: Add codegen_opt_level query option .. Patch Set 9: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/14105/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 9 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu Gerrit-Comment-Date: Thu, 28 Sep 2023 18:02:58 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Hello Daniel Becker, Yida Wu, Noemi Pap-Takacs, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20399 to look at the new patch set (#10). Change subject: IMPALA-5081: Add codegen_opt_level query option .. IMPALA-5081: Add codegen_opt_level query option Adds the 'codegen_opt_level' query option to select LLVM optimization level for generated code. Retains the prior behavior - O2 - as default. If optimization level is changed for an entry already in cache, the cache entry will be used unless the new optimization level is higher than the cached level. Adds unit tests that levels besides O0 inline and optimize a test function. Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd --- M be/src/codegen/CMakeLists.txt M be/src/codegen/llvm-codegen-cache-test.cc M be/src/codegen/llvm-codegen-cache.cc M be/src/codegen/llvm-codegen-cache.h 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/service/query-options.cc M be/src/service/query-options.h M common/thrift/ImpalaService.thrift M common/thrift/Query.thrift A testdata/llvm/test-opt.cc 12 files changed, 311 insertions(+), 39 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/99/20399/10 -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 10 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Hello Daniel Becker, Yida Wu, Noemi Pap-Takacs, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20399 to look at the new patch set (#9). Change subject: IMPALA-5081: Add codegen_opt_level query option .. IMPALA-5081: Add codegen_opt_level query option Adds the 'codegen_opt_level' query option to select LLVM optimization level for generated code. Retains the prior behavior - O2 - as default. If optimization level is changed for an entry already in cache, the cache entry will be used unless the new optimization level is higher than the cached level. Adds unit tests that levels besides O0 inline and optimize a test function. Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd --- M be/src/codegen/CMakeLists.txt M be/src/codegen/llvm-codegen-cache-test.cc M be/src/codegen/llvm-codegen-cache.cc M be/src/codegen/llvm-codegen-cache.h 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/service/query-options.cc M be/src/service/query-options.h M common/thrift/ImpalaService.thrift M common/thrift/Query.thrift A testdata/llvm/test-opt.cc 12 files changed, 311 insertions(+), 39 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/99/20399/9 -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 9 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20399 ) Change subject: IMPALA-5081: Add codegen_opt_level query option .. Patch Set 8: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 8 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu Gerrit-Comment-Date: Wed, 20 Sep 2023 03:22:29 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20399 ) Change subject: IMPALA-5081: Add codegen_opt_level query option .. Patch Set 8: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9734/ DRY_RUN=true -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 8 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu Gerrit-Comment-Date: Tue, 19 Sep 2023 22:56:30 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20399 ) Change subject: IMPALA-5081: Add codegen_opt_level query option .. Patch Set 8: Code-Review+1 Carrying +1. -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 8 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu Gerrit-Comment-Date: Tue, 19 Sep 2023 22:56:19 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20399 ) Change subject: IMPALA-5081: Add codegen_opt_level query option .. Patch Set 8: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/13992/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 8 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu Gerrit-Comment-Date: Tue, 12 Sep 2023 19:02:16 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20399 ) Change subject: IMPALA-5081: Add codegen_opt_level query option .. Patch Set 8: (4 comments) http://gerrit.cloudera.org:8080/#/c/20399/7/be/src/codegen/llvm-codegen-cache.h File be/src/codegen/llvm-codegen-cache.h: http://gerrit.cloudera.org:8080/#/c/20399/7/be/src/codegen/llvm-codegen-cache.h@178 PS7, Line 178: opt_level > nit. do you think it good to add a DCHECK for opt_level to see if it out of Compilation mostly handles this, and this value isn't directly used for any array accesses. http://gerrit.cloudera.org:8080/#/c/20399/7/be/src/codegen/llvm-codegen-cache.h@183 PS7, Line 183: er of instruction > nit. could you please add comments for the new fields? Would be good to tel Done http://gerrit.cloudera.org:8080/#/c/20399/7/be/src/codegen/llvm-codegen-cache.h@189 PS7, Line 189: /// The hashcode of function name > nit. could you please add a simple comment for the field. Done http://gerrit.cloudera.org:8080/#/c/20399/7/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: http://gerrit.cloudera.org:8080/#/c/20399/7/be/src/codegen/llvm-codegen.cc@1454 PS7, Line 1454: opt = llvm::PassBuilder::OptimizationLevel::O3; > nit. do we need the default case to handle unexpected values? The comment at line 1438 covers that. -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 8 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu Gerrit-Comment-Date: Tue, 12 Sep 2023 18:35:23 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Hello Daniel Becker, Yida Wu, Noemi Pap-Takacs, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20399 to look at the new patch set (#8). Change subject: IMPALA-5081: Add codegen_opt_level query option .. IMPALA-5081: Add codegen_opt_level query option Adds the 'codegen_opt_level' query option to select LLVM optimization level for generated code. Retains the prior behavior - O2 - as default. If optimization level is changed for an entry already in cache, the cache entry will be used unless the new optimization level is higher than the cached level. Adds unit tests that levels besides O0 inline and optimize a test function. Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd --- M be/src/codegen/CMakeLists.txt M be/src/codegen/llvm-codegen-cache-test.cc M be/src/codegen/llvm-codegen-cache.cc M be/src/codegen/llvm-codegen-cache.h 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/service/query-options.cc M be/src/service/query-options.h M common/thrift/ImpalaService.thrift M common/thrift/Query.thrift A testdata/llvm/test-opt.cc 12 files changed, 309 insertions(+), 38 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/99/20399/8 -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 8 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/20399 ) Change subject: IMPALA-5081: Add codegen_opt_level query option .. Patch Set 7: Code-Review+1 (4 comments) http://gerrit.cloudera.org:8080/#/c/20399/7/be/src/codegen/llvm-codegen-cache.h File be/src/codegen/llvm-codegen-cache.h: http://gerrit.cloudera.org:8080/#/c/20399/7/be/src/codegen/llvm-codegen-cache.h@178 PS7, Line 178: opt_level nit. do you think it good to add a DCHECK for opt_level to see if it out of TCodeGenOptLevel range? Because we will reset the cache entry from existing cache entry in the look up process, maybe some abnormal could be detected earlier here if opt value goes wrong. http://gerrit.cloudera.org:8080/#/c/20399/7/be/src/codegen/llvm-codegen-cache.h@183 PS7, Line 183: num_opt_functions nit. could you please add comments for the new fields? Would be good to tell the difference between the existing num_functions and num_instructions. http://gerrit.cloudera.org:8080/#/c/20399/7/be/src/codegen/llvm-codegen-cache.h@189 PS7, Line 189: TCodeGenOptLevel::type opt_level; nit. could you please add a simple comment for the field. http://gerrit.cloudera.org:8080/#/c/20399/7/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: http://gerrit.cloudera.org:8080/#/c/20399/7/be/src/codegen/llvm-codegen.cc@1454 PS7, Line 1454: break; nit. do we need the default case to handle unexpected values? -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 7 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu Gerrit-Comment-Date: Tue, 12 Sep 2023 17:51:49 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/20399 ) Change subject: IMPALA-5081: Add codegen_opt_level query option .. Patch Set 7: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 7 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu Gerrit-Comment-Date: Thu, 07 Sep 2023 12:25:07 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20399 ) Change subject: IMPALA-5081: Add codegen_opt_level query option .. Patch Set 7: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/13918/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 7 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu Gerrit-Comment-Date: Tue, 05 Sep 2023 19:00:20 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20399 ) Change subject: IMPALA-5081: Add codegen_opt_level query option .. Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/20399/6/be/src/codegen/llvm-codegen-test.cc File be/src/codegen/llvm-codegen-test.cc: http://gerrit.cloudera.org:8080/#/c/20399/6/be/src/codegen/llvm-codegen-test.cc@673 PS6, Line 673: constexpr > Could be constexpr. Done -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 7 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu Gerrit-Comment-Date: Tue, 05 Sep 2023 18:34:01 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Hello Daniel Becker, Yida Wu, Noemi Pap-Takacs, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20399 to look at the new patch set (#7). Change subject: IMPALA-5081: Add codegen_opt_level query option .. IMPALA-5081: Add codegen_opt_level query option Adds the 'codegen_opt_level' query option to select LLVM optimization level for generated code. Retains the prior behavior - O2 - as default. If optimization level is changed for an entry already in cache, the cache entry will be used unless the new optimization level is higher than the cached level. Adds unit tests that levels besides O0 inline and optimize a test function. Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd --- M be/src/codegen/CMakeLists.txt M be/src/codegen/llvm-codegen-cache-test.cc M be/src/codegen/llvm-codegen-cache.cc M be/src/codegen/llvm-codegen-cache.h 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/service/query-options.cc M be/src/service/query-options.h M common/thrift/ImpalaService.thrift M common/thrift/Query.thrift A testdata/llvm/test-opt.cc 12 files changed, 304 insertions(+), 38 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/99/20399/7 -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 7 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/20399 ) Change subject: IMPALA-5081: Add codegen_opt_level query option .. Patch Set 6: (1 comment) Thanks. http://gerrit.cloudera.org:8080/#/c/20399/6/be/src/codegen/llvm-codegen-test.cc File be/src/codegen/llvm-codegen-test.cc: http://gerrit.cloudera.org:8080/#/c/20399/6/be/src/codegen/llvm-codegen-test.cc@673 PS6, Line 673: std::arra Could be constexpr. -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 6 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu Gerrit-Comment-Date: Mon, 28 Aug 2023 11:30:58 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20399 ) Change subject: IMPALA-5081: Add codegen_opt_level query option .. Patch Set 6: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/13847/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 6 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu Gerrit-Comment-Date: Fri, 25 Aug 2023 18:03:34 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Hello Daniel Becker, Yida Wu, Noemi Pap-Takacs, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20399 to look at the new patch set (#6). Change subject: IMPALA-5081: Add codegen_opt_level query option .. IMPALA-5081: Add codegen_opt_level query option Adds the 'codegen_opt_level' query option to select LLVM optimization level for generated code. Retains the prior behavior - O2 - as default. If optimization level is changed for an entry already in cache, the cache entry will be used unless the new optimization level is higher than the cached level. Adds unit tests that levels besides O0 inline and optimize a test function. Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd --- M be/src/codegen/CMakeLists.txt M be/src/codegen/llvm-codegen-cache-test.cc M be/src/codegen/llvm-codegen-cache.cc M be/src/codegen/llvm-codegen-cache.h 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/service/query-options.cc M be/src/service/query-options.h M common/thrift/ImpalaService.thrift M common/thrift/Query.thrift A testdata/llvm/test-opt.cc 12 files changed, 304 insertions(+), 38 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/99/20399/6 -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 6 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20399 ) Change subject: IMPALA-5081: Add codegen_opt_level query option .. Patch Set 5: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/13846/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 5 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu Gerrit-Comment-Date: Fri, 25 Aug 2023 17:31:55 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20399 ) Change subject: IMPALA-5081: Add codegen_opt_level query option .. Patch Set 4: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/13845/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 4 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu Gerrit-Comment-Date: Fri, 25 Aug 2023 17:21:57 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Michael Smith has posted comments on this change. (
http://gerrit.cloudera.org:8080/20399 )
Change subject: IMPALA-5081: Add codegen_opt_level query option
..
Patch Set 5:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/20399/3/be/src/codegen/llvm-codegen-test.cc
File be/src/codegen/llvm-codegen-test.cc:
http://gerrit.cloudera.org:8080/#/c/20399/3/be/src/codegen/llvm-codegen-test.cc@679
PS3, Line 679:
> Nit: use.
Done
http://gerrit.cloudera.org:8080/#/c/20399/3/be/src/codegen/llvm-codegen-test.cc@694
PS3, Line 694: n
> Instead of hard-coding this value, we could extract the opt levels on L673
Done
http://gerrit.cloudera.org:8080/#/c/20399/3/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/20399/3/be/src/codegen/llvm-codegen.cc@129
PS3, Line 129: "The IR optimization level for pre-generated code; supports
O1, O2, and Os.");
> Is O0 removed because it prevents further optimisation?
It was an accidental change I reverted, this option is unrelated to the patch.
I guess I decided to omit O0 when creating this, just like O3 didn't seem to
help.
Also Clang O0 is weird and inserts lots of noinline comments, so similar to
https://gerrit.cloudera.org/c/20399/3/be/src/codegen/CMakeLists.txt#115 it
would almost certainly work poorly.
http://gerrit.cloudera.org:8080/#/c/20399/3/be/src/codegen/llvm-codegen.cc@1340
PS3, Line 1340: COUNTER_SET(num_opt_functions_,
> line too long (91 > 90)
Done
http://gerrit.cloudera.org:8080/#/c/20399/3/be/src/codegen/llvm-codegen.cc@1343
PS3, Line 1343: } else {
> line too long (91 > 90)
Done
--
To view, visit http://gerrit.cloudera.org:8080/20399
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd
Gerrit-Change-Number: 20399
Gerrit-PatchSet: 5
Gerrit-Owner: Michael Smith
Gerrit-Reviewer: Daniel Becker
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Smith
Gerrit-Reviewer: Noemi Pap-Takacs
Gerrit-Reviewer: Yida Wu
Gerrit-Comment-Date: Fri, 25 Aug 2023 17:06:25 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Hello Daniel Becker, Yida Wu, Noemi Pap-Takacs, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20399 to look at the new patch set (#5). Change subject: IMPALA-5081: Add codegen_opt_level query option .. IMPALA-5081: Add codegen_opt_level query option Adds the 'codegen_opt_level' query option to select LLVM optimization level for generated code. Retains the prior behavior - O2 - as default. If optimization level is changed for an entry already in cache, the cache entry will be used unless the new optimization level is higher than the cached level. Adds unit tests that levels besides O0 inline and optimize a test function. Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd --- M be/src/codegen/CMakeLists.txt M be/src/codegen/llvm-codegen-cache-test.cc M be/src/codegen/llvm-codegen-cache.cc M be/src/codegen/llvm-codegen-cache.h 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/service/query-options.cc M be/src/service/query-options.h M common/thrift/ImpalaService.thrift M common/thrift/Query.thrift A testdata/llvm/test-opt.cc 12 files changed, 304 insertions(+), 38 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/99/20399/5 -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 5 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Hello Daniel Becker, Yida Wu, Noemi Pap-Takacs, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20399 to look at the new patch set (#4). Change subject: IMPALA-5081: Add codegen_opt_level query option .. IMPALA-5081: Add codegen_opt_level query option Adds the 'codegen_opt_level' query option to select LLVM optimization level for generated code. Retains the prior behavior - O2 - as default. If optimization level is changed for an entry already in cache, the cache entry will be used unless the new optimization level is higher than the cached level. Adds unit tests that levels besides O0 inline and optimize a test function. Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd --- M be/src/codegen/CMakeLists.txt M be/src/codegen/llvm-codegen-cache-test.cc M be/src/codegen/llvm-codegen-cache.cc M be/src/codegen/llvm-codegen-cache.h 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/service/query-options.cc M be/src/service/query-options.h M common/thrift/ImpalaService.thrift M common/thrift/Query.thrift A testdata/llvm/test-opt.cc 12 files changed, 302 insertions(+), 38 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/99/20399/4 -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 4 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20399 ) Change subject: IMPALA-5081: Add codegen_opt_level query option .. Patch Set 4: (2 comments) http://gerrit.cloudera.org:8080/#/c/20399/4/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: http://gerrit.cloudera.org:8080/#/c/20399/4/be/src/codegen/llvm-codegen.cc@1340 PS4, Line 1340: COUNTER_SET(num_opt_functions_, counter.GetCount(InstructionCounter::TOTAL_FUNCTIONS)); line too long (91 > 90) http://gerrit.cloudera.org:8080/#/c/20399/4/be/src/codegen/llvm-codegen.cc@1343 PS4, Line 1343: COUNTER_SET(num_opt_functions_, counter.GetCount(InstructionCounter::TOTAL_FUNCTIONS)); line too long (91 > 90) -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 4 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu Gerrit-Comment-Date: Fri, 25 Aug 2023 17:00:07 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/20399 ) Change subject: IMPALA-5081: Add codegen_opt_level query option .. Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/20399/3/be/src/codegen/llvm-codegen-test.cc File be/src/codegen/llvm-codegen-test.cc: http://gerrit.cloudera.org:8080/#/c/20399/3/be/src/codegen/llvm-codegen-test.cc@679 PS3, Line 679: used Nit: use. http://gerrit.cloudera.org:8080/#/c/20399/3/be/src/codegen/llvm-codegen-test.cc@694 PS3, Line 694: 5 Instead of hard-coding this value, we could extract the opt levels on L673 into a collection (e.g. vector) and use its size. http://gerrit.cloudera.org:8080/#/c/20399/2/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: http://gerrit.cloudera.org:8080/#/c/20399/2/be/src/codegen/llvm-codegen.cc@1448 PS2, Line 1448: p > This is multiplication and preserves the prior formatting. Pretty sure code Right, I thought it was pointer dereferencing. http://gerrit.cloudera.org:8080/#/c/20399/3/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: http://gerrit.cloudera.org:8080/#/c/20399/3/be/src/codegen/llvm-codegen.cc@129 PS3, Line 129: "The IR optimization level for pre-generated code; supports O1, O2, and Os."); Is O0 removed because it prevents further optimisation? -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu Gerrit-Comment-Date: Fri, 25 Aug 2023 11:52:29 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Yida Wu has posted comments on this change. (
http://gerrit.cloudera.org:8080/20399 )
Change subject: IMPALA-5081: Add codegen_opt_level query option
..
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20399/2/be/src/codegen/llvm-codegen-cache.h
File be/src/codegen/llvm-codegen-cache.h:
http://gerrit.cloudera.org:8080/#/c/20399/2/be/src/codegen/llvm-codegen-cache.h@166
PS2, Line 166: bool Empty() { return engine_pointer == nullptr; }
> Not relevant to this change but wouldn't it be better to call Reset() here
Thanks Daniel for pointing this out. It makes sense to me. I think it was
because previously the struct contained shared_ptr, and somehow needs some hack
to initialize it, but now it changes to a pointer. It should be okay if all the
codegen caching tests can pass.
--
To view, visit http://gerrit.cloudera.org:8080/20399
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd
Gerrit-Change-Number: 20399
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Smith
Gerrit-Reviewer: Daniel Becker
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Smith
Gerrit-Reviewer: Noemi Pap-Takacs
Gerrit-Reviewer: Yida Wu
Gerrit-Comment-Date: Fri, 25 Aug 2023 00:09:35 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20399 ) Change subject: IMPALA-5081: Add codegen_opt_level query option .. Patch Set 3: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/13839/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu Gerrit-Comment-Date: Fri, 25 Aug 2023 00:08:03 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20399 ) Change subject: IMPALA-5081: Add codegen_opt_level query option .. Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/20399/3/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: http://gerrit.cloudera.org:8080/#/c/20399/3/be/src/codegen/llvm-codegen.cc@1340 PS3, Line 1340: COUNTER_SET(num_opt_functions_, counter.GetCount(InstructionCounter::TOTAL_FUNCTIONS)); line too long (91 > 90) http://gerrit.cloudera.org:8080/#/c/20399/3/be/src/codegen/llvm-codegen.cc@1343 PS3, Line 1343: COUNTER_SET(num_opt_functions_, counter.GetCount(InstructionCounter::TOTAL_FUNCTIONS)); line too long (91 > 90) -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu Gerrit-Comment-Date: Thu, 24 Aug 2023 23:49:17 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20399 ) Change subject: IMPALA-5081: Add codegen_opt_level query option .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/20399/2/be/src/codegen/llvm-codegen-test.cc File be/src/codegen/llvm-codegen-test.cc: http://gerrit.cloudera.org:8080/#/c/20399/2/be/src/codegen/llvm-codegen-test.cc@594 PS2, Line 594: > Nit: could be 'nullptr'. Done -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu Gerrit-Comment-Date: Thu, 24 Aug 2023 23:41:58 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Hello Daniel Becker, Yida Wu, Noemi Pap-Takacs, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20399 to look at the new patch set (#3). Change subject: IMPALA-5081: Add codegen_opt_level query option .. IMPALA-5081: Add codegen_opt_level query option Adds the 'codegen_opt_level' query option to select LLVM optimization level for generated code. Retains the prior behavior - O2 - as default. If optimization level is changed for an entry already in cache, the cache entry will be used unless the new optimization level is higher than the cached level. Adds unit tests that levels besides O0 inline and optimize a test function. Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd --- M be/src/codegen/CMakeLists.txt M be/src/codegen/llvm-codegen-cache-test.cc M be/src/codegen/llvm-codegen-cache.cc M be/src/codegen/llvm-codegen-cache.h 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/service/query-options.cc M be/src/service/query-options.h M common/thrift/ImpalaService.thrift M common/thrift/Query.thrift A testdata/llvm/test-opt.cc 12 files changed, 300 insertions(+), 38 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/99/20399/3 -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Michael Smith has posted comments on this change. (
http://gerrit.cloudera.org:8080/20399 )
Change subject: IMPALA-5081: Add codegen_opt_level query option
..
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/20399/2/be/src/codegen/llvm-codegen-cache.h
File be/src/codegen/llvm-codegen-cache.h:
http://gerrit.cloudera.org:8080/#/c/20399/2/be/src/codegen/llvm-codegen-cache.h@166
PS2, Line 166: void Init() { memset((uint8_t*)this, 0,
sizeof(CodeGenCacheEntry)); }
> Not relevant to this change but wouldn't it be better to call Reset() here
I don't see any problem with changing to that. I'll let Yida weigh in though.
http://gerrit.cloudera.org:8080/#/c/20399/2/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/20399/2/be/src/codegen/llvm-codegen.cc@1448
PS2, Line 1448:
> Nit: unneeded space.
This is multiplication and preserves the prior formatting. Pretty sure code
formatter would complain if I removed the space.
http://gerrit.cloudera.org:8080/#/c/20399/2/common/thrift/ImpalaService.thrift
File common/thrift/ImpalaService.thrift:
http://gerrit.cloudera.org:8080/#/c/20399/2/common/thrift/ImpalaService.thrift@845
PS2, Line 845: O1, Os, O2, or O3
> O0 is also a possibility.
Ack
http://gerrit.cloudera.org:8080/#/c/20399/2/common/thrift/ImpalaService.thrift@846
PS2, Line 846: Defaults to O2.
> If we ever change the default value we'll probably forget this comment. The
This is a pattern we use a lot of other places here. I'm amenable to this
argument however.
--
To view, visit http://gerrit.cloudera.org:8080/20399
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd
Gerrit-Change-Number: 20399
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Smith
Gerrit-Reviewer: Daniel Becker
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Smith
Gerrit-Reviewer: Noemi Pap-Takacs
Gerrit-Reviewer: Yida Wu
Gerrit-Comment-Date: Thu, 24 Aug 2023 16:43:47 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Daniel Becker has posted comments on this change. (
http://gerrit.cloudera.org:8080/20399 )
Change subject: IMPALA-5081: Add codegen_opt_level query option
..
Patch Set 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/20399/2/be/src/codegen/llvm-codegen-cache.h
File be/src/codegen/llvm-codegen-cache.h:
http://gerrit.cloudera.org:8080/#/c/20399/2/be/src/codegen/llvm-codegen-cache.h@166
PS2, Line 166: void Init() { memset((uint8_t*)this, 0,
sizeof(CodeGenCacheEntry)); }
Not relevant to this change but wouldn't it be better to call Reset() here
instead of memset()? Afaik 'nullptr' is not guaranteed to be represented by the
0 value.
http://gerrit.cloudera.org:8080/#/c/20399/2/be/src/codegen/llvm-codegen-test.cc
File be/src/codegen/llvm-codegen-test.cc:
http://gerrit.cloudera.org:8080/#/c/20399/2/be/src/codegen/llvm-codegen-test.cc@594
PS2, Line 594: NULL
Nit: could be 'nullptr'.
http://gerrit.cloudera.org:8080/#/c/20399/2/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/20399/2/be/src/codegen/llvm-codegen.cc@1448
PS2, Line 1448:
Nit: unneeded space.
http://gerrit.cloudera.org:8080/#/c/20399/2/common/thrift/ImpalaService.thrift
File common/thrift/ImpalaService.thrift:
http://gerrit.cloudera.org:8080/#/c/20399/2/common/thrift/ImpalaService.thrift@845
PS2, Line 845: O1, Os, O2, or O3
O0 is also a possibility.
http://gerrit.cloudera.org:8080/#/c/20399/2/common/thrift/ImpalaService.thrift@846
PS2, Line 846: Defaults to O2.
If we ever change the default value we'll probably forget this comment. The
default value can be seen in common/thrift/Query.thrift, so we don't need to
write it here.
--
To view, visit http://gerrit.cloudera.org:8080/20399
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd
Gerrit-Change-Number: 20399
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Smith
Gerrit-Reviewer: Daniel Becker
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Smith
Gerrit-Reviewer: Noemi Pap-Takacs
Gerrit-Reviewer: Yida Wu
Gerrit-Comment-Date: Thu, 24 Aug 2023 12:03:20 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20399 ) Change subject: IMPALA-5081: Add codegen_opt_level query option .. Patch Set 2: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/13827/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu Gerrit-Comment-Date: Wed, 23 Aug 2023 22:59:00 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20399 ) Change subject: IMPALA-5081: Add codegen_opt_level query option .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/20399/2/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: http://gerrit.cloudera.org:8080/#/c/20399/2/be/src/codegen/llvm-codegen.cc@1331 PS2, Line 1331: COUNTER_SET(num_opt_functions_, counter.GetCount(InstructionCounter::TOTAL_FUNCTIONS)); line too long (91 > 90) http://gerrit.cloudera.org:8080/#/c/20399/2/be/src/codegen/llvm-codegen.cc@1334 PS2, Line 1334: COUNTER_SET(num_opt_functions_, counter.GetCount(InstructionCounter::TOTAL_FUNCTIONS)); line too long (91 > 90) -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu Gerrit-Comment-Date: Wed, 23 Aug 2023 22:34:29 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Hello Daniel Becker, Yida Wu, Noemi Pap-Takacs, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20399 to look at the new patch set (#2). Change subject: IMPALA-5081: Add codegen_opt_level query option .. IMPALA-5081: Add codegen_opt_level query option Adds the 'codegen_opt_level' query option to select LLVM optimization level for generated code. Retains the prior behavior - O2 - as default. Adds unit tests that levels besides O0 inline and optimize a test function. Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd --- M be/src/codegen/CMakeLists.txt M be/src/codegen/llvm-codegen-cache.cc M be/src/codegen/llvm-codegen-cache.h 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/service/query-options.cc M be/src/service/query-options.h M common/thrift/ImpalaService.thrift M common/thrift/Query.thrift A testdata/llvm/test-opt.cc 11 files changed, 223 insertions(+), 24 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/99/20399/2 -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Noemi Pap-Takacs Gerrit-Reviewer: Yida Wu
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Michael Smith has posted comments on this change. (
http://gerrit.cloudera.org:8080/20399 )
Change subject: IMPALA-5081: Add codegen_opt_level query option
..
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/20399/1/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/20399/1/be/src/codegen/llvm-codegen.cc@1409
PS1, Line 1409: switch (optLevel) {
> As far as I know, non-default Query Options are visible in the profile.
Yes, it shows up in the query profile. I was thinking more along the lines of
verifying that we're actually optimizing at different levels.
llvm::PassBuilder::OptimizationLevel doesn't have a string representation. I
think the profile list of non-default query options is sufficient as feedback.
http://gerrit.cloudera.org:8080/#/c/20399/1/common/thrift/ImpalaService.thrift
File common/thrift/ImpalaService.thrift:
http://gerrit.cloudera.org:8080/#/c/20399/1/common/thrift/ImpalaService.thrift@837
PS1, Line 837: Has no effect on cached
: // codegen entries
> Do you think it would be worthwhile to make it possible to force clearing t
Yida and I were discussing this in JIRA. That's another option we didn't
mention. This is somewhat experimental, I suggested making improvements to the
workflow if it shows promising results.
It is possible to exclude a query from the cache while experimenting with
disable_codegen_cache, but that requires forethought.
--
To view, visit http://gerrit.cloudera.org:8080/20399
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd
Gerrit-Change-Number: 20399
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Smith
Gerrit-Reviewer: Daniel Becker
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Smith
Gerrit-Reviewer: Noemi Pap-Takacs
Gerrit-Reviewer: Yida Wu
Gerrit-Comment-Date: Wed, 23 Aug 2023 16:35:25 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Noemi Pap-Takacs has posted comments on this change. (
http://gerrit.cloudera.org:8080/20399 )
Change subject: IMPALA-5081: Add codegen_opt_level query option
..
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20399/1/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/20399/1/be/src/codegen/llvm-codegen.cc@1409
PS1, Line 1409: switch (optLevel) {
> Would it make sense to log the optimisation level in the profile (or in the
As far as I know, non-default Query Options are visible in the profile.
--
To view, visit http://gerrit.cloudera.org:8080/20399
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd
Gerrit-Change-Number: 20399
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Smith
Gerrit-Reviewer: Daniel Becker
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Smith
Gerrit-Reviewer: Noemi Pap-Takacs
Gerrit-Reviewer: Yida Wu
Gerrit-Comment-Date: Wed, 23 Aug 2023 12:43:00 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Daniel Becker has posted comments on this change. (
http://gerrit.cloudera.org:8080/20399 )
Change subject: IMPALA-5081: Add codegen_opt_level query option
..
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/20399/1/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/20399/1/be/src/codegen/llvm-codegen.cc@1409
PS1, Line 1409: switch (optLevel) {
> Tests of flags that are accepted could make sense. I'm not sure there are c
Would it make sense to log the optimisation level in the profile (or in the
INFO log)?
http://gerrit.cloudera.org:8080/#/c/20399/1/common/thrift/ImpalaService.thrift
File common/thrift/ImpalaService.thrift:
http://gerrit.cloudera.org:8080/#/c/20399/1/common/thrift/ImpalaService.thrift@837
PS1, Line 837: Has no effect on cached
: // codegen entries
Do you think it would be worthwhile to make it possible to force clearing the
cache without restarting Impala (in a separate commit)?
--
To view, visit http://gerrit.cloudera.org:8080/20399
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd
Gerrit-Change-Number: 20399
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Smith
Gerrit-Reviewer: Daniel Becker
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Smith
Gerrit-Reviewer: Yida Wu
Gerrit-Comment-Date: Wed, 23 Aug 2023 12:37:55 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Michael Smith has posted comments on this change. (
http://gerrit.cloudera.org:8080/20399 )
Change subject: IMPALA-5081: Add codegen_opt_level query option
..
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20399/1/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/20399/1/be/src/codegen/llvm-codegen.cc@1409
PS1, Line 1409: switch (optLevel) {
Tests of flags that are accepted could make sense. I'm not sure there are
clearly visible effects in the profile of choosing these different settings
that we could look for. Maybe resulting code size.
--
To view, visit http://gerrit.cloudera.org:8080/20399
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd
Gerrit-Change-Number: 20399
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Smith
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Smith
Gerrit-Reviewer: Yida Wu
Gerrit-Comment-Date: Tue, 22 Aug 2023 23:30:20 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20399 ) Change subject: IMPALA-5081: Add codegen_opt_level query option .. Patch Set 1: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/13815/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Smith Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Smith Gerrit-Reviewer: Yida Wu Gerrit-Comment-Date: Tue, 22 Aug 2023 21:05:16 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5081: Add codegen opt level query option
Michael Smith has uploaded this change for review. ( http://gerrit.cloudera.org:8080/20399 Change subject: IMPALA-5081: Add codegen_opt_level query option .. IMPALA-5081: Add codegen_opt_level query option Adds the 'codegen_opt_level' query option to select LLVM optimization level for generated code. Retains the prior behavior - O2 - as default. Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd --- M be/src/codegen/llvm-codegen.cc M be/src/service/query-options.cc M be/src/service/query-options.h M common/thrift/ImpalaService.thrift M common/thrift/Query.thrift 5 files changed, 41 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/99/20399/1 -- To view, visit http://gerrit.cloudera.org:8080/20399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I371f8758b6552263e91a1fbfd9a6e1c28e1fa2bd Gerrit-Change-Number: 20399 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Smith
