-
Notifications
You must be signed in to change notification settings - Fork 10.5k
SwiftCompilerSources doesn't work on Windows ARM64 #74866
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
Comments
#74599 seems to change the error to the following locally for me. So it may be fixing an error but the compiler still crashes.
|
#74432 temporarily fixes this issue. |
This was fixed in #74599 |
I'll leave this issue open to keep track of actually making this work. |
@kavon yes. Do you know if anyone working on this issue? |
On Windows/AArch64, a different register is used between when an arugment is both inreg and sret (X0 or X1) and when it is just sret (X8) as the following comment indicates: https://github.com/llvm/llvm-project/blob/46fe36a4295f05d5d3731762e31fc4e6e99863e9/llvm/lib/Target/AArch64/AArch64CallingConvention.td#L42 ``` // In AAPCS, an SRet is passed in X8, not X0 like a normal pointer parameter. // However, on windows, in some circumstances, the SRet is passed in X0 or X1 // instead. The presence of the inreg attribute indicates that SRet is // passed in the alternative register (X0 or X1), not X8: // - X0 for non-instance methods. // - X1 for instance methods. // The "sret" attribute identifies indirect returns. // The "inreg" attribute identifies non-aggregate types. // The position of the "sret" attribute identifies instance/non-instance // methods. // "sret" on argument 0 means non-instance methods. // "sret" on argument 1 means instance methods. CCIfInReg<CCIfType<[i64], CCIfSRet<CCIfType<[i64], CCAssignToReg<[X0, X1]>>>>>, CCIfSRet<CCIfType<[i64], CCAssignToReg<[X8]>>>, ``` So missing/dropping inreg can cause a codegen bug. This is a partial fix for swiftlang#74866
@eeckstein @kavon I believe I found a codegen bug that was a cause for this issue. A wrong register (x8) is used for an sret argument in a swift-to-C++ call where C++ rightly expects it to be on x1.
Draft fix PR: #76159 Unfortunately it seems like this is a partial fix as I still get a different type of crash in the Remaining crash
|
Awesome! Thanks a lot! |
CC @compnerd |
On Windows/AArch64, a different register is used between when an arugment is both inreg and sret (X0 or X1) and when it is just sret (X8) as the following comment indicates: https://github.com/llvm/llvm-project/blob/46fe36a4295f05d5d3731762e31fc4e6e99863e9/llvm/lib/Target/AArch64/AArch64CallingConvention.td#L42 ``` // In AAPCS, an SRet is passed in X8, not X0 like a normal pointer parameter. // However, on windows, in some circumstances, the SRet is passed in X0 or X1 // instead. The presence of the inreg attribute indicates that SRet is // passed in the alternative register (X0 or X1), not X8: // - X0 for non-instance methods. // - X1 for instance methods. // The "sret" attribute identifies indirect returns. // The "inreg" attribute identifies non-aggregate types. // The position of the "sret" attribute identifies instance/non-instance // methods. // "sret" on argument 0 means non-instance methods. // "sret" on argument 1 means instance methods. CCIfInReg<CCIfType<[i64], CCIfSRet<CCIfType<[i64], CCAssignToReg<[X0, X1]>>>>>, CCIfSRet<CCIfType<[i64], CCAssignToReg<[X8]>>>, ``` So missing/dropping inreg can cause a codegen bug. This is a partial fix for swiftlang#74866
cc @atrick |
This case was missed by swiftlang#76159. This is a partial fix for swiftlang#74866
@hjyamauchi Did you have a chance to look into the other issue, too? |
@eeckstein I'm looking into it. I think I found another indirect result calling convention mismatch but haven't fully figured it out. |
On Windows ARM64, how a struct value type is returned is sensitive to conditions including whether a user-defined constructor exists, etc. See https://learn.microsoft.com/en-us/cpp/build/arm64-windows-abi-conventions?view=msvc-170#return-values That caused a calling convention mismatch between the non-USED_IN_CPP_SOURCE (Swift) side and the USE_IN_CPP_SOURCE (C++) side and a crash. Add this constructor so that the calling convention matches. This is a fix for the OnoneSimplification crash in swiftlang#74866 (comment) and is a partial fix for swiftlang#74866 (comment)
On Windows/AArch64, a different register is used between when an arugment is both inreg and sret (X0 or X1) and when it is just sret (X8) as the following comment indicates: https://github.com/llvm/llvm-project/blob/46fe36a4295f05d5d3731762e31fc4e6e99863e9/llvm/lib/Target/AArch64/AArch64CallingConvention.td#L42 ``` // In AAPCS, an SRet is passed in X8, not X0 like a normal pointer parameter. // However, on windows, in some circumstances, the SRet is passed in X0 or X1 // instead. The presence of the inreg attribute indicates that SRet is // passed in the alternative register (X0 or X1), not X8: // - X0 for non-instance methods. // - X1 for instance methods. // The "sret" attribute identifies indirect returns. // The "inreg" attribute identifies non-aggregate types. // The position of the "sret" attribute identifies instance/non-instance // methods. // "sret" on argument 0 means non-instance methods. // "sret" on argument 1 means instance methods. CCIfInReg<CCIfType<[i64], CCIfSRet<CCIfType<[i64], CCAssignToReg<[X0, X1]>>>>>, CCIfSRet<CCIfType<[i64], CCAssignToReg<[X8]>>>, ``` So missing/dropping inreg can cause a codegen bug. This is a partial fix for swiftlang#74866 Cherrypick commit 3f0de57 Cherrypick PR swiftlang#76159
I managed to figure out a set of additional PRs to get the failing tests fixed in #77277. It's ready for review. @rjmccall @eeckstein @compnerd The PRs for |
An update on the Windows ARM64 compiler/toolchain.
|
We discussed this last week and it looks like the easiest way to go forward is to directly upgrade the hosttools to 6.0.
Thanks! |
The cherrypick PRs for |
👍 Let's wait for a 6.0 toolchain with this fixes. |
We'd still need to bootstrap a 6.0 toolchain in addition to have the above PRs merged, and I heard that #77815 may be a way we do it, correct? |
No, #77815 should not be needed to build a 6.0 toolchain without host tools. We were building 6.0 windows toolchains (without host tools) all the time and should have a new toolchain soon. |
@eeckstein I am only familiar with the way
Importantly, does this mean that merging the above two 6.0 PRs
And the plans sound like
Sounds about right? |
Yes, that is correct. Except:
It's enabled by default in |
@eeckstein Thank you! That helps me be much clearer on this :) I am not sure I can see but perhaps https://ci-external.swift.org/job/swift-6.0-windows-toolchain/495/consoleText
|
Right, I enabled it for Windows after 6.0 (and then disabled it again for Windows arm64) |
@hjyamauchi a new Windows 6.0 toolchain is now available |
yes, that should be it |
I locally tested the new 6.0 build: https://download.swift.org/swift-6.0-branch/windows10-arm64/swift-6.0-DEVELOPMENT-SNAPSHOT-2024-12-03-a/swift-6.0-DEVELOPMENT-SNAPSHOT-2024-12-03-a-windows10-arm64.exe a little bit and it seems in a working condition. I'm testing a main branch build using this new 6.0 build as the pinned toolchain (hosttools) now. Will update later |
My local main branch build is looking good though there is currently a different build issue in the win arm64 CI. I have up #78032 as a draft to update the pinned toolchain for the main branch to the above 6.0 build and enable CC @eeckstein |
#78032 has been merged. With that, |
That's great! |
I was able to confirm that the main-branch windows/arm64 toolchain works with a hello world program and building https://github.com/compnerd/swift-win32 as quick smoke tests :) However, I was not able to natively (on win/arm64) build the main branch swift toolchain using the 6.0.3 build as the pinned toolchain, due to a known MSVC arm64 miscompile bug (also reported here and here) which causes assertion failures in the pinned-toolchain compiler. Note
To fix this MSVC arm64 issue, we'd need to update the Visual Studio used in the swift.org CIs from VS 17.9.x (MSVC 14.39.x)
https://ci-external.swift.org/job/swift-main-windows-toolchain-arm64/821/consoleText to VS 17.11.x/MSVC 14.41.x or later (or go back to VS 17.8.x/MSVC 14.38.x)* Could this be done? (* I have been locally using VS 17.8.7 to avoid this issue.) |
cc @shahmishal |
We still need to have the Visual Studio / MSVC version updated on the CI to have fully fixed. |
@hjyamauchi Thanks for the ping! It will take a few weeks until we can make the upgrade. We'll let you know when we start the process. |
CI is now upgraded to new version of Visual Studio / MSVC version. @compnerd verified the issue was solved with the version change. |
@shahmishal Thank you! Could we have a new build of release/6.0 so that we can use it as the bootstrap/pinned toolchain for the native-arm64 windows toolchain (replacing Line 168 in 23a049c
|
Just checking in. Thanks for updating the MSVC in the CIs. With this the CIs are now able to produce the Windows ARM64 toolchain that's free from the MSVC miscompile issue and can work as the bootstrap/pinned toolchain for the native windows arm64 toolchain build. I understand that the swift.org CIs are not currently testing this (though we have a downstream CI that tests the native windows arm64 toolchain build). Is there a plan to cut a new release/6.0 build after this MSVC update so that we can use that as the bootstrap/pinned toolchain for the main branch (the default pinned toolchain in the |
Description
The compiler binary from the Windows ARM64 toolchain build crashes (segfaults) right after it's launched and it cannot even compile a hello world program.
https://download.swift.org/development/windows10-arm64/swift-DEVELOPMENT-SNAPSHOT-2024-06-03-a/swift-DEVELOPMENT-SNAPSHOT-2024-06-03-a-windows10-arm64.exe
https://ci-external.swift.org/job/swift-main-windows-toolchain-arm64/
Reproduction
Compile the hello world swift program
Stack dump
Expected behavior
No crash
Environment
Windows ARM64
Additional information
No response
The text was updated successfully, but these errors were encountered: