ext/standard: Remove useless _mm_movemask_epi8 check in rot13 - #23795
LamentXU123 wants to merge 1 commit into
Conversation
| gt = _mm_cmpgt_epi8(in, a_minus_1); | ||
| lt = _mm_cmplt_epi8(in, m_plus_1); | ||
| cmp = _mm_and_si128(lt, gt); | ||
| if (_mm_movemask_epi8(cmp)) { | ||
| cmp = _mm_and_si128(cmp, add); | ||
| delta = _mm_or_si128(delta, cmp); | ||
| } | ||
| cmp = _mm_and_si128(cmp, add); | ||
| delta = _mm_or_si128(delta, cmp); | ||
|
|
||
| gt = _mm_cmpgt_epi8(in, n_minus_1); | ||
| lt = _mm_cmplt_epi8(in, z_plus_1); | ||
| cmp = _mm_and_si128(lt, gt); | ||
| if (_mm_movemask_epi8(cmp)) { | ||
| cmp = _mm_and_si128(cmp, sub); | ||
| delta = _mm_or_si128(delta, cmp); | ||
| } | ||
| cmp = _mm_and_si128(cmp, sub); | ||
| delta = _mm_or_si128(delta, cmp); | ||
|
|
||
| gt = _mm_cmpgt_epi8(in, A_minus_1); | ||
| lt = _mm_cmplt_epi8(in, M_plus_1); | ||
| cmp = _mm_and_si128(lt, gt); | ||
| if (_mm_movemask_epi8(cmp)) { | ||
| cmp = _mm_and_si128(cmp, add); | ||
| delta = _mm_or_si128(delta, cmp); | ||
| } | ||
| cmp = _mm_and_si128(cmp, add); | ||
| delta = _mm_or_si128(delta, cmp); | ||
|
|
||
| gt = _mm_cmpgt_epi8(in, N_minus_1); | ||
| lt = _mm_cmplt_epi8(in, Z_plus_1); | ||
| cmp = _mm_and_si128(lt, gt); | ||
| if (_mm_movemask_epi8(cmp)) { | ||
| cmp = _mm_and_si128(cmp, sub); | ||
| delta = _mm_or_si128(delta, cmp); | ||
| } | ||
| cmp = _mm_and_si128(cmp, sub); | ||
| delta = _mm_or_si128(delta, cmp); |
There was a problem hiding this comment.
c | 0x20 maps A-Z onto a-z and moves nothing else into that range (it only touches bit 5, and bytes >= 0x80 stay negative under the signed compare), so the four range tests collapse into one plus a midpoint test:
const __m128i lowbit = _mm_set1_epi8(0x20);
const __m128i a_minus_1 = _mm_set1_epi8('a' - 1);
const __m128i z_plus_1 = _mm_set1_epi8('z' + 1);
const __m128i n = _mm_set1_epi8('n');
const __m128i add = _mm_set1_epi8(13);
const __m128i sub = _mm_set1_epi8(-13);
do {
__m128i in = _mm_loadu_si128((__m128i *)p);
__m128i low = _mm_or_si128(in, lowbit);
__m128i alpha = _mm_and_si128(_mm_cmpgt_epi8(low, a_minus_1), _mm_cmplt_epi8(low, z_plus_1));
__m128i first = _mm_cmplt_epi8(low, n);
__m128i delta = _mm_and_si128(_mm_or_si128(_mm_and_si128(first, add), _mm_andnot_si128(first, sub)), alpha);
_mm_storeu_si128((__m128i *)target, _mm_add_epi8(in, delta));
p += 16;
target += 16;
} while (e - p > 15);gcc 11.4 -O2: the compiled loop body goes from 28 SIMD instructions to 15, and the 4-deep serial OR chain on delta goes away. Best of 7 over 1 MiB against your version: 1.9x on all-lowercase, 1.9x on digits, 2.4x on mixed case. Byte-identical to the scalar fallback for every byte value in every lane.
Separately, for the description: 1.2-1.3x is the floor, not the typical case. Random bytes at 1 MiB measure 7.9x against master here, since the four branches mispredict. Small buffers hide that, the predictor just memorizes a short repeating pattern.
The _mm_movemask_epi8 if-branch seems useless. From comparing, the only possible cases for each byte is 0x00 and 0xFF. So just perform the
andandorcommand anyways is always correct.When the mask aren't all zero, removing them is certainly faster cuz we save a _mm_movemask_epi8 call. When it is all zero, from my limited knowledge I think the
pandplusporcommand is always faster thanpmovmskb(plus test + jcc if that counts). But I might be wrong.Here is the asm diff:
Orginal
This version
So. I think this if-branch "optimization" actually made things slower. Note that we are checking this four times in a loop.
I tested this on my personal PC and the improvements are quite stable. ~1.2x-1.3x and in the worse case I don't see anything negative appears. But note that my env is kind of noisy. I don't know much on this perhaps I need help from some expert. @ndossche ?