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

enable AtomicReference everywhere #65

Merged
merged 5 commits into from
Mar 17, 2023
Merged

enable AtomicReference everywhere #65

merged 5 commits into from
Mar 17, 2023

Conversation

tayloraswift
Copy link
Contributor

@tayloraswift tayloraswift commented Feb 17, 2023

fixes #64 , and updates documentation to remove references to custom build flags.

i was not able to build the tests in their entirety, the 40k gybbed Basics.swift overwhelms my local toolchain. but i tried to make individual subsets of Basics.swift compile a little faster (~15%) by adding type annotations.

side note:

compilation time seems to grow linearly with the number of types gyb generates tests for.

n time
1 4.5s
2 10s
3 20s
4 29s
5 40s

the smaller time for the first n is probably because Int benefits from being the default inferred integer literal type.

  types = [
    ("Int",        "Int",    "12", "23"),
    ("Int8",       "Int8",   "12", "23"),
    ("Int16",      "Int16",  "12", "23"),
    ("Int32",      "Int32",  "12", "23"),
    ("Int64",      "Int64",  "12", "23"),
    ("UInt",       "UInt",   "12", "23"),
    ("UInt8",      "UInt8",  "12", "23"),
# ...

@tayloraswift
Copy link
Contributor Author

update: i was able to build and run the tests and verified they all pass :)

@stephentyrone
Copy link
Member

stephentyrone commented Feb 17, 2023

My gut feeling is that we should resolve this by unconditionally defining ENABLE_DOUBLEWIDE_ATOMICS rather than removing the checks, to make supporting other platforms that don't have support easier in the future. Otherwise someone is going to port Swift to some constrained environment and they'll have to reintroduce all the checks.

@tayloraswift
Copy link
Contributor Author

i don't have a strong opinion on this, i would just prefer some version of this to be merged and tagged as soon as possible to unblock myself.

@lorentey which approach do you prefer?

@tayloraswift
Copy link
Contributor Author

My gut feeling is that we should resolve this by unconditionally defining ENABLE_DOUBLEWIDE_ATOMICS rather than removing the checks, to make supporting other platforms that don't have support easier in the future. Otherwise someone is going to port Swift to some constrained environment and they'll have to reintroduce all the checks.

i might just be brainfarting here, but how do you unconditionally define ENABLE_DOUBLEWIDE_ATOMICS within a swift source module? i don’t think i have ever used the compiler directives in this way.

@tayloraswift
Copy link
Contributor Author

according to https://forums.swift.org/t/how-to-use-compiler-directives-to-define-a-build-flag/63191/4 there is no way to accomplish this in swift today. @stephentyrone do you know of an alternative approach we can take here?

@lorentey
Copy link
Member

Unconditionally enabling double-wide atomics is the right move. It'll need to be tied to a release that bumps the minimum toolchain version to a Swift version whose runtime also requires 16-byte atomics, but other than that this should be smooth.

@lorentey
Copy link
Member

(I wouldn't like to leave the conditionals in place, unless we currently still support a platform that doesn't have wide atomics.)

@tayloraswift
Copy link
Contributor Author

great! this is what the PR implements right now. anything you would like changed, or is this good to merge?

Copy link
Member

@lorentey lorentey left a comment

Choose a reason for hiding this comment

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

Looks good. We'll also need to bump the required toolchain version in the package manifest.

Comment on lines 71 to 73
if [ "$(uname)" = "Linux" ]; then
try "spm.release.dword.build" $swift build -c release $spm_flags -Xcc -mcx16 -Xswiftc -DENABLE_DOUBLEWIDE_ATOMICS --build-path "$build_dir/spm.release.dword"
try "spm.release.dword.build" $swift build -c release $spm_flags --build-path "$build_dir/spm.release.dword"
fi
Copy link
Member

Choose a reason for hiding this comment

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

Nit: we should just remove this entire if altogether -- this configuration is now the default, so it is rolled into the default Linux case.

Comment on lines 135 to 137
if [ "$(uname)" != "Darwin" ]; then
try "spm.release.dword.test" $swift test -c release $spm_flags -Xcc -mcx16 -Xswiftc -DENABLE_DOUBLEWIDE_ATOMICS --build-path "$build_dir/spm.release.dword"
try "spm.release.dword.test" $swift test -c release $spm_flags --build-path "$build_dir/spm.release.dword"
fi
Copy link
Member

Choose a reason for hiding this comment

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

Same here -- let's remove this if.

Sources/Atomics/IntegerConformances.swift.gyb Show resolved Hide resolved
@lorentey
Copy link
Member

@swift-ci test

@lorentey
Copy link
Member

@swift-ci test

@lorentey lorentey merged commit 0bcc0c5 into apple:main Mar 17, 2023
@lorentey lorentey added this to the 1.1.0 milestone Mar 17, 2023
@tayloraswift
Copy link
Contributor Author

YESS

This pull request was closed.
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.

Atomics cannot be used in a Swift Package Index package with AtomicReference enabled.
3 participants