[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.

2016-09-01 Thread Internal Jenkins (Code Review) 
Internal Jenkins has posted comments on this change.

Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
..


Patch Set 6: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Youwei Wang 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.

2016-09-01 Thread Internal Jenkins (Code Review) 
Internal Jenkins has submitted this change and it was merged.

Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
..


IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.

This changes the code to use the lddqu and movdqu instructions (via
Intel intrinsics) to allow unaligned memory access.

Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Reviewed-on: http://gerrit.cloudera.org:8080/4205
Reviewed-by: Tim Armstrong 
Reviewed-by: Dan Hecht 
Tested-by: Internal Jenkins
---
M be/src/exprs/expr-test.cc
M be/src/util/bit-util-test.cc
M be/src/util/bit-util.cc
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
4 files changed, 43 insertions(+), 13 deletions(-)

Approvals:
  Internal Jenkins: Verified
  Dan Hecht: Looks good to me, approved
  Tim Armstrong: Looks good to me, but someone else must approve



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Youwei Wang

[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.

2016-09-01 Thread Tim Armstrong (Code Review) 
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
..


Patch Set 6:

See https://gerrit.cloudera.org/#/c/4290/ for the benchmark

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Youwei Wang 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.

2016-09-01 Thread Dan Hecht (Code Review) 
Dan Hecht has posted comments on this change.

Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
..


Patch Set 6: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Youwei Wang 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.

2016-09-01 Thread Tim Armstrong (Code Review) 
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
..


Patch Set 6: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Youwei Wang 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.

2016-09-01 Thread Tim Armstrong (Code Review) 
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
..


Patch Set 5: Code-Review+1

(1 comment)

I also extended the microbenchmark to test perf when loads and stores were 
actually misaligned (not just using the different instruction). AVX2 is still 
significantly faster than SSE. I'll post a CR with that benchmark change 
separately.

I0901 13:47:02.025802  4956 bswap-benchmark.cc:122] Machine Info: Intel(R) 
Core(TM) i7-4790 CPU @ 3.60GHz
ByteSwap benchmark alignment=0:Function  iters/ms   10%ile   50%ile   
90%ile 10%ile 50%ile 90%ile
 
(relative) (relative) (relative)

-
 FastScalar881 1.08e+03 1.14e+03
 1X 1X 1X
  SSSE3   9.33e+03 1.02e+04 1.03e+04
  10.6X  9.46X  9.03X
   AVX2   3.43e+04 3.78e+04 3.84e+04
  38.9X  35.2X  33.7X
   SIMD   3.27e+04 3.78e+04 3.83e+04
  37.1X  35.2X  33.6X
ByteSwap benchmark alignment=1:Function  iters/ms   10%ile   50%ile   
90%ile 10%ile 50%ile 90%ile
 
(relative) (relative) (relative)

-
 FastScalar  1e+03 1.08e+03 1.15e+03
 1X 1X 1X
  SSSE3   8.67e+03 9.01e+03 9.11e+03
  8.66X  8.31X  7.95X
   AVX2   2.62e+04 2.75e+04 2.77e+04
  26.2X  25.4X  24.2X
   SIMD   2.56e+04 2.72e+04 2.75e+04
  25.6X  25.1X24X

http://gerrit.cloudera.org:8080/#/c/4205/5/be/src/util/bit-util-test.cc
File be/src/util/bit-util-test.cc:

PS5, Line 116: o
or


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Youwei Wang 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.

2016-09-01 Thread Jim Apple (Code Review) 
Jim Apple has uploaded a new patch set (#6).

Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
..

IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.

This changes the code to use the lddqu and movdqu instructions (via
Intel intrinsics) to allow unaligned memory access.

Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
---
M be/src/exprs/expr-test.cc
M be/src/util/bit-util-test.cc
M be/src/util/bit-util.cc
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
4 files changed, 43 insertions(+), 13 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/05/4205/6
-- 
To view, visit http://gerrit.cloudera.org:8080/4205
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Youwei Wang

[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.

2016-09-01 Thread Dan Hecht (Code Review) 
Dan Hecht has posted comments on this change.

Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
..


Patch Set 5:

How about adding an end-to-end case in expr.test (or somewhere) especially 
since this bug is more about how the environment is set up when invoking the 
routine so would be good to exercise it in the real environment (as opposed to 
the routine semantics).

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Youwei Wang 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.

2016-09-01 Thread Tim Armstrong (Code Review) 
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
..


Patch Set 5:

We don't support parquet decimals wider than 16 bytes so I don't think the AVX2 
routine would have kicked in there.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Youwei Wang 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.

2016-09-01 Thread Dan Hecht (Code Review) 
Dan Hecht has posted comments on this change.

Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
..


Patch Set 5:

Clearly we were missing some basic test coverage. Do we have any other test 
gaps around this code?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Youwei Wang 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.

2016-09-01 Thread Youwei Wang (Code Review) 
Youwei Wang has posted comments on this change.

Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4205/3/be/src/util/bit-util.cc
File be/src/util/bit-util.cc:

Line 161:   _mm_storeu_si128(reinterpret_cast<__m128i*>(dst + 16), part1);
> What's the perf impact?  I assume we still benefit from specializing this r
Greetings All.
@Dan I have conducted the performance test using bswap-benchmark. There is no 
performance loss comparing the Impala-2809. And even I got slight performance 
improvement using these new code. (_mm_storeu_si128/_mm_loadu_si128)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Youwei Wang 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.

2016-09-01 Thread Jim Apple (Code Review) 
Jim Apple has posted comments on this change.

Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
..


Patch Set 5:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4205/3/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:

Line 1931:   TestStringValue("reverse('abcdefg')", "gfedcba");
> I think let's just make it a separate test, the long functions are fine whe
Done in PS 5.


http://gerrit.cloudera.org:8080/#/c/4205/3/be/src/util/bit-util.cc
File be/src/util/bit-util.cc:

Line 161:   _mm_storeu_si128(reinterpret_cast<__m128i*>(dst + 16), part1);
> What's the perf impact?  I assume we still benefit from specializing this r
I ran the benchmark and the AVX2 version is still >3x faster than the SSSE3 
version.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Youwei Wang 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.

2016-09-01 Thread Jim Apple (Code Review) 
Jim Apple has uploaded a new patch set (#5).

Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
..

IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.

This changes the code to use the lddqu and movdqu instructions (via
Intel intrinsics) to allow unaligned memory access.

Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
---
M be/src/exprs/expr-test.cc
M be/src/util/bit-util-test.cc
M be/src/util/bit-util.cc
3 files changed, 31 insertions(+), 13 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/05/4205/5
-- 
To view, visit http://gerrit.cloudera.org:8080/4205
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Youwei Wang

[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.

2016-09-01 Thread Jim Apple (Code Review) 
Jim Apple has uploaded a new patch set (#4).

Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
..

IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.

This changes the code to use the lddqu and movdqu instructions (via
Intel intrinsics) to allow unaligned memory access.

Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
---
M be/src/exprs/expr-test.cc
M be/src/util/bit-util-test.cc
M be/src/util/bit-util.cc
3 files changed, 32 insertions(+), 13 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/05/4205/4
-- 
To view, visit http://gerrit.cloudera.org:8080/4205
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Youwei Wang

[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.

2016-09-01 Thread Tim Armstrong (Code Review) 
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4205/3/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:

Line 1931:   {
> Just for scoping so MAX_LEN, to_reverse and reversed don't conflict with an
I think let's just make it a separate test, the long functions are fine when 
the tests are one-liners but when they have some setup, etc it gets hard to 
read (that's also all over the place in this file but I don't see a good reason 
to emulate it).


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Youwei Wang 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.

2016-09-01 Thread Dan Hecht (Code Review) 
Dan Hecht has posted comments on this change.

Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4205/3/be/src/util/bit-util.cc
File be/src/util/bit-util.cc:

Line 161:   _mm_storeu_si128(reinterpret_cast<__m128i*>(dst + 16), part1);
> movdqa
What's the perf impact?  I assume we still benefit from specializing this 
routine for AVX2?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Youwei Wang 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.

2016-09-01 Thread Jim Apple (Code Review) 
Jim Apple has posted comments on this change.

Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
..


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4205/3/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:

Line 1931:   {
> Why the nested block? Should probably just be a separate function if we wan
Just for scoping so MAX_LEN, to_reverse and reversed don't conflict with 
anything.

For better or worse, this file includes very long functions. I was trying to 
match that style without polluting the space of local variable names.


http://gerrit.cloudera.org:8080/#/c/4205/3/be/src/util/bit-util.cc
File be/src/util/bit-util.cc:

Line 161:   _mm_storeu_si128(reinterpret_cast<__m128i*>(dst + 16), part1);
> what instructions did the compiler emit before this change?
movdqa


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Youwei Wang 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.

2016-09-01 Thread Dan Hecht (Code Review) 
Dan Hecht has posted comments on this change.

Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4205/3/be/src/util/bit-util.cc
File be/src/util/bit-util.cc:

Line 161:   _mm_storeu_si128(reinterpret_cast<__m128i*>(dst + 16), part1);
what instructions did the compiler emit before this change?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Youwei Wang 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.

2016-09-01 Thread Tim Armstrong (Code Review) 
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4205/3/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:

Line 1931:   {
Why the nested block? Should probably just be a separate function if we want to 
separate it.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Youwei Wang 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.

2016-09-01 Thread Jim Apple (Code Review) 
Jim Apple has posted comments on this change.

Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4205/3/be/src/util/bit-util-test.cc
File be/src/util/bit-util-test.cc:

Line 116:   // IMPALA-4058: test that bswap_fptr works when it reads to o 
writes from memory with
> What was the result of this test before the fix? A crash?
Yes - a segfault.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Youwei Wang 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.

2016-09-01 Thread Tim Armstrong (Code Review) 
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4205/3/be/src/util/bit-util-test.cc
File be/src/util/bit-util-test.cc:

Line 116:   // IMPALA-4058: test that bswap_fptr works when it reads to o 
writes from memory with
What was the result of this test before the fix? A crash?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Youwei Wang 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.

2016-09-01 Thread Jim Apple (Code Review) 
Jim Apple has posted comments on this change.

Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
..


Patch Set 2:

> > > Greetings, All.
 > > > @Jim, considering the string reverse function is using the
 > SIMDed
 > > > Byteswap as internal implementation, do you thinkt it is okay
 > to
 > > > merge some test cases from this? 
 > > > https://gerrit.cloudera.org/#/c/4204/1/be/src/exprs/expr-test.cc
 > > > Thank you :D
 > >
 > > What do you mean by "merge some test cases from"?
 > 
 > Hi Jim. I have added some extra test cases for the function
 > "reverse" in the file expr-test.cc: (line 1933-1946)
 > https://gerrit.cloudera.org/#/c/4204/1/be/src/exprs/expr-test.cc
 > 
 > If you don't mind, would you please take a look at that and then
 > decide whether to add those test cases or not? Thank you :)

Done

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Youwei Wang 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.

2016-09-01 Thread Jim Apple (Code Review) 
Jim Apple has uploaded a new patch set (#3).

Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
..

IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.

This changes the code to use the lddqu and movdqu instructions (via
Intel intrinsics) to allow unaligned memory access.

Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
---
M be/src/exprs/expr-test.cc
M be/src/util/bit-util-test.cc
M be/src/util/bit-util.cc
3 files changed, 29 insertions(+), 12 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/05/4205/3
-- 
To view, visit http://gerrit.cloudera.org:8080/4205
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Youwei Wang

[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.

2016-09-01 Thread Youwei Wang (Code Review) 
Youwei Wang has posted comments on this change.

Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
..


Patch Set 2: -Code-Review

> > Greetings, All.
 > > @Jim, considering the string reverse function is using the SIMDed
 > > Byteswap as internal implementation, do you thinkt it is okay to
 > > merge some test cases from this? 
 > > https://gerrit.cloudera.org/#/c/4204/1/be/src/exprs/expr-test.cc
 > > Thank you :D
 > 
 > What do you mean by "merge some test cases from"?

Hi Jim. I have added some extra test cases for the function "reverse" in the 
file expr-test.cc: (line 1933-1946)
https://gerrit.cloudera.org/#/c/4204/1/be/src/exprs/expr-test.cc

If you don't mind, would you please take a look at that and then decide whether 
to add those test cases or not? Thank you :)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Youwei Wang 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.

2016-09-01 Thread Jim Apple (Code Review) 
Jim Apple has posted comments on this change.

Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
..


Patch Set 2:

> Greetings, All.
 > @Jim, considering the string reverse function is using the SIMDed
 > Byteswap as internal implementation, do you thinkt it is okay to
 > merge some test cases from this? 
 > https://gerrit.cloudera.org/#/c/4204/1/be/src/exprs/expr-test.cc
 > Thank you :D

What do you mean by "merge some test cases from"?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Youwei Wang 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.

2016-09-01 Thread Youwei Wang (Code Review) 
Youwei Wang has posted comments on this change.

Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
..


Patch Set 2: Code-Review+1

Greetings, All.
@Jim, considering the string reverse function is using the SIMDed Byteswap as 
internal implementation, do you thinkt it is okay to merge some test cases from 
this? https://gerrit.cloudera.org/#/c/4204/1/be/src/exprs/expr-test.cc
Thank you :D

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Youwei Wang 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.

2016-09-01 Thread Jim Apple (Code Review) 
Jim Apple has uploaded a new patch set (#2).

Change subject: IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.
..

IMPALA-4058: ByteSwap256 assumed memory was 16-byte aligned.

This changes the code to use the lddqu and movdqu instructions (via
Intel intrinsics) to allow unaligned memory access.

Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
---
M be/src/util/bit-util-test.cc
M be/src/util/bit-util.cc
2 files changed, 20 insertions(+), 12 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/05/4205/2
-- 
To view, visit http://gerrit.cloudera.org:8080/4205
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I39b2b47bb717d5ac9727512a24fcf8a8a6a8dcc6
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Youwei Wang