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

math ops fail to do int/float conversion or generate an error on SSE2 #66

Closed
peabody-korg opened this issue Sep 7, 2017 · 4 comments
Closed

Comments

@peabody-korg
Copy link
Contributor

float32x4 foo(float32x4 a, int32x4 b)
{
    return a+b;
}

results in a single addps instruction for SSE2, as if b were a float32.

float32x4 foo(float32x4 a, int32x4 b)
{
    return add(a,b);
}

results in a compilation error, which is resolved by explicitly converting b with to_float32(). Presumably use of + should fail in the same was as use of add() (or even better would be to automatically convert between float/int just scalar operations would)

@p12tic
Copy link
Owner

p12tic commented Sep 12, 2017

Thanks for the bug report. Unfortunately I can't reproduce the issue on neither the master branch nor C++11 or C++98 versions of the 2.0 release. Which version of libsimdpp are you using?

@peabody-korg
Copy link
Contributor Author

I'm at 2545b97 (libsimdpp-2.x branch, v2.0 tag). the problem appears to be isolated to xcode 8.3.3 and associated clang. In case it matters, I'm using SIMDPP_ARCH_X86_SSSE3, and using xcode's platform default vector set (which appears to be SSE 4.2).

@p12tic
Copy link
Owner

p12tic commented Oct 6, 2017

Turns out this is caused by the clang vector extension (corresponding GCC docs). The extension defines various operators for the native SIMD types such as __m128 and __m128i. With default options, one can write this code:

__m128i a;
__m128 b;

b = b + a; (results in addps)

This API was probably supposed to be used with the native clang vector types that expose more information about the type of data within them. Extending this API to types such as __m128i seems to be poor design choice, as they correspond to several different data layouts. For example, addition of two __m128i quantities always results in paddq instruction.

With regards to libsimdpp, this revelation is unfortunate, because code like this will result in wrong behavior instead of failing to compile:

int8<16> foo (int8<16> a, int16<8> b)
{
    // the following invokes int8<16>::native_type() and int16<8>::native_type() conversion
    // operators, retrieves two __m128i quantities and adds them via paddq instruction
    return a + b; 
}

Unfortunately there seems no way to disable the operators provided by the vector extension. There is -fno-lax-vector-conversions option, but it still allows adding two __m128i variables, so it's not a solution. If this information is correct, then the native_type() operators will need to be removed eventually, which means that it no longer will be possible to write code like this:

int8<16> a, b;
a = _mm_add_epi8(a, b);

As a replacement, a member function will be added which will enable the code to be rewritten like this:

int8<16> a, b;
a = _mm_add_epi8(a.native(), b.native());

@p12tic
Copy link
Owner

p12tic commented Oct 8, 2017

Member function to retrieve native vector value to all vector types have been added in 40ba6dd.

The usage of the old conversion operator throughout the library have been removed in ba136bf.

Tests to verify that wrong conversions are no longer accepted have been added in in afd8708.

The old conversion operators have been marked as deprecated in cac3857. The API will be removed some time in the future.

If one wants to disable the old operators to avoid this bug, SIMDPP_DEFINE_IMPLICIT_CONVERSION_OPERATOR_TO_NATIVE_TYPES can be defined to 0.

@p12tic p12tic closed this as completed Oct 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants