search

intel / ARM_NEON_2_x86_SSE

The platform independent header allowing to compile any C/C++ code containing ARM NEON intrinsic functions for x86 target systems using SIMD up to AVX2 intrinsic functions
Other
430 stars 149 forks source link

Fixes warning C4310 from Visual Studio #26

Closed Bizonu closed 5 years ago

Bizonu commented 5 years ago

Fixes some warnings reported by Visual Studio: warning C4310: cast truncates constant value

Zvictoria commented 5 years ago

Hi, Bizonu. I do understand your concern about warnings but... I'd prefer to live with them than to have these contr-intuitive nubmers usage in the set functions. It not my style of coding, sorry :( However this decision is not the final one and probably I will re-consider it some day. But no promises there.

Bizonu commented 5 years ago

OK, you are the maintainer of this code, so is your decision what changes to include...

Usually I tend to fix all warnings (not just to disable them), because some warnings really are bugs, like in the following code:

_NEON2SSE_INLINE _NEON2SSE_PERFORMANCE_WARNING(float64x2_t vrndnq_f64(float64x2_t a), _NEON2SSE_REASON_SLOW_SERIAL) { _NEON2SSE_ALIGN_16 float64_t res[2]; _mm_store_pd(res, a); res[0] = nearbyintf(res[0]); // warning C4244: 'function': conversion from 'float64_t' to 'float', possible loss of data res[1] = nearbyintf(res[1]); // warning C4244: 'function': conversion from 'float64_t' to 'float', possible loss of data return _mm_load_pd(res); }`

So for numbers bigger than 2^24, the above function will return different results than on a real ARM CPU (or SSE4 implementation).

Zvictoria commented 5 years ago

Bizonu, thanks a lot. I agree that warnings are tricky and it would be cool if we find some way to fix them not loosing code quality :) and I will also look into vrndnq_f64 issue but with low priority.

Bizonu commented 5 years ago

Should I close the pull request, then?

Zvictoria commented 5 years ago

No, I prefer it to hand around for some time to remind me about this problem existence. Then let's see how it goes. Thanks again.

Zvictoria commented 5 years ago

Hi again. After some considerations I think your changes for int set functions make sense while my code quality concerns make sense as well :). So I will accept them with pleasure if you change them moving the original value to comment like in this line of code " - c_32768 = _mm_set1_epi16 ((int16_t)0x8000); //-32768 " "+ c_32768 = _mm_set1_epi16 (-32768); //0x8000 " Makes sense?

Bizonu commented 5 years ago

I'm not yet used with github and I thought that the pull request changes will not get updated when I commit on the same branch... yesterday I also fixed some other warnings and also the function vrndnq_f64().

I think it makes sense to add the constants as comments in hexadecimal, it makes the code more readable. I'll commit these change in few minutes...

Bizonu commented 5 years ago

I've added also the comments, as you suggested...