search

redorav / hlslpp

Math library using HLSL syntax with multiplatform SIMD support
MIT License
579 stars 47 forks source link

Integer vector division is working incorrectly on Macs with Apple M1 (ARM) #60

Closed egorodet closed 2 years ago

egorodet commented 2 years ago

Signed and unsigned integer vector division operators both for scalar and vector divisors are working incorrectly when compiled for Apple M1 on MacOS >= 11. Here's an example of failed Catch tests for a wrapper class Point<T,size> in my project for HLSL++ vector. It is also reproducible under PR #59 , which adds missing unit tests for division operator.

Screenshot 2022-07-18 at 12 21 25
redorav commented 2 years ago

Hi @egorodet, it is a known issue that integer division is broken. There are no intrinsics for integer division and when I sat down to find a solution I couldn't find anything that wasn't a bit of a monster and left it for the future. Currently all it does is cast to float, divide and cast back which obviously isn't very good.

I had an issue open already, I'll close that one and track here instead. If you know of a good solution I could put in that would make it much easier, otherwise I'll see when I can find a bit of time to look into it.

redorav commented 2 years ago

Copying original comment from the other bug:

SSE doesn't have native division instructions for vectors. One possibility is to extract the scalars, divide, then put back. Another alternative is to take a look at this website which seems to have alternatives and claim to be fast

http://libdivide.com/

egorodet commented 2 years ago

Hi @redorav, yes I see that current implementation is not perfect, but it seems to work in most cases on x86/64 architecture, but gives completely wrong results on ARM (Apple M1 in my case). So additionally to existing issue, there's something wrong in the NEON implementation of the current workaround.

egorodet commented 2 years ago

I added workaround in the code of my wrapper class Point, so it should not be a stopper for me. It's up to you if it's better to keep or close this issue. I'm fine with description of the existing issue, just want to raise its priority because current implementation could give wrong results.

redorav commented 2 years ago

I've fixed division in NEON, the explanation for why it was broken is here f8ebd874c8fdb5328c0c21e4b7fc8740e7b30540. Let me know if this is still an issue for you. It is "fixed" in the sense that it is consistent with SSE now, but the issue of large integers remains.

egorodet commented 2 years ago

@redorav Thanks for quick turnaround! My tests are passing fine with this update on M1 ✅

redorav commented 2 years ago

Thank you for detailed report, hlsl++ is a lot better thanks to your efforts