Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

There is a problem with max and min #349

Open
wjr-z opened this issue Dec 9, 2022 · 4 comments · Fixed by #350
Open

There is a problem with max and min #349

wjr-z opened this issue Dec 9, 2022 · 4 comments · Fixed by #350

Comments

@wjr-z
Copy link

wjr-z commented Dec 9, 2022

最新的Vc版本的Vc/sse/Detail.h中看到min(__m128i,uchar)和max(__m128i,uchar)中出现了问题,函数中应该使用的是min(__m128i,__m128i,uchar())或max(__m128i,__m128i,uchar()),而实际使用的是schar()
测试如下:
https://godbolt.org/z/eEYjYTrnf

@bernhardmgruber
Copy link
Collaborator

No worries, I could translate! I will answer in English, so you can pick your favorite translation engine!

I can confirm, this is a bug in the implementation of uchar Vc::Detail::max(__m128i a, uchar). It should use uchar instead of schar in it's implementation. The bug also occurs when the function is called via the public API:

    uint8_t a[32]{0, 10, 250};
    std::cout << (int)Vc::schar_v((signed char*)a).max() << '\n'; // correctly returns 10
    std::cout << (int)Vc::uchar_v(a).max() << '\n'; // returns 10, should be 250

See: https://godbolt.org/z/8K6Yb3oYv.

Would you like to provide us with a pull request? Otherwise, I can fix it myself.

@wjr-z
Copy link
Author

wjr-z commented Dec 9, 2022

不用担心,我可以翻译!我会用英语回答,所以你可以选择你最喜欢的翻译引擎!

我可以确认,这是实现中的错误。它应该使用而不是在其实现中使用。当通过公共 API 调用函数时,也会发生此错误:uchar Vc::Detail::max(__m128i a, uchar)``uchar``schar

    uint8_t a[32]{0, 10, 250};
    std::cout << (int)Vc::schar_v((signed char*)a).max() << '\n'; // correctly returns 10
    std::cout << (int)Vc::uchar_v(a).max() << '\n'; // returns 10, should be 250

请参阅:https://godbolt.org/z/8K6Yb3oYv。

您想向我们提供拉取请求吗?否则,我可以自己修复它。

我不太会pull请求,您可以自己修复它。

bernhardmgruber added a commit to bernhardmgruber/Vc that referenced this issue Dec 9, 2022
@wjr-z
Copy link
Author

wjr-z commented Dec 10, 2022

No worries, I could translate! I will answer in English, so you can pick your favorite translation engine!

I can confirm, this is a bug in the implementation of uchar Vc::Detail::max(__m128i a, uchar). It should use uchar instead of schar in it's implementation. The bug also occurs when the function is called via the public API:

    uint8_t a[32]{0, 10, 250};
    std::cout << (int)Vc::schar_v((signed char*)a).max() << '\n'; // correctly returns 10
    std::cout << (int)Vc::uchar_v(a).max() << '\n'; // returns 10, should be 250

See: https://godbolt.org/z/8K6Yb3oYv.

Would you like to provide us with a pull request? Otherwise, I can fix it myself.

实际上min也有问题,不过我当时只列出了max的测试,抱歉!
对于min的测试如下
https://godbolt.org/z/d36c8YPsY
显然第二个的min应该是0而不是1。
对于a[2+16]=250,如果没有这一句,很容易发现不出错误。
https://godbolt.org/z/q9dnqx4v8
造成这个的原因大概是使用Vc_AVX_TO_SSE_2_NEW,导致先使用了AVX2中的unsigned函数,后使用SSE2中的signed函数,使得250先与1进行min_epu8,导致发现不出错误。

bernhardmgruber added a commit to bernhardmgruber/Vc that referenced this issue Feb 6, 2023
bernhardmgruber added a commit to bernhardmgruber/Vc that referenced this issue Sep 29, 2023
bernhardmgruber added a commit to bernhardmgruber/Vc that referenced this issue Sep 29, 2023
bernhardmgruber added a commit to bernhardmgruber/Vc that referenced this issue Sep 29, 2023
bernhardmgruber added a commit that referenced this issue Sep 29, 2023
@bernhardmgruber
Copy link
Collaborator

Reopen, because of remaining issue with clang.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants