impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Youwei Wang (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-2809: improve ByteSwap with builtin function or SSE or AVX2
Date Sat, 21 May 2016 13:37:12 GMT
Youwei Wang has posted comments on this change.

Change subject: IMPALA-2809: improve ByteSwap with builtin function or SSE or AVX2
......................................................................


Patch Set 5:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/3081/5/be/src/benchmarks/bswap-benchmark.cc
File be/src/benchmarks/bswap-benchmark.cc:

Line 36: // Impala: the old Impala implementation.
> I'm sorry, but this is still confusing. What does "old" mean, here? Is Bswa
Old Impala implementation refers to some code has been discarded in previous patch. I reserve
a copy as the function ImpalaByteSwap in this source file. I will improve such description
as your request.


http://gerrit.cloudera.org:8080/#/c/3081/5/be/src/util/bit-util.inline.h
File be/src/util/bit-util.inline.h:

Line 161: template <int template_data_width>
> template params should be in caps, like a constant. We follow https://googl
Done


Line 167:     if(len >= 16) data_width = 16;
> Please put both branches of a conditional inside {}
Done


Line 174:   static const FuncPointer SIMDFuncTable[2] = {ByteSwapSSE_Unit, ByteSwapAVX2_Unit};
> I think you can make this an actual function parameter, which might compile
Hi Jim. If you don't mind, I just want to make sure that you suggest to make the variable
func_idx an actual function parameter? If so, I am afraid this is not the case since the func_idx
is used as an index to choose the correct function from the jump table "SIMDFuncTable". Maybe
we can still make it as an actual function parameter but it requires to unify the ByteSwapSSE_Unit
and ByteSwapSSE_Unit functions. I am afraid this is not practical according to our context.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I392ed5a8d5683f30f161282c228c1aedd7b648c1
Gerrit-PatchSet: 5
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Youwei Wang <429222616@qq.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Jim Apple <jbapple@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mmokhtar@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Youwei Wang <429222616@qq.com>
Gerrit-HasComments: Yes

Mime
View raw message