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

core.int128 alignment is adjusted #14768

Merged
merged 1 commit into from
Jan 26, 2023
Merged

Conversation

WalterBright
Copy link
Member

As alignment is now implemented per #14764 the code generated trying to align everything to 16 bytes when the stack is aligned to less is just terrible. I see no reason for alignment to be more than the stack alignment, so this changes the alignment of Cent to match the stack.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @WalterBright!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#14768"

@maxhaton
Copy link
Member

maxhaton commented Jan 2, 2023

Why is it explicitly aligned at all?

ibuclaw
ibuclaw previously requested changes Jan 2, 2023
Copy link
Member

@ibuclaw ibuclaw left a comment

Choose a reason for hiding this comment

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

Doesn't match gdc or ldc.

@ibuclaw
Copy link
Member

ibuclaw commented Jan 2, 2023

Why is it explicitly aligned at all?

Indeed, if a compiler has a specific requirement, then the alignment should be dictated by the compiler, not the library.

As for your question, I expect to match native int128 alignment. Could be a __traits that asks the Target, preferably something that allows querying any sort of intN types.

@WalterBright
Copy link
Member Author

I don't really know what change you are requesting. The compiler is using the predefined versions to tell it what alignment to use.

@WalterBright
Copy link
Member Author

I changed it so the alignment that works best with D is under version (DigitalMars)

@RazvanN7
Copy link
Contributor

RazvanN7 commented Jan 5, 2023

@ibuclaw is this ok?

@ibuclaw
Copy link
Member

ibuclaw commented Jan 5, 2023

Dmd would no longer be abi compatible with native int128 as a result of this. If everyone is fine letting dmd to continue shooting itself, then OK.

@WalterBright
Copy link
Member Author

I don't know why the native ABI would require it be aligned on 16 bytes anyway because the instructions to implement don't need it.

Be that as it may, for someone who is trying to interface it with the native ABI, for those instances that need it, align(16) in front of those instances will do the trick. Assuming the alignment PR also gets pulled.

version (D_SIMD)
private enum Cent_alignment = 16;
else version (X86_64)
private enum Cent_alignment = 4;
Copy link
Member

Choose a reason for hiding this comment

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

Looks like these last two cases are interchanged - this should be 8 and the else branch below should be 4. However, doesn't X86_64 imply D_SIMD anyway? X86-64 has SSE2 support in its base spec.

Copy link
Member Author

Choose a reason for hiding this comment

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

The layout exactly matches what the dmd compiler does.

Copy link
Member

Choose a reason for hiding this comment

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

The comment above:

is64bit ? 8 : 4

vs.

version (X86_64)
    private enum Cent_alignment = 4;
else
    private enum Cent_alignment = 8;

looks to be in contradiction to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see it now. Oops. good catch

@ljmf00
Copy link
Member

ljmf00 commented Jan 10, 2023

Why is it explicitly aligned at all?

To be ABI complaint. See System V spec on __int128 C type.

@maxhaton
Copy link
Member

Why is it explicitly aligned at all?

To be ABI complaint. See System V spec on __int128 C type.

This isn't compliance should be noted in the file if this is intended.

@WalterBright
Copy link
Member Author

There is no reason for dmd to align this. This PR needs to be pulled.

@RazvanN7 RazvanN7 merged commit fbf8061 into dlang:master Jan 26, 2023
@WalterBright WalterBright deleted the centAlign branch January 27, 2023 05: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.

7 participants