Skip to content

Commit e5acade

Browse files
maxbachmannmaxbachmann
committed
fix incorrect transposition calc in simd impl of Jaro
1 parent 0151750 commit e5acade

File tree

7 files changed

+95
-39
lines changed

7 files changed

+95
-39
lines changed

‎CHANGELOG.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,9 @@
77
### Changed
88
- changed many types in the interface from int64_t to size_t, since they can't be negative.
99

10+
### Fixed
11+
- fix incorrect transposition calculation in simd implementation of Jaro similarity
12+
1013
## [2.2.3] - 2023-11-02
1114
### Fixed
1215
- use _mm_malloc/_mm_free on macOS if aligned_alloc is unsupported

‎extras/rapidfuzz_amalgamated.hpp‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
// Licensed under the MIT License <http://opensource.org/licenses/MIT>.
22
// SPDX-License-Identifier: MIT
33
// RapidFuzz v1.0.2
4-
// Generated: 2023-12-24 18:34:14.416744
4+
// Generated: 2023-12-25 11:17:08.593362
55
// ----------------------------------------------------------
66
// This file is an amalgamation of multiple different files.
77
// You probably shouldn't edit it directly.
@@ -2425,7 +2425,7 @@ static inline native_simd<T> min32(const native_simd<T>& a, const native_simd<T>
24252425
static inline native_simd<uint8_t> sllv(const native_simd<uint8_t>& a,
24262426
const native_simd<uint8_t>& count_) noexcept
24272427
{
2428-
__m256i mask_hi = _mm256_set1_epi32(0xFF00FF00);
2428+
__m256i mask_hi = _mm256_set1_epi32(static_cast<int32_t>(0xFF00FF00));
24292429
__m256i multiplier_lut = _mm256_set_epi8(0, 0, 0, 0, 0, 0, 0, 0, char(128), 64, 32, 16, 8, 4, 2, 1, 0, 0,
24302430
0, 0, 0, 0, 0, 0, char(128), 64, 32, 16, 8, 4, 2, 1);
24312431

@@ -2448,7 +2448,7 @@ static inline native_simd<uint8_t> sllv(const native_simd<uint8_t>& a,
24482448
static inline native_simd<uint16_t> sllv(const native_simd<uint16_t>& a,
24492449
const native_simd<uint16_t>& count) noexcept
24502450
{
2451-
const __m256i mask = _mm256_set1_epi32(0xFFFF0000);
2451+
const __m256i mask = _mm256_set1_epi32(static_cast<int32_t>(0xFFFF0000));
24522452
__m256i low_half = _mm256_sllv_epi32(a, _mm256_andnot_si256(mask, count));
24532453
__m256i high_half = _mm256_sllv_epi32(_mm256_and_si256(mask, a), _mm256_srli_epi32(count, 16));
24542454
return _mm256_blend_epi16(low_half, high_half, 0xAA);
@@ -5988,8 +5988,9 @@ jaro_similarity_simd_long_s2(Range<double*> scores, const detail::BlockPatternMa
59885988

59895989
VecType PatternFlagMask = blsi(P_flag_cur);
59905990

5991-
uint64_t PM_j = block.get(
5992-
cur_block, s2[countr_zero(T_flag_cur) + T_word_index * sizeof(VecType) * 8]);
5991+
uint64_t PM_j =
5992+
block.get(cur_vec + cur_block,
5993+
s2[countr_zero(T_flag_cur) + T_word_index * sizeof(VecType) * 8]);
59935994
Transpositions += !(PM_j & (static_cast<uint64_t>(PatternFlagMask) << offset));
59945995

59955996
T_flag_cur = blsr(T_flag_cur);
@@ -6090,7 +6091,7 @@ jaro_similarity_simd_short_s2(Range<double*> scores, const detail::BlockPatternM
60906091
while (P_flag_cur) {
60916092
VecType PatternFlagMask = blsi(P_flag_cur);
60926093

6093-
uint64_t PM_j = block.get(cur_block, s2[countr_zero(T_flag_cur)]);
6094+
uint64_t PM_j = block.get(cur_vec + cur_block, s2[countr_zero(T_flag_cur)]);
60946095
Transpositions += !(PM_j & (static_cast<uint64_t>(PatternFlagMask) << offset));
60956096

60966097
T_flag_cur = blsr(T_flag_cur);

‎fuzzing/fuzz_jaro_similarity.cpp‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,11 +44,12 @@ void validate_simd(const std::basic_string<uint8_t>& s1, const std::basic_string
4444
double reference_sim = rapidfuzz_reference::jaro_similarity(strings[i], s2);
4545

4646
if (!is_close(simd_results[i], reference_sim, 0.0001)) {
47-
print_seq("s1", s1);
47+
print_seq("s1", strings[i]);
4848
print_seq("s2", s2);
4949
throw std::logic_error(std::string("jaro similarity using simd failed (reference_score = ") +
5050
std::to_string(reference_sim) + std::string(", score = ") +
51-
std::to_string(simd_results[i]) + ")");
51+
std::to_string(simd_results[i]) + std::string(", i = ") +
52+
std::to_string(i) + ")");
5253
}
5354
}
5455

‎fuzzing/fuzz_levenshtein_distance.cpp‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,11 +38,12 @@ void validate_simd(const std::basic_string<uint8_t>& s1, const std::basic_string
3838
for (size_t i = 0; i < strings.size(); ++i) {
3939
size_t reference_score = rapidfuzz_reference::levenshtein_distance(strings[i], s2);
4040
if (reference_score != simd_results[i]) {
41-
print_seq("s1: ", s1);
41+
print_seq("s1: ", strings[i]);
4242
print_seq("s2: ", s2);
4343
throw std::logic_error(std::string("levenshtein distance using simd failed (reference_score = ") +
4444
std::to_string(reference_score) + std::string(", score = ") +
45-
std::to_string(simd_results[i]) + ")");
45+
std::to_string(simd_results[i]) + std::string(", i = ") +
46+
std::to_string(i) + ")");
4647
}
4748
}
4849
#else

‎rapidfuzz/details/simd_avx2.hpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -601,7 +601,7 @@ static inline native_simd<T> min32(const native_simd<T>& a, const native_simd<T>
601601
static inline native_simd<uint8_t> sllv(const native_simd<uint8_t>& a,
602602
const native_simd<uint8_t>& count_) noexcept
603603
{
604-
__m256i mask_hi = _mm256_set1_epi32(0xFF00FF00);
604+
__m256i mask_hi = _mm256_set1_epi32(static_cast<int32_t>(0xFF00FF00));
605605
__m256i multiplier_lut = _mm256_set_epi8(0, 0, 0, 0, 0, 0, 0, 0, char(128), 64, 32, 16, 8, 4, 2, 1, 0, 0,
606606
0, 0, 0, 0, 0, 0, char(128), 64, 32, 16, 8, 4, 2, 1);
607607

@@ -624,7 +624,7 @@ static inline native_simd<uint8_t> sllv(const native_simd<uint8_t>& a,
624624
static inline native_simd<uint16_t> sllv(const native_simd<uint16_t>& a,
625625
const native_simd<uint16_t>& count) noexcept
626626
{
627-
const __m256i mask = _mm256_set1_epi32(0xFFFF0000);
627+
const __m256i mask = _mm256_set1_epi32(static_cast<int32_t>(0xFFFF0000));
628628
__m256i low_half = _mm256_sllv_epi32(a, _mm256_andnot_si256(mask, count));
629629
__m256i high_half = _mm256_sllv_epi32(_mm256_and_si256(mask, a), _mm256_srli_epi32(count, 16));
630630
return _mm256_blend_epi16(low_half, high_half, 0xAA);

‎rapidfuzz/distance/Jaro_impl.hpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -674,7 +674,7 @@ jaro_similarity_simd_long_s2(Range<double*> scores, const detail::BlockPatternMa
674674
VecType PatternFlagMask = blsi(P_flag_cur);
675675

676676
uint64_t PM_j = block.get(
677-
cur_block, s2[countr_zero(T_flag_cur) + T_word_index * sizeof(VecType) * 8]);
677+
cur_vec + cur_block, s2[countr_zero(T_flag_cur) + T_word_index * sizeof(VecType) * 8]);
678678
Transpositions += !(PM_j & (static_cast<uint64_t>(PatternFlagMask) << offset));
679679

680680
T_flag_cur = blsr(T_flag_cur);
@@ -775,7 +775,7 @@ jaro_similarity_simd_short_s2(Range<double*> scores, const detail::BlockPatternM
775775
while (P_flag_cur) {
776776
VecType PatternFlagMask = blsi(P_flag_cur);
777777

778-
uint64_t PM_j = block.get(cur_block, s2[countr_zero(T_flag_cur)]);
778+
uint64_t PM_j = block.get(cur_vec + cur_block, s2[countr_zero(T_flag_cur)]);
779779
Transpositions += !(PM_j & (static_cast<uint64_t>(PatternFlagMask) << offset));
780780

781781
T_flag_cur = blsr(T_flag_cur);

‎test/distance/tests-Jaro.cpp‎

Lines changed: 75 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -29,28 +29,40 @@ double jaro_similarity(const Sentence1& s1, const Sentence2& s2, double score_cu
2929
#ifdef RAPIDFUZZ_SIMD
3030
std::vector<double> results(256 / 8);
3131
if (s1.size() <= 8) {
32-
rapidfuzz::experimental::MultiJaro<8> simd_scorer(1);
33-
simd_scorer.insert(s1);
32+
rapidfuzz::experimental::MultiJaro<8> simd_scorer(32);
33+
for(size_t i = 0; i < 32; ++i)
34+
simd_scorer.insert(s1);
35+
3436
simd_scorer.similarity(&results[0], results.size(), s2, score_cutoff);
35-
REQUIRE(res1 == Approx(results[0]));
37+
for(size_t i = 0; i < 32; ++i)
38+
REQUIRE(res1 == Approx(results[i]));
3639
}
3740
if (s1.size() <= 16) {
38-
rapidfuzz::experimental::MultiJaro<16> simd_scorer(1);
39-
simd_scorer.insert(s1);
41+
rapidfuzz::experimental::MultiJaro<16> simd_scorer(16);
42+
for(size_t i = 0; i < 16; ++i)
43+
simd_scorer.insert(s1);
44+
4045
simd_scorer.similarity(&results[0], results.size(), s2, score_cutoff);
41-
REQUIRE(res1 == Approx(results[0]));
46+
for(size_t i = 0; i < 16; ++i)
47+
REQUIRE(res1 == Approx(results[i]));
4248
}
4349
if (s1.size() <= 32) {
44-
rapidfuzz::experimental::MultiJaro<32> simd_scorer(1);
45-
simd_scorer.insert(s1);
50+
rapidfuzz::experimental::MultiJaro<32> simd_scorer(8);
51+
for(size_t i = 0; i < 8; ++i)
52+
simd_scorer.insert(s1);
53+
4654
simd_scorer.similarity(&results[0], results.size(), s2, score_cutoff);
47-
REQUIRE(res1 == Approx(results[0]));
55+
for(size_t i = 0; i < 8; ++i)
56+
REQUIRE(res1 == Approx(results[i]));
4857
}
4958
if (s1.size() <= 64) {
50-
rapidfuzz::experimental::MultiJaro<64> simd_scorer(1);
51-
simd_scorer.insert(s1);
59+
rapidfuzz::experimental::MultiJaro<64> simd_scorer(4);
60+
for(size_t i = 0; i < 4; ++i)
61+
simd_scorer.insert(s1);
62+
5263
simd_scorer.similarity(&results[0], results.size(), s2, score_cutoff);
53-
REQUIRE(res1 == Approx(results[0]));
64+
for(size_t i = 0; i < 4; ++i)
65+
REQUIRE(res1 == Approx(results[i]));
5466
}
5567
#endif
5668

@@ -87,28 +99,40 @@ double jaro_distance(const Sentence1& s1, const Sentence2& s2, double score_cuto
8799
#ifdef RAPIDFUZZ_SIMD
88100
std::vector<double> results(256 / 8);
89101
if (s1.size() <= 8) {
90-
rapidfuzz::experimental::MultiJaro<8> simd_scorer(1);
91-
simd_scorer.insert(s1);
102+
rapidfuzz::experimental::MultiJaro<8> simd_scorer(32);
103+
for(size_t i = 0; i < 32; ++i)
104+
simd_scorer.insert(s1);
105+
92106
simd_scorer.distance(&results[0], results.size(), s2, score_cutoff);
93-
REQUIRE(res1 == Approx(results[0]));
107+
for(size_t i = 0; i < 32; ++i)
108+
REQUIRE(res1 == Approx(results[i]));
94109
}
95110
if (s1.size() <= 16) {
96-
rapidfuzz::experimental::MultiJaro<16> simd_scorer(1);
97-
simd_scorer.insert(s1);
111+
rapidfuzz::experimental::MultiJaro<16> simd_scorer(16);
112+
for(size_t i = 0; i < 16; ++i)
113+
simd_scorer.insert(s1);
114+
98115
simd_scorer.distance(&results[0], results.size(), s2, score_cutoff);
99-
REQUIRE(res1 == Approx(results[0]));
116+
for(size_t i = 0; i < 16; ++i)
117+
REQUIRE(res1 == Approx(results[i]));
100118
}
101119
if (s1.size() <= 32) {
102-
rapidfuzz::experimental::MultiJaro<32> simd_scorer(1);
103-
simd_scorer.insert(s1);
120+
rapidfuzz::experimental::MultiJaro<32> simd_scorer(8);
121+
for(size_t i = 0; i < 8; ++i)
122+
simd_scorer.insert(s1);
123+
104124
simd_scorer.distance(&results[0], results.size(), s2, score_cutoff);
105-
REQUIRE(res1 == Approx(results[0]));
125+
for(size_t i = 0; i < 8; ++i)
126+
REQUIRE(res1 == Approx(results[i]));
106127
}
107128
if (s1.size() <= 64) {
108-
rapidfuzz::experimental::MultiJaro<64> simd_scorer(1);
109-
simd_scorer.insert(s1);
129+
rapidfuzz::experimental::MultiJaro<64> simd_scorer(4);
130+
for(size_t i = 0; i < 4; ++i)
131+
simd_scorer.insert(s1);
132+
110133
simd_scorer.distance(&results[0], results.size(), s2, score_cutoff);
111-
REQUIRE(res1 == Approx(results[0]));
134+
for(size_t i = 0; i < 4; ++i)
135+
REQUIRE(res1 == Approx(results[i]));
112136
}
113137
#endif
114138

@@ -197,4 +221,30 @@ TEST_CASE("JaroTest")
197221
"0000000000000000000000000000000000000000000000000000000000")) ==
198222
Approx(0.8359375));
199223
}
200-
}
224+
225+
SECTION("testFuzzingRegressions")
226+
{
227+
#ifdef RAPIDFUZZ_SIMD
228+
{
229+
std::string s2 = "010101010101010101010101010101010101010101010101010101010101010101"
230+
"010101010101010101010101010101010101010101010101010101010101010101"
231+
"010101010101010101010101010101010101010101010101010101010101010101"
232+
"0101010101010101010101010101010101010101010101010101010101";
233+
234+
std::vector<double> results(512 / 8);
235+
rapidfuzz::experimental::MultiJaro<8> simd_scorer(64);
236+
for(size_t i = 0; i < 32; ++i)
237+
simd_scorer.insert("10010010");
238+
239+
simd_scorer.insert("00100100");
240+
simd_scorer.similarity(&results[0], results.size(), s2);
241+
242+
for(size_t i = 0; i < 32; ++i)
243+
REQUIRE(results[i] == Approx(0.593750));
244+
245+
REQUIRE(results[32] == Approx(0.593750));
246+
}
247+
#endif
248+
249+
}
250+
}

0 commit comments

Comments
 (0)