Wrong code for avx512 intrinsic

[rockeet]

#include <immintrin.h>
using byte_t = unsigned char;
size_t avx512_search_byte_max32(const byte_t* data, size_t len, byte_t key) {
	return _tzcnt_u32(_mm256_mask_cmpge_epi8_mask(_bzhi_u32(-1, len),
                            *(__m256i_u*)data, _mm256_set1_epi8(key)));
}

see here, clang generate:

avx512_search_byte_max32(unsigned char const*, unsigned long, unsigned char):
        vpbroadcastb    ymm0, edx
        mov     eax, -1
        bzhi    eax, eax, esi
        vpcmpleb        k0, ymm0, ymmword ptr [rdi]
        kmovd   ecx, k0
        and     ecx, eax
        tzcnt   eax, ecx
        vzeroupper
        ret

intel icc generate:

avx512_search_byte_max32(unsigned char const*, unsigned long, unsigned char):
..B3.1:                         # Preds ..B3.0
        mov       eax, -1                                       #20.20
        vpbroadcastb ymm0, edx                                  #20.20
        bzhi      ecx, eax, esi                                 #20.20
        kmovd     k1, ecx                                       #20.20
        vpcmpb    k0{k1}, ymm0, YMMWORD PTR [rdi], 2            #20.20
        kmovd     r8d, k0                                       #20.20
        tzcnt     eax, r8d                                      #20.9
        vzeroupper                                              #20.9
        ret                                                     #20.9

The issue is: if [data, data+32) spans page boundary and bytes corresponding to 0 in mask is invalid address(such as segfault/bus error), code generated by clang will coredump.

[llvmbot]

@llvm/issue-subscribers-backend-x86

Author: rockeet (rockeet)

```c++ #include <immintrin.h> using byte_t = unsigned char; size_t avx512_search_byte_max32(const byte_t* data, size_t len, byte_t key) { return _tzcnt_u32(_mm256_mask_cmpge_epi8_mask(_bzhi_u32(-1, len), *(__m256i_u*)data, _mm256_set1_epi8(key))); } ``` [see here](https://godbolt.org/z/zMGKdfGzY), clang generate: ```nasm avx512_search_byte_max32(unsigned char const*, unsigned long, unsigned char): vpbroadcastb ymm0, edx mov eax, -1 bzhi eax, eax, esi vpcmpleb k0, ymm0, ymmword ptr [rdi] kmovd ecx, k0 and ecx, eax tzcnt eax, ecx vzeroupper ret ``` intel icc generate: ```nasm avx512_search_byte_max32(unsigned char const*, unsigned long, unsigned char): ..B3.1: # Preds ..B3.0 mov eax, -1 #20.20 vpbroadcastb ymm0, edx #20.20 bzhi ecx, eax, esi #20.20 kmovd k1, ecx #20.20 vpcmpb k0{k1}, ymm0, YMMWORD PTR [rdi], 2 #20.20 kmovd r8d, k0 #20.20 tzcnt eax, r8d #20.9 vzeroupper #20.9 ret #20.9 ```

The issue is: if [data, data+32) spans page boundary and bytes corresponding to 0 in mask is invalid address(such as segfault/bus error), code generated by clang will coredump.

[abhishek-kaushik22]

@phoebewang @e-kud does this intrinsic _mm256_mask_cmpge_epi8_mask imply that the inputs loads are masked, or is that up to the user to make sure that the loads are masked correctly with something like _mm256_maskz_loadu_epi8?
For the case where both operands are loaded from memory, even ICC generates an unmasked load which will have the same problem as this one. https://godbolt.org/z/YEExxsj84

__mmask32 foo(__mmask32 k1, const byte_t* a, const byte_t* b) {
    return _mm256_mask_cmpge_epi8_mask (k1, *(__m256i_u*)a, *(__m256i_u*)b);
}

foo(unsigned int, unsigned char const*, unsigned char const*):
..B1.1:                         # Preds ..B1.0
        vmovdqu   ymm0, YMMWORD PTR [rsi]                       #10.12
        kmovd     k1, edi                                       #10.12
        vpcmpb    k0{k1}, ymm0, YMMWORD PTR [rdx], 5            #10.12
        kmovd     eax, k0                                       #10.12
        vzeroupper                                              #10.12
        ret 

[phoebewang]

No. The *(__m256i_u*)data indicates it's a full range load. If you want to a masked load, use masked load intrinsics directly like https://godbolt.org/z/K16PMMG73

[rockeet]

GCC also has this bug and GCC accepted the bug and renamed as Not fusim unaligned load into cmp with mask for avx512 intrinsic.

[phoebewang]

GCC considers it as a missed optimization that folding load into the compare. LLVM has folded it already.

[rockeet]

GCC considers it as a missed optimization that folding load into the compare. LLVM has folded it already.

Yes, clang folded load into compare, but does not use mask compare, instead it use full compare then compute the final mask by bit and -- this is semantically correct for *(__m256i_u*)data + mask compare but lose "Masked Fault Suppression" property.

When explicitly using a mask load and a mask compare, clang does not fold the two into a mask compare with mem oprand(see https://godbolt.org/z/8r1Ycv7K1):

size_t avx512_search_byte_max32_2(const byte_t* data, size_t len, byte_t key) {
	__mmask32 k = _bzhi_u32(-1, len);
	__m256i d = _mm256_maskz_loadu_epi8(k, data); // Masked Fault Suppression
	return _tzcnt_u32(_mm256_mask_cmpeq_epi8_mask(k, d, _mm256_set1_epi8(key)));
}