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 Issue 17237 - Wrong alignment for 256-bit vectors. #6582

Merged
merged 1 commit into from
Mar 2, 2017

Conversation

ibuclaw
Copy link
Member

@ibuclaw ibuclaw commented Mar 1, 2017

Relaxed the condition to allow 256-bit vector alignment, but I won't change the logic of anything else incase it affects something, you can do this in a much better way, but it's not my place to say.

Moved alignment adjusting code to Target::fieldalign, as the frontend should adhere to the result returned from this function, unless some explicit alignment was requested, and not overwrite it in frontend-only code.

@dlang-bot
Copy link
Contributor

Fix Bugzilla Description
17237 Wrong alignment of 256-bit and 512-bit vectors

@ibuclaw ibuclaw changed the base branch from master to stable March 1, 2017 23:03
@WalterBright
Copy link
Member

Auto-merge toggled on

@WalterBright WalterBright merged commit 25f8bc5 into dlang:stable Mar 2, 2017
@ibuclaw ibuclaw deleted the issue17237 branch March 2, 2017 07:21
if ((global.params.is64bit || global.params.isOSX) && (size == 16 || size == 32))
return size;

return (8 < size) ? 8 : size;
Copy link
Contributor

@kinke kinke Mar 3, 2017

Choose a reason for hiding this comment

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

@ibuclaw: What's the reason for this upper bound of 8/32 bytes (why not allow 64 for cache-line aligned fields etc.)? And I find the name size for actual alignment rather confusing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Meh, I don't use this code. It doesn't make sense to me either. You shouldn't be using this code either.

Copy link
Member Author

Choose a reason for hiding this comment

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

As for max field alignment, it should be 4 bytes for x86, 16 bytes for 64bit, excluding vectors and x87 types.

Copy link
Member Author

Choose a reason for hiding this comment

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

Your backend should handle this for you, however, but I won't feel sorry for you if it doesn't. :D

Copy link
Contributor

@kinke kinke Mar 3, 2017

Choose a reason for hiding this comment

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

Lol, thanks for the hint (LLVM obviously does), I've just seen that I messed up a front-end merge, so that we now have 2 Target::alignsize() implementations in LDC (D and C++), don't know which one gets used. ;) We don't use it either. ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants