[android] don't enable WMO as the old driver fails to build with WMO … #851
Conversation
|
build.ps1 uses the old driver which has an issue with the WMO mode for android here. |
|
@swift-ci please test |
There was a problem hiding this comment.
build.ps1 uses the old driver which has an issue with the WMO mode for android here.
Is this a bug we're tracking? It'd be nice to know so that we can hopefully fix it. Enabling WMO has a >4x improvement in performance for JSONDecoder in some cases so it'd be great if platforms like Windows and Android can eventually get that too 🙂
|
@jmschonfeld interesting, thanks for mentioning the numbers. That's definitely something that we want to fix. I filed swiftlang/swift#75890 to track this |
|
@swift-ci please test |
|
@jmschonfeld how does it look now? |
There was a problem hiding this comment.
@hyp makes sense to me, could we run a quick test on the swift repo via a cross repo test? (sorry, we don't have any CI on the cmake build in this repo yet so a quick validation on that before merging would be great - we're hoping to enable that on this repo in the future)
| if(NOT CMAKE_SYSTEM_NAME STREQUAL Windows) | ||
| # Don't enable WMO on Windows hosts due to linker failures, and the use of swift's | ||
| # old driver from build.ps1 when building for windows/android on a windows host. | ||
| if(NOT CMAKE_HOST_SYSTEM_NAME STREQUAL Windows) |
There was a problem hiding this comment.
Can we just drop this or conditionalise it on the CMake version? CMake has had -wmo as the default since 3.26. The compilation mode defaults were also part of the release when CMP0157 was introduced. Effectively, this is just to enable -wmo with CMake 3.24..<3.26.
There was a problem hiding this comment.
I originally tried dropping this conditional (and enabling WMO on Windows via this CMake code) and CI failed with errors such as the following:
FoundationEssentials-a79a82.o : error LNK2019: unresolved external symbol _foundation_uuid_unparse_upper referenced in function $s20FoundationEssentials4UUIDV10uuidStringSSvg
FoundationEssentials-a79a82.o : error LNK2019: unresolved external symbol _stringshims_strtod_l referenced in function $s20FoundationEssentials15JSONDecoderImpl33_B7023549748C8ED7BD56D5ACF500CBFALLC33_slowpath_unwrapFixedWidthInteger2as5json512numberBuffer10fullSource14digitBeginning3for_xxm_SbAA0T4ViewVys5UInt8VGApA0tZ5IndexVyAOGAA15_CodingPathNodeOq_SgtKs0noP0Rzs9CodingKeyR_r0_lFZ
It seemed there was an issue with WMO + the CShims on Windows. If we can find a way to enable WMO on these platforms I'm all for it, but we should double check that CI passes to make sure we don't hit these issues again
There was a problem hiding this comment.
Well, the problem is that WMO doesn't work very well with the old driver :(. When we build the toolchain, we don't yet have swift-driver as we need Foundation to build swift-driver. We can elide the WMO on Windows using -D CMAKE_Swift_COMPILER_USE_OLD_DRIVER=YES to workaround the issue for now. We will want to consider a multi-stage build so we can build with the new driver.
|
@compnerd I tested @jmschonfeld do you mind merging this in ? I ran the cross-PR testing in swiftlang/swift#75927 |
swiftlang#851) * [android] don't enable WMO as the old driver fails to build with WMO on windows * Switch to host system name check
…on windows