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 max_align_t for __int128 #3088

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

davebayer
Copy link
Contributor

The max_align_t is described as:

max_align_t is a standard-layout TrivialType(until C++26)TriviallyCopyable type(since C++26) whose alignment requirement is at least as strict (as large) as that of every scalar type.

The max_align_t was defined to be of type long double which could lead to invalid alignment e. g. for NVRTC with __int128 support because long double may be implemented as double which would lead to 8-byte alignment instead of 16-byte.

This PR adds a check for __int128 support before falling back to long double.

@davebayer davebayer requested review from a team as code owners December 9, 2024 09:37
@davebayer davebayer requested review from miscco and griwes December 9, 2024 09:37
Copy link

copy-pr-bot bot commented Dec 9, 2024

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@bernhardmgruber
Copy link
Contributor

Thanks for opening this PR, because it makes us aware of the problem. However, touching max_align_t is scary, because we need to make sure it also aligns with the compilers we have in use. Your changeset looks reasonable to me, but I definitely want to have more opinions here.

Copy link
Member

@wmaxey wmaxey left a comment

Choose a reason for hiding this comment

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

Since _LIBCUDACXX_HAS_NO_INT128 is extremely pessimistic, I would say this change is fine. Most compilers will define their max_align_t and will never end up here. NVRTC alone has the unique property where there is no 'host' alignment.

I can see this backfiring where the host uses 64b alignment and NVRTC compiles with 128b alignment. Interop between host and device might result in silent failures just like #3066.

I'd also like to hear more opinions.

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

Successfully merging this pull request may close these issues.

3 participants