Fix failure of AVX2-accelerated mb_check_encoding on 32-bit MS Windows #10776
Conversation
| * (Bad 7/8-byte UTF-8 strings would be wrongly passed through as 'valid') | ||
| * It is not clear whether the root cause is a problem with MS Visual C++'s handling | ||
| * of AVX2 intrinsics when building 32-bit binaries, or something else */ | ||
| operand = _mm256_set_epi32(0, 0, 0, 0, 0, 0, ((int32_t*)p)[1], ((int32_t*)p)[0]); |
There was a problem hiding this comment.
same instruction is used in
php-src/ext/hash/xxhash/xxhash.h
Line 3895 in 7638640
There was a problem hiding this comment.
Indeed.
I don't know if the AVX2-accelerated code you are showing from xxhash is used on 32-bit Windows hosts or not. If it is, that indicates that using _mm256_set_epi64x is not necessarily a problem in of itself. Whatever the case, a root-cause analysis cannot be done without access to a test machine, and I don't have one. However, the change in this PR seems to have fixed a problem.
If anyone else has more insight into the issue at hand, I would love to hear their comments.
There was a problem hiding this comment.
Should we use an #if and keep using _mm256_set_epi64x for everything but Windows x86 32-bit?
There was a problem hiding this comment.
seems to be https://stackoverflow.com/questions/37509129/potential-bug-in-visual-studio-c-compiler-or-in-intel-intrinsics-avx2-mm256-s and xxhash should be fixed as well
There was a problem hiding this comment.
@mvorisek Good find, that explains it 🙂
There was a problem hiding this comment.
@mvorisek Great find!!
As a suggestion, I think the first thing would be to add a test case which exposes the problem on 32-bit Windows. Then it will be easy to verify the fix.
There was a problem hiding this comment.
Should we use an
#ifand keep using_mm256_set_epi64xfor everything but Windows x86 32-bit?
I don't think so.
If this particular line was performance-critical, then I would say yes, but I think the performance loss here will be minute. (If the compiler is smart enough to generate optimal code on x86-64, we won't lose anything.) The effect will only be on 7 and 8 byte UTF-8 strings, and for those, I expect the overhead of entering/exiting a function from PHP code will dominate this tiny bit of overhead.
Having a lot of #ifs in C code is a bad thing. I'm sure you know all the reasons why that is so.
There was a problem hiding this comment.
@iluuu1994 Please note, I would be happy to be proved wrong on the above statement.
There was a problem hiding this comment.
I wouldn't consider an #if an issue, but I'm fine either way 🙂
|
Hmm, well, the Windows x86 CI build has passed! @iluuu1994 Have a look! |
There was a problem hiding this comment.
@alexdowad Thanks a lot for having a look! that's very odd, but I don't know much about the peculiarities of Windows and don't own a Windows machine. The reason this wasn't previously caught by CI is that the new Windows job on GitHub actions tests intrinsics on 32-bit instead of 64-bit.
| * (Bad 7/8-byte UTF-8 strings would be wrongly passed through as 'valid') | ||
| * It is not clear whether the root cause is a problem with MS Visual C++'s handling | ||
| * of AVX2 intrinsics when building 32-bit binaries, or something else */ | ||
| operand = _mm256_set_epi32(0, 0, 0, 0, 0, 0, ((int32_t*)p)[1], ((int32_t*)p)[0]); |
There was a problem hiding this comment.
Should we use an #if and keep using _mm256_set_epi64x for everything but Windows x86 32-bit?
|
I think I should edit that StackOverflow link into this PR. It will be useful for others who work on this code down the road. |
Thanks to Ilija Tovilo for noticing and reporting this problem. Thanks also to Michael Voříšek for finding the StackOverflow post which explained the reason for the failure.
|
Landed on |
Similar bug as before in php#10776, but now in other code.
Not sure if this will do the trick, just opening a PR to see what happens in CI run...