Add "_multithreaded" as valid _runtime argument
#72649
Conversation
kateinoigakukun
force-pushed
the
pr-4bfcd7ce97f156f7d629719f05149ee051ecf670
branch
from
7efe712 to
e85f246
Compare
kateinoigakukun
requested review from
ahoppen,
bnbarham,
CodaFi,
DougGregor,
hamishknight and
rintaro
as code owners
|
@swift-ci Please smoke test |
| @@ -8,3 +8,7 @@ var x = C() | |||
| #endif | |||
| #endif | |||
| var y = x | |||
|
|
|||
| #if !targetEnvironment(threads) | |||
There was a problem hiding this comment.
I'm still wondering whether #if !canImport(Synchronization) would be a better approach for something like this?
There was a problem hiding this comment.
Hmm, I feel targetEnvironment(threads) is a very straightforward representation considering threads is the environment part of target triple and other environments like simulator in arm64-apple-ios-simulator or macabi in x86_64-apple-ios-macabi. Also I think it's not desirable situation to recommend people use canImport for detecting target, which is beyond the original meaning of canImport detecting module importability.
There was a problem hiding this comment.
I think the confusion is that "threads" is not really an "environment", it's a platform feature. "The iOS simulator" is an environment, as is "Mac Catalyst". canImport() does seem closer to me to the right mechanism because the question you're asking is, approximately, "can I import the threading library?"
There was a problem hiding this comment.
Alternatively, perhaps _runtime(_multithreaded)?
There was a problem hiding this comment.
-threads implies not only the ability to import the threading library but also different ABI and CPU features. But I agree that "threads" is not an actual environment as others, thus I prefer _runtime(_multithreaded) now. WDYT @MaxDesiatov ?
There was a problem hiding this comment.
FWIW _runtime(_multithreaded), however we precisely spell it, would also be useful on other platforms (e.g. Embedded Swift) where the existence of multithreading is not otherwise knowable. I think for Darwin, Linux, and Windows, it would just need to be set to true.
There was a problem hiding this comment.
would also be useful on other platforms (e.g. Embedded Swift)
+1
There was a problem hiding this comment.
IIUC in general we now prefer experimental feature flags to underscored symbols, any preferences on this one specifically?
There was a problem hiding this comment.
I think the feature is very trivial and it's not difficult to keep compatibility with future evolution. So adding a new "experimental feature" sounds too much for me.
There was a problem hiding this comment.
I think for Darwin, Linux, and Windows, it would just need to be set to true.
Agreed, it should just be true in all of those cases.
I'm still wondering whether #if !canImport(Synchronization) would be a better approach for something like this?
I would say not. There's no reason you shouldn't be able to e.g. import Synchronization on a system that has no threads; IMO synchronization primitives in that case should probably just do nothing, rather than being unimplemented.
|
I agree this should be a _runtime condition. Also, please be sure to update swift-syntax! |
kateinoigakukun
force-pushed
the
pr-4bfcd7ce97f156f7d629719f05149ee051ecf670
branch
from
e85f246 to
10431a4
Compare
This patch adds "_multithreaded" as a valid `_runtime` argument and sets it when the target is `wasm32-unknown-wasi-threads` or other non-none OS targets.
kateinoigakukun
force-pushed
the
pr-4bfcd7ce97f156f7d629719f05149ee051ecf670
branch
from
10431a4 to
7441e03
Compare
|
@swift-ci Please smoke test |
kateinoigakukun
changed the title
targetEnvironment argument_runtime argument
|
Updated with |
| @@ -111,6 +111,7 @@ static const SupportedConditionalValue SupportedConditionalCompilationPointerBit | |||
| static const SupportedConditionalValue SupportedConditionalCompilationRuntimes[] = { | |||
| "_ObjC", | |||
| "_Native", | |||
| "_multithreaded", | |||
There was a problem hiding this comment.
If this is the spelling we want, it should probably match the casing of the other valid values.
|
Is a swift-syntax change also needed? |
|
As far as I checked, we don't need swift-syntax side support because it's handled as |
This PR opts WASI builds into using `pthread_mutex_t` in `Locked` when the WASI environment supports threading. `_runtime(_multithreaded)` was added very recently with swiftlang/swift#72649, so we need an additional compiler version check before testing the runtime flag. WASI with Swift 5.10, as well as WASI without threading, will continue to stub out `Locked`.
) This PR opts WASI builds into using `pthread_mutex_t` in `Locked` when the WASI environment supports threading. `_runtime(_multithreaded)` was added very recently with swiftlang/swift#72649, so we need an additional compiler version check before testing the runtime flag. That change has not been cherry-picked to Swift 6.0, so assume 6.1 or later is needed. WASI with Swift 5.10, as well as WASI without threading, will continue to stub out `Locked`. ### Checklist: - [x] Code and documentation should follow the style of the [Style Guide](https://github.com/apple/swift-testing/blob/main/Documentation/StyleGuide.md). - [x] If public symbols are renamed or modified, DocC references should be updated.
This patch adds "threads" as a valid
targetEnvironmentargument and sets it when the target iswasm32-unknown-wasi-threads.