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

Fix ispc 32-bit address limits in SDF geometries. #530

Merged
merged 1 commit into from
Aug 22, 2018

Conversation

karjonas
Copy link
Contributor

No description provided.

@karjonas karjonas requested a review from favreau August 22, 2018 08:23
@favreau
Copy link
Member

favreau commented Aug 22, 2018

Ultra super cool PR. Looking at it now :)

Copy link
Member

@favreau favreau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be even nicer to have this mechanism extended to other geometries. But in a separate PR of course. Great job in any case.

@hernando
Copy link
Contributor

hernando commented Aug 22, 2018

As discussed offline with @karjonas , the actual was problem seems to be the overflow in multiplying two int variables to obtain an offset, obviously the result becomes negative and thus the segfault. The PR could be greatly simplified by spotting the offending arithmetic and promoting the types to uint64.
I would not merge it as is, as it makes the code unnecessarily obscure.

The trick to avoid overflow problems with memory buffers in ispc is to not use the syntax buffer[index] (because the ispc compiler casts index to a int32 integer in the assembler instruction), but rather *(buffer + index). The original code was already using pointer arithmetic.

@karjonas karjonas force-pushed the sdfaddress branch 2 times, most recently from 65fc443 to ff4e972 Compare August 22, 2018 14:04
@karjonas
Copy link
Contributor Author

I have done some more tests now and it seems like not even *(buffer + index) is good enough. Anything using pointer arithmetics is giving issues so what I ended up doing is casting the pointer to a uint64 and doing all arithmetics and then casting it back to a pointer of the right type. See __define_safe_ptr_inc macro in ExtendedSDFGeometries.ispc.

@karjonas karjonas merged commit a94fe34 into BlueBrain:master Aug 22, 2018
@karjonas karjonas deleted the sdfaddress branch August 22, 2018 14:59
@karjonas karjonas restored the sdfaddress branch August 22, 2018 14:59
@karjonas karjonas deleted the sdfaddress branch August 22, 2018 14:59
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

Successfully merging this pull request may close these issues.

3 participants