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 GCC vector alignment and aliasing issues #93

Merged
merged 1 commit into from
Feb 12, 2022

Conversation

adbancroft
Copy link
Collaborator

Fix #90

Only occurs under -O3 on GCC x86_64 (v9.3 tested). GCC has built in SSE & AVX2 vector types as well as built in vector extensions, including subscription operator. I think the original code was clashing with this.

@adbancroft
Copy link
Collaborator Author

The real solution is to fully implement 16-bit vector division.

@ridiculousfish
Copy link
Owner

ridiculousfish commented Feb 11, 2022

Thanks for tackling this, using unions here is the correct fix, nicely done. I didn't realize that gcc would complain about aliasing vectors through pointers-to-integers.

Regarding the second commit, I think there's no "array overrun" but instead what's happening is that our tester was dividing INT_MIN by -1 (or 64 bit analog) which is undefined behavior. Our tester generates random data and wasn't protecting against that possibility.

This is "fixed" by correctly observing strict aliasing, which implies that the INT_MIN came from the compiler reacting to the UB. The fix to use a union is good and correct, fixing the strict aliasing violations, but it does not address the underlying problem which is that we "test" -1 / INT_MIN and we should make sure that doesn't happen.

Anyways I have some nits with the comments but the overall approach of using unions is right. You can fix the -1/INT_MIN issue or leave it for later and I'll pick it up.

Please squash the two commits together (unless there really is an array overrun which I am missing).

Issue ridiculousfish#90 with the simple vector division: GCC warnings & test failures
This is a performance penalty
@adbancroft
Copy link
Collaborator Author

The array overrun was caused by my first check in - I should have squashed that commit in my local repo. Fixed.

@adbancroft
Copy link
Collaborator Author

I didn't realize that gcc would complain about aliasing vectors through pointers-to-integers.

To be fair, I probably introduced this when I added 16-bit support and the SIMPLE_VECTOR_DIVISION macro.

@adbancroft adbancroft merged commit d02d313 into ridiculousfish:master Feb 12, 2022
@adbancroft adbancroft deleted the Issue_90 branch February 12, 2022 01:08
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.

libdivide.h:1691:59: error: ‘numers’ may be used uninitialized in this function
2 participants