You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org> on 2018/07/18 18:19:51 UTC
[Impala-ASF-CR] IMPALA-6299: Modify IRBuilder to use LLVM's CPU features
Pooja Nilangekar has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10979
Change subject: IMPALA-6299: Modify IRBuilder to use LLVM's CPU features
......................................................................
IMPALA-6299: Modify IRBuilder to use LLVM's CPU features
Previously, the IRBuilder of the LlvmCodeGen class used CpuInfo's
list of enabled features while determine the validity of certian
instructions. It did not consider the whitelist which passed while
initilaizing the LlvmCodeGen class. Now, the IRBuilder inspects
its own cpu attributes before emitting instruction. This change
also adds functionality to modify the cpu attributes of the
LlvmCodeGen class for testing.
Testing: Verified that the current tests which use and modify
CpuInfo produce expected results.
Change-Id: Ifece8949c143146d2a1b38d72d21c2d733bed90f
---
M be/src/benchmarks/bloom-filter-benchmark.cc
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/util/bit-util-test.cc
M be/src/util/cpu-info.cc
M be/src/util/cpu-info.h
7 files changed, 110 insertions(+), 30 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/10979/2
--
To view, visit http://gerrit.cloudera.org:8080/10979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ifece8949c143146d2a1b38d72d21c2d733bed90f
Gerrit-Change-Number: 10979
Gerrit-PatchSet: 2
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-6299: Modify IRBuilder to use LLVM's CPU features
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/10979 )
Change subject: IMPALA-6299: Modify IRBuilder to use LLVM's CPU features
......................................................................
Patch Set 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/10979/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10979/2//COMMIT_MSG@10
PS2, Line 10: while
nit: to
http://gerrit.cloudera.org:8080/#/c/10979/2//COMMIT_MSG@10
PS2, Line 10: certian
nit:typo
http://gerrit.cloudera.org:8080/#/c/10979/2/be/src/codegen/llvm-codegen.h
File be/src/codegen/llvm-codegen.h:
http://gerrit.cloudera.org:8080/#/c/10979/2/be/src/codegen/llvm-codegen.h@737
PS2, Line 737: current_cpu_attrs_
> Maybe enabled_cpu_attrs_?
I agree with Tim
http://gerrit.cloudera.org:8080/#/c/10979/2/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/10979/2/be/src/codegen/llvm-codegen.cc@252
PS2, Line 252: auto cpu_flag_it = cpu_flag_mappings_.find(CpuInfo::SSE4_2);
: DCHECK(cpu_flag_it != cpu_flag_mappings_.end());
: if (current_cpu_attrs_.find(cpu_flag_it->second) != current_cpu_attrs_.end(
can we use IsCPUFeatureEnabled() here?
http://gerrit.cloudera.org:8080/#/c/10979/2/be/src/codegen/llvm-codegen.cc@1581
PS2, Line 1581: auto cpu_flag_it = cpu_flag_mappings_.find(CpuInfo::SSE4_2);
: DCHECK(cpu_flag_it != cpu_flag_mappings_.end());
: if (current_cpu_attrs_.find(cpu_flag_it->second) != current_cpu_attrs_.end(
same here, IsCPUFeatureEnabled can be used
--
To view, visit http://gerrit.cloudera.org:8080/10979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifece8949c143146d2a1b38d72d21c2d733bed90f
Gerrit-Change-Number: 10979
Gerrit-PatchSet: 2
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Jul 2018 20:40:42 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6299: Modify IRBuilder to use LLVM's CPU features
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10979 )
Change subject: IMPALA-6299: Modify IRBuilder to use LLVM's CPU features
......................................................................
Patch Set 2:
I made the mistake of only looking at the code that was changed. I think this is missing some updates to codegen functions. E.g. there's a CpuInfo::IsSupported(CpuInfo::AVX2) in FilterContext::CodegenInsert() that should be converted to the new API. I did a quick grep and I think that might be the only place, but you should also verify that we don't need to update any more callsites of CpuInfo::IsSupported
--
To view, visit http://gerrit.cloudera.org:8080/10979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifece8949c143146d2a1b38d72d21c2d733bed90f
Gerrit-Change-Number: 10979
Gerrit-PatchSet: 2
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Jul 2018 16:50:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6299: Modify IRBuilder to use LLVM's CPU features
Posted by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org>.
Pooja Nilangekar has posted comments on this change. ( http://gerrit.cloudera.org:8080/10979 )
Change subject: IMPALA-6299: Modify IRBuilder to use LLVM's CPU features
......................................................................
Patch Set 5:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/10979/2/be/src/codegen/llvm-codegen-test.cc
File be/src/codegen/llvm-codegen-test.cc:
http://gerrit.cloudera.org:8080/#/c/10979/2/be/src/codegen/llvm-codegen-test.cc@467
PS2, Line 467: data1_hash_fn, llvm::ArrayRef<llvm::Value*>({llvm_data1, llvm_len1, seed}));
> Thanks for explaining. Might be worth leaving a brief comment explaining th
Done
http://gerrit.cloudera.org:8080/#/c/10979/2/be/src/codegen/llvm-codegen.h
File be/src/codegen/llvm-codegen.h:
http://gerrit.cloudera.org:8080/#/c/10979/2/be/src/codegen/llvm-codegen.h@737
PS2, Line 737:
> How about we move the method EnableCPUFeature out from this class and into
Done. I have moved the helper function to the test class and added a comment stating that cpu_attrs_ should be modified only for tests.
http://gerrit.cloudera.org:8080/#/c/10979/3/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/10979/3/be/src/codegen/llvm-codegen.cc@1519
PS3, Line 1519:
> nit: not a big deal but just to be consistent with CpuInfo and the static f
Done
--
To view, visit http://gerrit.cloudera.org:8080/10979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifece8949c143146d2a1b38d72d21c2d733bed90f
Gerrit-Change-Number: 10979
Gerrit-PatchSet: 5
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 21 Jul 2018 00:33:52 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6299: Use LlvmCodeGen's internal list of white-listed CPU attributes for handcrafting IRs
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10979 )
Change subject: IMPALA-6299: Use LlvmCodeGen's internal list of white-listed CPU attributes for handcrafting IRs
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/10979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifece8949c143146d2a1b38d72d21c2d733bed90f
Gerrit-Change-Number: 10979
Gerrit-PatchSet: 6
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 23 Jul 2018 20:41:02 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6299: Modify IRBuilder to use LLVM's CPU features
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/10979 )
Change subject: IMPALA-6299: Modify IRBuilder to use LLVM's CPU features
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10979/2/be/src/codegen/llvm-codegen.h
File be/src/codegen/llvm-codegen.h:
http://gerrit.cloudera.org:8080/#/c/10979/2/be/src/codegen/llvm-codegen.h@737
PS2, Line 737: enabled_cpu_attrs_
> Thanks for the explanation. I agree that we should be defensive. Maybe all
How about we move the method EnableCPUFeature out from this class and into the test class only and maintain this duplication of attributes there.
and also comment on cpu_attrs_ that it should not be modified except in InitializeLlvm() or for testing purposes.
http://gerrit.cloudera.org:8080/#/c/10979/3/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/10979/3/be/src/codegen/llvm-codegen.cc@1519
PS3, Line 1519: long
nit: not a big deal but just to be consistent with CpuInfo and the static flags, use int64_t
--
To view, visit http://gerrit.cloudera.org:8080/10979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifece8949c143146d2a1b38d72d21c2d733bed90f
Gerrit-Change-Number: 10979
Gerrit-PatchSet: 3
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 20 Jul 2018 19:10:42 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6299: Use LlvmCodeGen's internal list of white-listed CPU attributes for handcrafting IRs
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10979 )
Change subject: IMPALA-6299: Use LlvmCodeGen's internal list of white-listed CPU attributes for handcrafting IRs
......................................................................
Patch Set 6:
Build Started https://jenkins.impala.io/job/gerrit-code-review-checks/12/
Running initial code review checks.
--
To view, visit http://gerrit.cloudera.org:8080/10979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifece8949c143146d2a1b38d72d21c2d733bed90f
Gerrit-Change-Number: 10979
Gerrit-PatchSet: 6
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 23 Jul 2018 20:10:48 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6299: Use LlvmCodeGen's internal list of white-listed CPU attributes for handcrafting IRs
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10979 )
Change subject: IMPALA-6299: Use LlvmCodeGen's internal list of white-listed CPU attributes for handcrafting IRs
......................................................................
IMPALA-6299: Use LlvmCodeGen's internal list of white-listed CPU attributes for handcrafting IRs
Previously, the IRBuilder of the LlvmCodeGen class used CpuInfo's
list of enabled features to determine the validity of certain
instructions to emit intrinsics. It did not consider the whitelist
which passed while initializing the LlvmCodeGen class. Now, the
IRBuilder inspects its own CPU attributes before emitting
instruction. This change also adds functionality to modify the CPU
attributes of the LlvmCodeGen class for testing.
Testing: Verified that the current tests which use and modify
CpuInfo produce expected results.
Change-Id: Ifece8949c143146d2a1b38d72d21c2d733bed90f
Reviewed-on: http://gerrit.cloudera.org:8080/10979
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/benchmarks/bloom-filter-benchmark.cc
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/filter-context.cc
M be/src/util/cpu-info.cc
6 files changed, 95 insertions(+), 29 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/10979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ifece8949c143146d2a1b38d72d21c2d733bed90f
Gerrit-Change-Number: 10979
Gerrit-PatchSet: 8
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-6299: Use LlvmCodeGen's internal list of white-listed CPU attributes for handcrafting IRs
Posted by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org>.
Pooja Nilangekar has posted comments on this change. ( http://gerrit.cloudera.org:8080/10979 )
Change subject: IMPALA-6299: Use LlvmCodeGen's internal list of white-listed CPU attributes for handcrafting IRs
......................................................................
Patch Set 6:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/10979/5//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10979/5//COMMIT_MSG@7
PS5, Line 7: IMPALA-6299: Use LlvmCodeGen's internal list of white-li
> Use LlvmCodeGen's internal list of white-listed CPU attributes for handcraf
Done
http://gerrit.cloudera.org:8080/#/c/10979/5//COMMIT_MSG@10
PS5, Line 10: determine the validity of certain
: instructions
> to emit intrinsics
Done
http://gerrit.cloudera.org:8080/#/c/10979/5/be/src/codegen/llvm-codegen-test.cc
File be/src/codegen/llvm-codegen-test.cc:
http://gerrit.cloudera.org:8080/#/c/10979/5/be/src/codegen/llvm-codegen-test.cc@116
PS5, Line 116: EnableCodeGenCPUFeat
> nit: EnableCodeGenCPUFeature
Done
--
To view, visit http://gerrit.cloudera.org:8080/10979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifece8949c143146d2a1b38d72d21c2d733bed90f
Gerrit-Change-Number: 10979
Gerrit-PatchSet: 6
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 23 Jul 2018 20:10:54 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6299: Use LlvmCodeGen's internal list of white-listed CPU attributes for handcrafting IRs
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10979 )
Change subject: IMPALA-6299: Use LlvmCodeGen's internal list of white-listed CPU attributes for handcrafting IRs
......................................................................
Patch Set 6:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/11/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/10979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifece8949c143146d2a1b38d72d21c2d733bed90f
Gerrit-Change-Number: 10979
Gerrit-PatchSet: 6
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 23 Jul 2018 20:23:14 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6299: Use LlvmCodeGen's internal list of white-listed CPU attributes for handcrafting IRs
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10979 )
Change subject: IMPALA-6299: Use LlvmCodeGen's internal list of white-listed CPU attributes for handcrafting IRs
......................................................................
Patch Set 6:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/12/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/10979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifece8949c143146d2a1b38d72d21c2d733bed90f
Gerrit-Change-Number: 10979
Gerrit-PatchSet: 6
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 23 Jul 2018 20:38:05 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6299: Modify IRBuilder to use LLVM's CPU features
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/10979 )
Change subject: IMPALA-6299: Modify IRBuilder to use LLVM's CPU features
......................................................................
Patch Set 5: Code-Review+1
(3 comments)
http://gerrit.cloudera.org:8080/#/c/10979/5//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10979/5//COMMIT_MSG@7
PS5, Line 7: IMPALA-6299: Modify IRBuilder to use LLVM's CPU features
Use LlvmCodeGen's internal list of white-listed CPU attributes for handcrafting IRs
http://gerrit.cloudera.org:8080/#/c/10979/5//COMMIT_MSG@10
PS5, Line 10: determine the validity of certain
: instructions
to emit intrinsics
http://gerrit.cloudera.org:8080/#/c/10979/5/be/src/codegen/llvm-codegen-test.cc
File be/src/codegen/llvm-codegen-test.cc:
http://gerrit.cloudera.org:8080/#/c/10979/5/be/src/codegen/llvm-codegen-test.cc@116
PS5, Line 116: EnableCodeGenFeature
nit: EnableCodeGenCPUFeature
--
To view, visit http://gerrit.cloudera.org:8080/10979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifece8949c143146d2a1b38d72d21c2d733bed90f
Gerrit-Change-Number: 10979
Gerrit-PatchSet: 5
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 23 Jul 2018 19:13:46 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6299: Modify IRBuilder to use LLVM's CPU features
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10979 )
Change subject: IMPALA-6299: Modify IRBuilder to use LLVM's CPU features
......................................................................
Patch Set 2:
(3 comments)
No worries about the rebase, it happens sometimes.
http://gerrit.cloudera.org:8080/#/c/10979/2/be/src/codegen/llvm-codegen-test.cc
File be/src/codegen/llvm-codegen-test.cc:
http://gerrit.cloudera.org:8080/#/c/10979/2/be/src/codegen/llvm-codegen-test.cc@467
PS2, Line 467: CpuInfo::EnableFeature(CpuInfo::SSE4_2, false);
> Yes, because `expected_hash` is computed using the `HashUtil::Hash` functio
Thanks for explaining. Might be worth leaving a brief comment explaining this.
http://gerrit.cloudera.org:8080/#/c/10979/2/be/src/codegen/llvm-codegen.h
File be/src/codegen/llvm-codegen.h:
http://gerrit.cloudera.org:8080/#/c/10979/2/be/src/codegen/llvm-codegen.h@737
PS2, Line 737: current_cpu_attrs_
Thanks for the explanation. I agree that we should be defensive. Maybe all this needs is a slight clarifying comment about when the contents of the maps may change?
> The other option I can think of is trying to figure out from the context if this is an impalad instance or a test call.
We have TestInfo::is_test() - you could add a DCHECK.
http://gerrit.cloudera.org:8080/#/c/10979/2/be/src/util/bit-util-test.cc
File be/src/util/bit-util-test.cc:
http://gerrit.cloudera.org:8080/#/c/10979/2/be/src/util/bit-util-test.cc@325
PS2, Line 325: const int64_t CPU_INFO_FLAG = CpuInfo::SSSE3;
> I had to change it because I moved the declaration of CpuInfo::SSSE3 to cpu
Gotcha, didn't think of that
--
To view, visit http://gerrit.cloudera.org:8080/10979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifece8949c143146d2a1b38d72d21c2d733bed90f
Gerrit-Change-Number: 10979
Gerrit-PatchSet: 2
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 20 Jul 2018 18:28:29 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6299: Use LlvmCodeGen's internal list of white-listed CPU attributes for handcrafting IRs
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10979 )
Change subject: IMPALA-6299: Use LlvmCodeGen's internal list of white-listed CPU attributes for handcrafting IRs
......................................................................
Patch Set 7:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2849/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/10979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifece8949c143146d2a1b38d72d21c2d733bed90f
Gerrit-Change-Number: 10979
Gerrit-PatchSet: 7
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 23 Jul 2018 20:55:11 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6299: Modify IRBuilder to use LLVM's CPU features
Posted by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org>.
Pooja Nilangekar has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/10979 )
Change subject: IMPALA-6299: Modify IRBuilder to use LLVM's CPU features
......................................................................
IMPALA-6299: Modify IRBuilder to use LLVM's CPU features
Previously, the IRBuilder of the LlvmCodeGen class used CpuInfo's
list of enabled features to determine the validity of certain
instructions. It did not consider the whitelist which passed while
initilaizing the LlvmCodeGen class. Now, the IRBuilder inspects
its own cpu attributes before emitting instruction. This change
also adds functionality to modify the cpu attributes of the
LlvmCodeGen class for testing.
Testing: Verified that the current tests which use and modify
CpuInfo produce expected results.
Change-Id: Ifece8949c143146d2a1b38d72d21c2d733bed90f
---
M be/src/benchmarks/bloom-filter-benchmark.cc
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/filter-context.cc
M be/src/util/cpu-info.cc
6 files changed, 102 insertions(+), 23 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/10979/3
--
To view, visit http://gerrit.cloudera.org:8080/10979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifece8949c143146d2a1b38d72d21c2d733bed90f
Gerrit-Change-Number: 10979
Gerrit-PatchSet: 3
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-6299: Use LlvmCodeGen's internal list of white-listed CPU attributes for handcrafting IRs
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10979 )
Change subject: IMPALA-6299: Use LlvmCodeGen's internal list of white-listed CPU attributes for handcrafting IRs
......................................................................
Patch Set 7: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/10979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifece8949c143146d2a1b38d72d21c2d733bed90f
Gerrit-Change-Number: 10979
Gerrit-PatchSet: 7
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 23 Jul 2018 20:55:10 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6299: Modify IRBuilder to use LLVM's CPU features
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10979 )
Change subject: IMPALA-6299: Modify IRBuilder to use LLVM's CPU features
......................................................................
Patch Set 2:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/10979/2/be/src/codegen/llvm-codegen-test.cc
File be/src/codegen/llvm-codegen-test.cc:
http://gerrit.cloudera.org:8080/#/c/10979/2/be/src/codegen/llvm-codegen-test.cc@467
PS2, Line 467: CpuInfo::EnableFeature(CpuInfo::SSE4_2, false);
Do we still need to toggle CpuInfo?
http://gerrit.cloudera.org:8080/#/c/10979/2/be/src/codegen/llvm-codegen.h
File be/src/codegen/llvm-codegen.h:
http://gerrit.cloudera.org:8080/#/c/10979/2/be/src/codegen/llvm-codegen.h@737
PS2, Line 737: current_cpu_attrs_
Maybe enabled_cpu_attrs_?
I'm also wondering if we need this additional map - can we just modify cpu_attrs_ for testing purposes?
On principle I'd prefer to keep the product code as simple as possible even if it makes the test code a little more complex.
http://gerrit.cloudera.org:8080/#/c/10979/2/be/src/codegen/llvm-codegen.h@745
PS2, Line 745: /// Mapping between CpuInfo flags and the corresponding strings.
Can you briefly document the key? Since it seems like you're doing something non-trivial with bitwise negation.
http://gerrit.cloudera.org:8080/#/c/10979/2/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/10979/2/be/src/codegen/llvm-codegen.cc@127
PS2, Line 127: const std::map<int64_t, std::string> LlvmCodeGen::cpu_flag_mappings_{
nit: we generally avoid std:: prefixes in .cc files. common/names.h actually imports a lot of these standard classes into the namespace.
http://gerrit.cloudera.org:8080/#/c/10979/2/be/src/util/bit-util-test.cc
File be/src/util/bit-util-test.cc:
http://gerrit.cloudera.org:8080/#/c/10979/2/be/src/util/bit-util-test.cc@325
PS2, Line 325: const int64_t CPU_INFO_FLAG = CpuInfo::SSSE3;
Either const or constexpr here seems fine - why the change?
http://gerrit.cloudera.org:8080/#/c/10979/2/be/src/util/cpu-info.cc
File be/src/util/cpu-info.cc:
http://gerrit.cloudera.org:8080/#/c/10979/2/be/src/util/cpu-info.cc@71
PS2, Line 71: const int64_t CpuInfo::SSSE3 = (1 << 1);
We still want to have the constant values visible in the header file so they can be inlined in other modules. You can do that and just define them here without the value, i.e.
const int64_t CpuInfo::SSE4_1;
Normally the benefit of inlining is minimal, but the CpuInfo checks are done on some very frequently-executed code paths.
--
To view, visit http://gerrit.cloudera.org:8080/10979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifece8949c143146d2a1b38d72d21c2d733bed90f
Gerrit-Change-Number: 10979
Gerrit-PatchSet: 2
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Jul 2018 00:31:11 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6299: Use LlvmCodeGen's internal list of white-listed CPU attributes for handcrafting IRs
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10979 )
Change subject: IMPALA-6299: Use LlvmCodeGen's internal list of white-listed CPU attributes for handcrafting IRs
......................................................................
Patch Set 7: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/10979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifece8949c143146d2a1b38d72d21c2d733bed90f
Gerrit-Change-Number: 10979
Gerrit-PatchSet: 7
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 23 Jul 2018 23:56:13 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6299: Use LlvmCodeGen's internal list of white-listed CPU attributes for handcrafting IRs
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10979 )
Change subject: IMPALA-6299: Use LlvmCodeGen's internal list of white-listed CPU attributes for handcrafting IRs
......................................................................
Patch Set 6: Code-Review+1
Will let Bikram verify that his comments were addressed.
--
To view, visit http://gerrit.cloudera.org:8080/10979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifece8949c143146d2a1b38d72d21c2d733bed90f
Gerrit-Change-Number: 10979
Gerrit-PatchSet: 6
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 23 Jul 2018 20:41:34 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6299: Use LlvmCodeGen's internal list of white-listed CPU attributes for handcrafting IRs
Posted by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org>.
Pooja Nilangekar has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/10979 )
Change subject: IMPALA-6299: Use LlvmCodeGen's internal list of white-listed CPU attributes for handcrafting IRs
......................................................................
IMPALA-6299: Use LlvmCodeGen's internal list of white-listed CPU attributes for handcrafting IRs
Previously, the IRBuilder of the LlvmCodeGen class used CpuInfo's
list of enabled features to determine the validity of certain
instructions to emit intrinsics. It did not consider the whitelist
which passed while initializing the LlvmCodeGen class. Now, the
IRBuilder inspects its own CPU attributes before emitting
instruction. This change also adds functionality to modify the CPU
attributes of the LlvmCodeGen class for testing.
Testing: Verified that the current tests which use and modify
CpuInfo produce expected results.
Change-Id: Ifece8949c143146d2a1b38d72d21c2d733bed90f
---
M be/src/benchmarks/bloom-filter-benchmark.cc
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/filter-context.cc
M be/src/util/cpu-info.cc
6 files changed, 95 insertions(+), 29 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/10979/6
--
To view, visit http://gerrit.cloudera.org:8080/10979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifece8949c143146d2a1b38d72d21c2d733bed90f
Gerrit-Change-Number: 10979
Gerrit-PatchSet: 6
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-6299: Modify IRBuilder to use LLVM's CPU features
Posted by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org>.
Pooja Nilangekar has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/10979 )
Change subject: IMPALA-6299: Modify IRBuilder to use LLVM's CPU features
......................................................................
IMPALA-6299: Modify IRBuilder to use LLVM's CPU features
Previously, the IRBuilder of the LlvmCodeGen class used CpuInfo's
list of enabled features to determine the validity of certain
instructions. It did not consider the whitelist which passed while
initilaizing the LlvmCodeGen class. Now, the IRBuilder inspects
its own cpu attributes before emitting instruction. This change
also adds functionality to modify the cpu attributes of the
LlvmCodeGen class for testing.
Testing: Verified that the current tests which use and modify
CpuInfo produce expected results.
Change-Id: Ifece8949c143146d2a1b38d72d21c2d733bed90f
---
M be/src/benchmarks/bloom-filter-benchmark.cc
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/filter-context.cc
M be/src/util/cpu-info.cc
6 files changed, 90 insertions(+), 22 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/10979/5
--
To view, visit http://gerrit.cloudera.org:8080/10979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifece8949c143146d2a1b38d72d21c2d733bed90f
Gerrit-Change-Number: 10979
Gerrit-PatchSet: 5
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-6299: Use LlvmCodeGen's internal list of white-listed CPU attributes for handcrafting IRs
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/10979 )
Change subject: IMPALA-6299: Use LlvmCodeGen's internal list of white-listed CPU attributes for handcrafting IRs
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/10979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifece8949c143146d2a1b38d72d21c2d733bed90f
Gerrit-Change-Number: 10979
Gerrit-PatchSet: 6
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 23 Jul 2018 20:54:43 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6299: Use LlvmCodeGen's internal list of white-listed CPU attributes for handcrafting IRs
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10979 )
Change subject: IMPALA-6299: Use LlvmCodeGen's internal list of white-listed CPU attributes for handcrafting IRs
......................................................................
Patch Set 6:
I cancelled the job while playing around - ignore the failure.
--
To view, visit http://gerrit.cloudera.org:8080/10979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifece8949c143146d2a1b38d72d21c2d733bed90f
Gerrit-Change-Number: 10979
Gerrit-PatchSet: 6
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 23 Jul 2018 20:39:16 +0000
Gerrit-HasComments: No