libsndfile / libsamplerate

An audio Sample Rate Conversion library
http://libsndfile.github.io/libsamplerate/
BSD 2-Clause "Simplified" License
613 stars 169 forks source link

use sse2 intrinsics for lrint/lrintf only on windows x64 #193

Closed sezero closed 1 year ago

sezero commented 1 year ago

Fixes https://github.com/libsndfile/libsamplerate/issues/187 Fixes https://github.com/libsndfile/libsamplerate/pull/188 Fixes https://github.com/libsndfile/libsamplerate/issues/191

CC: @rfomin

rfomin commented 1 year ago

LGTM

evpobr commented 1 year ago

Do we really need to limit SSE2 to x64?

https://github.com/libsndfile/libsndfile/blob/71f7bde180ade1869542d36483bc2e2c809c2189/cmake/CheckCPUArch.cmake#L17-L23

sezero commented 1 year ago

Do we really need to limit SSE2 to x64?

No, we don't. But can you guarantee that __SSE2__ is defined on x86?

sezero commented 1 year ago

Do we really need to limit SSE2 to x64?

No, we don't. But can you guarantee that __SSE2__ is defined on x86?

E.g.: see the build error I noted in #191

sezero commented 1 year ago

Any verdict ?

evpobr commented 1 year ago

Not always: https://stackoverflow.com/questions/18563978/detect-the-availability-of-sse-sse2-instruction-set-in-visual-studio

evpobr commented 1 year ago

Any verdict ?

Not sure redefining lrintf is good idea.

sezero commented 1 year ago

Not sure redefining lrintf is good idea.

I don't see the harm since we're doing it privately - but if you want static inlines, tell me.

sezero commented 1 year ago

Anyways, I force-pushed a version that keeps the inlines as inlines - no redefines.

evpobr commented 1 year ago

Thanks @sezero !