impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jim Apple (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4300: Speed up BloomFilter::Or with SIMD
Date Sun, 23 Oct 2016 19:51:56 GMT
Jim Apple has posted comments on this change.

Change subject: IMPALA-4300: Speed up BloomFilter::Or with SIMD

Patch Set 1:

File be/src/util/

Line 167
> One reason auto-vectorisation must fail here is that the two directories ma
What's stringe is that without the pragma and without __restrict__, Matt Godbolt's compiler
explorer still shows the loop being autovectorized.

I tried loop unrolling, but it still didn't vectorize.

PS1, Line 162: DCHECK

Line 163:   auto simd_in = reinterpret_cast<const double*>(in);
> I think double*/const double* is more readable than auto here - I'm used to

PS1, Line 166: int
> Use size_t for consistency? I had to think about whether the implicit casts

PS1, Line 168: _mm256_loadu_pd
> We should be able to use the aligned versions here, right, since we allocat
BloomFilters do, but TBloomFilters do not, and from using gdb I see that they are not 32-byte

PS1, Line 181: should
> It shouldn't actually with the code in it's current form since it has to as
Let's continue conversation on overlapping at the other comment.

Line 184:   // TODO: Tune gcc flags to auto-vectorize. This might not be possible.
> See my other comment - should be possible.
Let's continue conversation on that comment.

Line 186:   // TODO: IMPALA-4312: Evaluate clang for builds other than ASAN.
> Just remove this TODO? I don't think it's informative for people trying to 

Line 190:     auto simd_in = reinterpret_cast<const __m128i*>(&[0]);
> This code is simple enough that I'm fine with moving forward with it even i
I will leave as-is.

PS1, Line 191: auto
> I think just repeating the types const__m128i* or __m128i* is more readable

PS1, Line 192: auto
> I'd prefer the int type was explicit here.

PS1, Line 196: int
> size_t for consistency?

PS1, Line 199: _mm_storeu_si128
> We should be able to use the aligned versions here too.
Let's continue this conversation above.

FWIW, i also tried some very complex methods of using non-simd ops to move both pointers forward
until at least one was aligned. The performance was sometimes worse, so I put that aside for
another day.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I840799d9cfb81285c796e2abfe2029bb869b0f67
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <>
Gerrit-Reviewer: Jim Apple <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message