Added windows cross compile builds & fixed build issues

[ashbob999]

• Added cross compiling builds (x64, x86, ARM64) for windows (MSVC), and fixed build issues.
• Added support for using MSVC ARM intrinsics.
• Added tests for sz_u32_clz which will help catch if the tests are run without BMI support.
• Fixed build issues when building MacOS Universal2 binaries.
• Disabled AVX for 32-bit binaries.
• Fixed lots of issues when building a Windows 32-bit binary.
• Updated release.yml to build x86/x64/arm64 windows versions, and included the .lib file in the archive.
• Added checks for making sure stringzillite is built without any dependencies.
• Fixed Windows stringzillite might be broken #185

[ashbob999]

@ashvardanian
For the x86 (32-bit) builds, there are warnings about conversion loss of data, due to converison between sz_size_t and sz_u64_t.

sz_sorted_idx_t is defined as a sz_u64_t.

Functions where this happens: sz_partition, sz_merge, sz_sort_introsort_recursion, sz_sort_partial.

Plus there are other places where the sz_u64_t are used directly, but only to be converted to sz_size_t.

StringZilla/include/stringzilla/stringzilla.h

Lines 3478 to 3487 in a34d836

	SZ_PUBLIC void sz_sort_insertion(sz_sequence_t *sequence, sz_sequence_comparator_t less) {
	sz_u64_t *keys = sequence->order;
	sz_size_t keys_count = sequence->count;
	for (sz_size_t i = 1; i < keys_count; i++) {
	sz_u64_t i_key = keys[i];
	sz_size_t j = i;
	for (; j > 0 && less(sequence, i_key, keys[j - 1]); --j) keys[j] = keys[j - 1];
	keys[j] = i_key;
	}
	}

Other functions it occurs in: _sz_sift_down, sz_sort_introsort_recursion, sz_string_unpack.

So should all of these be changed to use sz_size_t?

Also should we be running the NumPy tests when building the python binaries?

[ashvardanian]

Hey @ashbob999! Those places require the unsigned integer to be 64 bit, so it shouldn't be changed.

[ashbob999]

Hey @ashbob999! Those places require the unsigned integer to be 64 bit, so it shouldn't be changed.

Although, if I have understood the sz_sort_insertion function correctly and the sz_sequence_t struct.

That the order member is a pointer to an array of indexes, which specify the sorted order, and the count is the number of indexes in the array. Then don't we already have a mismatch, because we have a uint32 for the count and a uint64 for the index of each string?

And because the order pointer array is populated with the data ptr from a std::vector, which the vector max size is 32-bit.

[ashbob999]

@ashvardanian what should I do about the warnings for the 32-builds (for the sorting functions)?

[ashvardanian]

@ashbob999, what's the cleanest way to reproduce some of those warnings/errors with GCC/Clang? It would be easier to choose a path forward if I can better understand their nature.

[ashbob999]

@ashvardanian

Just building it as 32-bit (m32 and adding -Wconversion although this adds other warnings for clang) should be enough. Although I cannot seem to reproduce some of the warnings that MSVC emits.

For example the warning about converting the result of sz_blend_u64 doesn't get emitted even though it is truncating from u64 to u32.

StringZilla/include/stringzilla/stringzilla.h

Line 3162 in dd2b949

*space = sz_u64_blend(SZ_STRING_INTERNAL_SPACE, string->external.space, is_big_mask);

But only a very few actually get emitted (GCC).

[1/2] Building CXX object CMakeFiles/stringzilla_test_cpp14.dir/scripts/test.cpp.o
In file included from /usr/include/c++/11/cassert:44,
                 from ../include/stringzilla/stringzilla.hpp:57,
                 from ../scripts/test.cpp:21:
../scripts/test.cpp: In function ‘void test_sequence_algorithms()’:
../scripts/test.cpp:1347:90: warning: conversion from ‘__gnu_cxx::__alloc_traits<std::allocator<long long unsigned int>, long long unsigned int>::value_type’ {aka ‘long long unsigned int’} to ‘std::vector<std::__cxx11::basic_string<char> >::size_type’ {aka ‘unsigned int’} may change value [-Wconversion]
 1347 |             for (std::size_t i = 1; i != dataset_size; ++i) { assert(dataset[order[i - 1]] <= dataset[order[i]]); }
      |                                                                                          ^
../scripts/test.cpp:1347:111: warning: conversion from ‘__gnu_cxx::__alloc_traits<std::allocator<long long unsigned int>, long long unsigned int>::value_type’ {aka ‘long long unsigned int’} to ‘std::vector<std::__cxx11::basic_string<char> >::size_type’ {aka ‘unsigned int’} may change value [-Wconversion]
 1347 |             for (std::size_t i = 1; i != dataset_size; ++i) { assert(dataset[order[i - 1]] <= dataset[order[i]]); }
      |                                                                                                               ^
[2/2] Linking CXX executable stringzilla_test_cpp14

But for whatever reason (maybe my building of the 32-bit with GCC/Clang is broken) but if I build it as 64-bit but with sz_size_t and sz_ssize_t changed to be uint32_t and int32_t respectively, then I get the warnings that MSVC emitted.

[ashvardanian]

@ashbob999, great! For basic operations like sz_fill we can add and #if condition with a 32-bit variant inside. I'd avoid creating separate function definitions. sz_copy and sz_move may benefit as well. Any other APIs on your mind?

[ashbob999]

@ashvardanian okay, that's what I was going to do. As for other APIs we'll see once I fix sz_fill and other similar ones.

As for the 32-bit warnings, should we adjust them, and say there is a 4 billion limit for sorting?

[ashvardanian]

I believe it's already documented, that sorting over 4B entries is not currently supported. If not, we should mark that.

[ashbob999]

@ashvardanian that's fine, so it should then be fine to change the size of the values in the sorting functions to sz_size_t, because do correct me if i'm wrong but i don't remember seeing any logic that explicitly required 64-bit integer (as they are just used for indexes).

[ashbob999]

@ashvardanian, Somehow I missed that for sz_sort_partial we are adding the first 4 bytes from each string into the upper half of the sz_u64_t. Which would make sense why it's limited to 4 billion (although I couldn't find anything that said it is limited) so it needs to be a sz_u64_t.

Would it make sense to change the type of count in sz_sequence_t from a sz_size_t to sz_u32_t as on 64-bit platforms giving more would cause it to behave incorrectly. Although this would break ABI with previous versions.

Can you think of any better way of forcing the 4B limit on the user?

[ashvardanian]

Can you think of any better way of forcing the 4B limit on the user?

I would stick to size_t for sizes and focus on designing a better sorting algorithm. Long-term sustainable solutions are better than short-term gains 😉

[ashvardanian]

Hey @ashbob999! I'll have more time to look into this next week.

I want to thank you for following the git message style - it's very pleasing to see! Unlike most PRs, where I end up squashing the patches and merging under a new name, I would love to preserve your entire commit history. I'd just ask you to reformat/squash the last three commits that deviate from the style. Thanks again!

[ashbob999]

@ashvardanian no problem, I purposely did not make them that way jsut because they were fixing issues that I noticed from previous commits in the merge.

I can squash them into a single one, labelled as minor fixes (or equivalent), do you also want me to rebate to fix one in the middle a35cc50?