Skip to content

Conversation

@Azoy
Copy link
Contributor

@Azoy Azoy commented Jun 18, 2024

Also adds a concrete implementation of _lowWord for both Int128 and UInt128. I've sprinkled available attributes everywhere because I personally dislike the implicit availability from the extension declaration because a new API introduced in an existing extension may forget to add availability which would be incorrect, so littering this everywhere hopefully makes it more obvious.

Resolves: #74481

@Azoy Azoy requested a review from a team as a code owner June 18, 2024 17:49
@Azoy
Copy link
Contributor Author

Azoy commented Jun 18, 2024

@swift-ci please test

@Azoy
Copy link
Contributor Author

Azoy commented Jun 18, 2024

@swift-ci please benchmark

@benrimmington

This comment was marked as outdated.

#elseif _pointerBitWidth(_32)
UInt(Builtin.trunc_Int64_Int32(_low._value))
#else
UInt(truncatingIfNeeded: _low)
Copy link
Contributor

Choose a reason for hiding this comment

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

Formally I think the #else branch here should probably hit the default implementation instead, but that requires refactoring this a bit, so I'm ok with this for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this implementation is technically incorrect for words larger than 64 bits, so I just wrote this as truncating self.

@inline(__always)

@available(SwiftStdlib 6.0, *)
@_transparent
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not certain that we want these to be @_transparent. These are potentially pretty heavyweight operations to drop into the caller (though we might reap benefits in the case of constant divisors).

@Azoy
Copy link
Contributor Author

Azoy commented Jun 18, 2024

@stephentyrone how do we feel about making _low and _high public? Considering that the underlying builtin _value property is public (for good reason) it seems like we should just make these other accessors public as well. Thoughts?

@stephentyrone
Copy link
Contributor

It's a little bit weird since we don't have them for any other integer types, but other than that I'm OK with it.

@Azoy
Copy link
Contributor Author

Azoy commented Jun 18, 2024

@swift-ci please test

@Azoy
Copy link
Contributor Author

Azoy commented Jun 18, 2024

@swift-ci please benchmark

@Azoy Azoy force-pushed the int128-attributes branch from 52ac1e6 to 87a8edb Compare June 18, 2024 23:08
@Azoy
Copy link
Contributor Author

Azoy commented Jun 18, 2024

@swift-ci please test

@Azoy
Copy link
Contributor Author

Azoy commented Jun 20, 2024

@swift-ci please test

@benrimmington
Copy link
Contributor

Do you also want to @swift-ci test WebAssembly for the _pointerBitWidth(_32) parts?

@Azoy
Copy link
Contributor Author

Azoy commented Jun 20, 2024

@swift-ci test WebAssembly

@Azoy
Copy link
Contributor Author

Azoy commented Jun 20, 2024

Sure!

@Azoy Azoy merged commit dd8a21e into swiftlang:main Jun 21, 2024
@Azoy Azoy deleted the int128-attributes branch June 21, 2024 01:28
Azoy added a commit to Azoy/swift that referenced this pull request Jun 25, 2024
[stdlib] Apply @_transparent consistently for (U)Int128
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.

[SE-0425] Int128 and UInt128 APIs have different attributes

3 participants