Add the inreg attribute to sreg when present. #76159

hjyamauchi · 2024-08-30T00:57:49Z

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 #74866

hjyamauchi · 2024-08-30T00:58:02Z

@swift-ci please test

egorzhdan

I'm not familiar with this attribute, but the change looks reasonable, thanks!

rjmccall · 2024-08-30T17:44:58Z

lib/IRGen/GenCall.cpp

+        llvm::AttrBuilder builder(func->getContext());
+        builder.addAttribute(llvm::Attribute::InReg);
+        attrs = attrs.addParamAttributes(func->getContext(), argIndex, builder);
+      }


Hmm. I see that this is the established pattern we're using here, but it's clearly wrong and won't handle indirect calls correctly; we need to be setting these attributes based on the CGFunctionInfo.

@rjmccall It's not obvious to me, can you elaborate on how to set these attributes based on the CGFunctionInfo?

You want the attribute to already be present in the attributes returned by fn.getAttributes(), which ultimately means it should be added in SignatureExpansion::expandExternalSignatureTypes based on the same logic that Clang uses to do it.

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

hjyamauchi · 2024-09-05T01:03:37Z

@swift-ci please test

rjmccall

Thank you, that looks right.

compnerd · 2024-09-05T04:09:12Z

@swift-ci please test Windows platform

hjyamauchi · 2024-09-05T16:28:34Z

@swift-ci please test Windows platform

This case was missed by swiftlang#76159. This is a partial fix for swiftlang#74866

egorzhdan · 2024-09-10T14:52:42Z

@hjyamauchi looks like a test started failing on rebranch:

******************** TEST 'Swift(windows-x86_64) :: Interop/Cxx/class/method/methods-this-and-indirect-return-irgen-msvc.swift' FAILED ********************
Exit Code: 1

Command Output (stdout):
--
# RUN: at line 1
't:\\5\\bin\\swift-frontend.exe' -target x86_64-unknown-windows-msvc  -module-cache-path T:\5\swift-test-results\x86_64-unknown-windows-msvc\clang-module-cache -vfsoverlay T:/5/tools/swift\stdlib\windows-vfs-overlay.yaml -strict-implicit-module-context -Xcc -Xclang -Xcc -fbuiltin-headers-in-system-modules -swift-version 4  -define-availability 'SwiftStdlib 9999:macOS 9999, iOS 9999, watchOS 9999, tvOS 9999' -define-availability 'SwiftStdlib 5.0:macOS 10.14.4, iOS 12.2, watchOS 5.2, tvOS 12.2' -define-availability 'SwiftStdlib 5.1:macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0' -define-availability 'SwiftStdlib 5.2:macOS 10.15.4, iOS 13.4, watchOS 6.2, tvOS 13.4' -define-availability 'SwiftStdlib 5.3:macOS 11.0, iOS 14.0, watchOS 7.0, tvOS 14.0' -define-availability 'SwiftStdlib 5.4:macOS 11.3, iOS 14.5, watchOS 7.4, tvOS 14.5' -define-availability 'SwiftStdlib 5.5:macOS 12.0, iOS 15.0, watchOS 8.0, tvOS 15.0' -define-availability 'SwiftStdlib 5.6:macOS 12.3, iOS 15.4, watchOS 8.5, tvOS 15.4' -define-availability 'SwiftStdlib 5.7:macOS 13.0, iOS 16.0, watchOS 9.0, tvOS 16.0' -define-availability 'SwiftStdlib 5.8:macOS 13.3, iOS 16.4, watchOS 9.4, tvOS 16.4' -define-availability 'SwiftStdlib 5.9:macOS 14.0, iOS 17.0, watchOS 10.0, tvOS 17.0' -define-availability 'SwiftStdlib 5.10:macOS 14.4, iOS 17.4, watchOS 10.4, tvOS 17.4, visionOS 1.1' -define-availability 'SwiftStdlib 6.0:macOS 15.0, iOS 18.0, watchOS 11.0, tvOS 18.0, visionOS 2.0' -define-availability 'SwiftStdlib 6.1:macOS 9999, iOS 9999, watchOS 9999, tvOS 9999, visionOS 9999' -typo-correction-limit 10  -emit-irgen -I C:\Users\swift-ci\jenkins\workspace\swift-rebranch-windows-toolchain\swift\test\Interop\Cxx\class\method/Inputs -enable-experimental-cxx-interop C:\Users\swift-ci\jenkins\workspace\swift-rebranch-windows-toolchain\swift\test\Interop\Cxx\class\method\methods-this-and-indirect-return-irgen-msvc.swift | "C:\Program Files (x86)\Microsoft Visual Studio\Shared\Python39_64\python.exe" C:\Users\swift-ci\jenkins\workspace\swift-rebranch-windows-toolchain\swift\utils\PathSanitizingFileCheck --allow-unused-prefixes --sanitize BUILD_DIR=T:/5/tools/swift --sanitize SOURCE_DIR=C:/Users/swift-ci/jenkins/workspace/swift-rebranch-windows-toolchain/swift --use-filecheck t:\5\bin\filecheck.exe  --enable-windows-compatibility C:\Users\swift-ci\jenkins\workspace\swift-rebranch-windows-toolchain\swift\test\Interop\Cxx\class\method\methods-this-and-indirect-return-irgen-msvc.swift -check-prefix=CHECK -check-prefix=CHECK-x86_64
# executed command: 't:\\5\\bin\\swift-frontend.exe' -target x86_64-unknown-windows-msvc -module-cache-path 'T:\5\swift-test-results\x86_64-unknown-windows-msvc\clang-module-cache' -vfsoverlay 'T:/5/tools/swift\stdlib\windows-vfs-overlay.yaml' -strict-implicit-module-context -Xcc -Xclang -Xcc -fbuiltin-headers-in-system-modules -swift-version 4 -define-availability 'SwiftStdlib 9999:macOS 9999, iOS 9999, watchOS 9999, tvOS 9999' -define-availability 'SwiftStdlib 5.0:macOS 10.14.4, iOS 12.2, watchOS 5.2, tvOS 12.2' -define-availability 'SwiftStdlib 5.1:macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0' -define-availability 'SwiftStdlib 5.2:macOS 10.15.4, iOS 13.4, watchOS 6.2, tvOS 13.4' -define-availability 'SwiftStdlib 5.3:macOS 11.0, iOS 14.0, watchOS 7.0, tvOS 14.0' -define-availability 'SwiftStdlib 5.4:macOS 11.3, iOS 14.5, watchOS 7.4, tvOS 14.5' -define-availability 'SwiftStdlib 5.5:macOS 12.0, iOS 15.0, watchOS 8.0, tvOS 15.0' -define-availability 'SwiftStdlib 5.6:macOS 12.3, iOS 15.4, watchOS 8.5, tvOS 15.4' -define-availability 'SwiftStdlib 5.7:macOS 13.0, iOS 16.0, watchOS 9.0, tvOS 16.0' -define-availability 'SwiftStdlib 5.8:macOS 13.3, iOS 16.4, watchOS 9.4, tvOS 16.4' -define-availability 'SwiftStdlib 5.9:macOS 14.0, iOS 17.0, watchOS 10.0, tvOS 17.0' -define-availability 'SwiftStdlib 5.10:macOS 14.4, iOS 17.4, watchOS 10.4, tvOS 17.4, visionOS 1.1' -define-availability 'SwiftStdlib 6.0:macOS 15.0, iOS 18.0, watchOS 11.0, tvOS 18.0, visionOS 2.0' -define-availability 'SwiftStdlib 6.1:macOS 9999, iOS 9999, watchOS 9999, tvOS 9999, visionOS 9999' -typo-correction-limit 10 -emit-irgen -I 'C:\Users\swift-ci\jenkins\workspace\swift-rebranch-windows-toolchain\swift\test\Interop\Cxx\class\method/Inputs' -enable-experimental-cxx-interop 'C:\Users\swift-ci\jenkins\workspace\swift-rebranch-windows-toolchain\swift\test\Interop\Cxx\class\method\methods-this-and-indirect-return-irgen-msvc.swift'
# .---command stderr------------
# | <unknown>:0: warning: the '-enable-experimental-cxx-interop' flag is deprecated; please pass '-cxx-interoperability-mode=' instead
# | <unknown>:0: note: Swift will maintain source compatibility for imported APIs based on the selected compatibility mode, so updating the Swift compiler will not change how APIs are imported
# `-----------------------------
# executed command: 'C:\Program Files (x86)\Microsoft Visual Studio\Shared\Python39_64\python.exe' 'C:\Users\swift-ci\jenkins\workspace\swift-rebranch-windows-toolchain\swift\utils\PathSanitizingFileCheck' --allow-unused-prefixes --sanitize BUILD_DIR=T:/5/tools/swift --sanitize SOURCE_DIR=C:/Users/swift-ci/jenkins/workspace/swift-rebranch-windows-toolchain/swift --use-filecheck 't:\5\bin\filecheck.exe' --enable-windows-compatibility 'C:\Users\swift-ci\jenkins\workspace\swift-rebranch-windows-toolchain\swift\test\Interop\Cxx\class\method\methods-this-and-indirect-return-irgen-msvc.swift' -check-prefix=CHECK -check-prefix=CHECK-x86_64
# .---command stderr------------
# | C:\Users\swift-ci\jenkins\workspace\swift-rebranch-windows-toolchain\swift\test\Interop\Cxx\class\method\methods-this-and-indirect-return-irgen-msvc.swift:18:18: error: CHECK-x86_64: expected string not found in input
# | // CHECK-x86_64: define {{.*}} void @"?nonConstPassThroughAsWrapper@HasMethods@@QEAA?AUNonTrivialInWrapper@@H@Z"(ptr {{.*}} %{{.*}}, ptr noalias sret(%struct.NonTrivialInWrapper) {{.*}} %{{.*}}, i32 noundef %{{.*}})
# |                  ^
# | <stdin>:39:165: note: scanning from here
# |  call void @"?nonConstPassThroughAsWrapper@HasMethods@@QEAA?AUNonTrivialInWrapper@@H@Z"(ptr %instance, ptr noalias sret(%TSo19NonTrivialInWrapperV) %result, i32 42)
# |                                                                                                                                                                     ^
# | 
# | Input file: <stdin>
# | Check file: C:\Users\swift-ci\jenkins\workspace\swift-rebranch-windows-toolchain\swift\test\Interop\Cxx\class\method\methods-this-and-indirect-return-irgen-msvc.swift
# | 
# | -dump-input=help explains the following input dump.
# | 
# | Input was:
# | <<<<<<
# |           .
# |           .
# |           .
# |          34:  %result = alloca %TSo19NonTrivialInWrapperV, align 4 
# |          35:  call void @llvm.memset.p0.i64(ptr align 4 %result, i8 0, i64 4, i1 false) 
# |          36:  call void @llvm.lifetime.start.p0(i64 1, ptr %instance) 
# |          37:  %0 = call ptr @"__swift_cxx_ctor??0HasMethods@@QEAA@XZ"(ptr %instance) 
# |          38:  call void @llvm.lifetime.start.p0(i64 4, ptr %result) 
# |          39:  call void @"?nonConstPassThroughAsWrapper@HasMethods@@QEAA?AUNonTrivialInWrapper@@H@Z"(ptr %instance, ptr noalias sret(%TSo19NonTrivialInWrapperV) %result, i32 42) 
# | check:18                                                                                                                                                                         X error: no match found
# |          40:  %result.value = getelementptr inbounds %TSo19NonTrivialInWrapperV, ptr %result, i32 0, i32 0 
# | check:18     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# |          41:  %result.value._value = getelementptr inbounds %Ts5Int32V, ptr %result.value, i32 0, i32 0 
# | check:18     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# |          42:  %1 = load i32, ptr %result.value._value, align 4 
# | check:18     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# |          43:  call void @"??1NonTrivialInWrapper@@QEAA@XZ"(ptr %result) 
# | check:18     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# |          44:  call void @llvm.lifetime.end.p0(i64 4, ptr %result) 
# | check:18     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# |           .
# |           .
# |           .
# | >>>>>>
# `-----------------------------
# error: command failed with exit status: 1

--

https://ci-external.swift.org/job/swift-rebranch-windows-toolchain/311/console

It seems to be failing on x86_64, do you know if inreg should also be added to the expected test output there?

hjyamauchi · 2024-09-10T16:36:13Z

@egorzhdan inreg shouldn't be there for x86_64 but I suppose the param attributes could change on a rebranch. How do I locally reproduce this (which branches do the swift and llvm-project repo need to be sync'd to)? CC @compnerd

hjyamauchi · 2024-09-10T16:38:32Z

The CI link isn't the correct one?
I see

  Swift(windows-x86_64) :: Interop/Cxx/stdlib/use-std-vector.swift

egorzhdan · 2024-09-10T16:40:30Z

My bad! The correct link is https://ci-external.swift.org/job/swift-rebranch-windows-toolchain/311/console.

hjyamauchi · 2024-09-10T16:44:49Z

CC @rjmccall @compnerd @hyp This PR missed an additional missing inreg on sret and #76324 should fix it. Please take a look.

hjyamauchi · 2024-09-10T16:52:29Z

@egorzhdan I will take a look at the test failure.

hjyamauchi · 2024-09-10T17:02:37Z

@egorzhdan This PR #76373 should already fix the failure CC @hyp

egorzhdan · 2024-09-10T22:05:29Z

That's right, thanks @hjyamauchi.

This case was missed by swiftlang#76159. This is a partial fix for swiftlang#74866

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 swiftlang#76159

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

hjyamauchi mentioned this pull request Aug 30, 2024

SwiftCompilerSources doesn't work on Windows ARM64 #74866

Open

hjyamauchi marked this pull request as ready for review August 30, 2024 16:11

hjyamauchi requested review from zoecarver, hyp, egorzhdan and rjmccall as code owners August 30, 2024 16:11

hjyamauchi requested a review from compnerd August 30, 2024 16:12

egorzhdan approved these changes Aug 30, 2024

View reviewed changes

hyp approved these changes Aug 30, 2024

View reviewed changes

compnerd approved these changes Aug 30, 2024

View reviewed changes

rjmccall reviewed Aug 30, 2024

View reviewed changes

hjyamauchi force-pushed the issue74866 branch from c798df6 to 3f0de57 Compare September 5, 2024 01:02

hjyamauchi changed the title ~~Replicate the inreg attribute from the declaration to the call site~~ Add the inreg attribute to sreg when present. Sep 5, 2024

rjmccall approved these changes Sep 5, 2024

View reviewed changes

hjyamauchi merged commit 273f5fd into swiftlang:main Sep 5, 2024
5 checks passed

hjyamauchi added a commit to hjyamauchi/swift that referenced this pull request Sep 7, 2024

Also add inreg to sreg that's added by CreateCallOrInvoke

53072ce

This case was missed by swiftlang#76159. This is a partial fix for swiftlang#74866

hjyamauchi mentioned this pull request Sep 7, 2024

Fix the IR gen for C++ method calls and refactor around CGFunctionInfo #76324

Merged

hjyamauchi added a commit to hjyamauchi/swift that referenced this pull request Sep 24, 2024

Also add inreg to sreg that's added by CreateCallOrInvoke

a31e89d

This case was missed by swiftlang#76159. This is a partial fix for swiftlang#74866

hjyamauchi mentioned this pull request Oct 28, 2024

Cherrypick fixes for issue 74866 to release/6.0 #77261

Merged

hjyamauchi mentioned this pull request Oct 29, 2024

Cherrypick fixes for issue 74866 to release/5.10 #77277

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the inreg attribute to sreg when present. #76159

Add the inreg attribute to sreg when present. #76159

hjyamauchi commented Aug 30, 2024

hjyamauchi commented Aug 30, 2024

egorzhdan left a comment

rjmccall Aug 30, 2024

hjyamauchi Aug 30, 2024

rjmccall Aug 30, 2024

hjyamauchi commented Sep 5, 2024

rjmccall left a comment

compnerd commented Sep 5, 2024

hjyamauchi commented Sep 5, 2024

egorzhdan commented Sep 10, 2024 •

edited

Loading

hjyamauchi commented Sep 10, 2024

hjyamauchi commented Sep 10, 2024

egorzhdan commented Sep 10, 2024

hjyamauchi commented Sep 10, 2024

hjyamauchi commented Sep 10, 2024

hjyamauchi commented Sep 10, 2024

egorzhdan commented Sep 10, 2024

Add the inreg attribute to sreg when present. #76159

Add the inreg attribute to sreg when present. #76159

Conversation

hjyamauchi commented Aug 30, 2024

hjyamauchi commented Aug 30, 2024

egorzhdan left a comment

Choose a reason for hiding this comment

rjmccall Aug 30, 2024

Choose a reason for hiding this comment

hjyamauchi Aug 30, 2024

Choose a reason for hiding this comment

rjmccall Aug 30, 2024

Choose a reason for hiding this comment

hjyamauchi commented Sep 5, 2024

rjmccall left a comment

Choose a reason for hiding this comment

compnerd commented Sep 5, 2024

hjyamauchi commented Sep 5, 2024

egorzhdan commented Sep 10, 2024 • edited Loading

hjyamauchi commented Sep 10, 2024

hjyamauchi commented Sep 10, 2024

egorzhdan commented Sep 10, 2024

hjyamauchi commented Sep 10, 2024

hjyamauchi commented Sep 10, 2024

hjyamauchi commented Sep 10, 2024

egorzhdan commented Sep 10, 2024

egorzhdan commented Sep 10, 2024 •

edited

Loading