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

hjyamauchi · 2024-09-07T01:31:08Z

This case was missed by #76159.

This is a partial fix for #74866

hjyamauchi · 2024-09-07T01:31:21Z

@swift-ci please test

hjyamauchi · 2024-09-07T19:15:09Z

@swift-ci please test macOS platform

hjyamauchi · 2024-09-08T03:16:54Z

@swift-ci please test macOS platform

hjyamauchi · 2024-09-09T16:29:02Z

@swift-ci please test macOS platform

hjyamauchi · 2024-09-09T21:49:11Z

@swift-ci please test macOS platform

hjyamauchi · 2024-09-10T04:51:12Z

@swift-ci please test macOS platform

rjmccall

Why is this going through this path instead of using the proper call-lowering path?

hjyamauchi · 2024-09-11T23:17:17Z

Do you mean why all attributes aren't added in SignatureExpansion::expandExternalSignatureTypes?
I wonder that, too. I don't know this code enough to know off hand.

Would looking at these commits, which seem to have added or touched this code, help to clarify?
0e31acb
b3f302b

rjmccall · 2024-09-12T16:07:33Z

@egorzhdan Can you help out here? Why does the FunctionPointer not just have the right attributes for these calls?

egorzhdan · 2024-09-18T16:29:31Z

I'm afraid I don't know the reason for the missing attribute. Perhaps this sret/noreg logic should be moved to SignatureExpansion::expandExternalSignatureTypes?

@ahatanaka WDYT?

hjyamauchi · 2024-09-23T16:50:55Z

@rjmccall WDYT? I think I will see what I can figure out. If there are some hints or places to look at, it'd be great if you can point me to them.

hjyamauchi · 2024-09-24T19:54:14Z

Rebased. @swift-ci please test

compnerd · 2024-09-24T20:35:58Z

@ahatanaka ping

hjyamauchi · 2024-09-24T22:28:52Z

@swift-ci please test

ahatanaka · 2024-09-24T23:39:45Z

It looks like it's possible to add sret and inreg in SignatureExpansion::expandExternalSignatureTypes, but special care must be taken to handle noreturn functions correctly as SILGen changes the return type to Never.

For example, this is the SIL for the call to Large largeStructNoReturn() __attribute__((noreturn)) in testIndirectReturnNoReturn in test/IRGen/noreturn.swift.

  %0 = function_ref @largeStructNoReturn : $@convention(c) () -> Never // user: %1
  %1 = apply %0() : $@convention(c) () -> Never

hjyamauchi · 2024-09-25T00:50:47Z

@ahatanaka So it sounds like that was the reason to add sreg in IRBuilder::CreateCallOrInvoke?

@rjmccall What do you think?

hjyamauchi · 2024-10-23T18:38:39Z

As suggested, dropping the return type from the operator+= etc. only when the return type is of a reference type seems to make the above assertion failures go away. I also tried to limit this to the this pointer case but it seems to cause quite a few other test failures maybe because the conformance checking relies on the return type being void (the error details at the bottom). I think it's sufficient to drop the return type only when the return type is a reference type because that's probably what the return-type-dropping code originally intended and a reference-type return type means it's unlikely an indirect return, which solves this CGFunctionInfo/operator+= issue with the above assert failures. Please take another look @rjmccall

The test failures (with restricting to this pointer)

******************** TEST 'Swift(windows-x86_64) :: Interop/Cxx/class/protocol-conformance-typechecker.swift' FAILED ********************
S:\SourceCache\swift\test\Interop\Cxx\class\protocol-conformance-typechecker.swift:48:15: error: unexpected note produced: protocol requires function '+=' with type '(inout HasOperatorPlusEqual<CInt>, Int32) -> ()'
  static func +=(lhs: inout Self, x: Int32)
              ^
S:\SourceCache\swift\test\Interop\Cxx\class\protocol-conformance-typechecker.swift:51:1: error: unexpected error produced: type 'HasOperatorPlusEqual<CInt>' does not conform to protocol 'HasOperatorPlusEqualProtocol'
extension HasOperatorPlusEqualInt : HasOperatorPlusEqualProtocol {}
^
S:\SourceCache\swift\test\Interop\Cxx\class\protocol-conformance-typechecker.swift:51:1: error: unexpected note produced: add stubs for conformance
extension HasOperatorPlusEqualInt : HasOperatorPlusEqualProtocol {}
^

******************** TEST 'Swift(windows-x86_64) :: Interop/Cxx/stdlib/msvcprt-module-interface.swift' FAILED ********************
S:\SourceCache\swift\test\Interop\Cxx\stdlib\msvcprt-module-interface.swift:15:18: error: CHECK-STRING: expected string not found in input
// CHECK-STRING: struct basic_string<CChar, char_traits<CChar>, allocator<CChar>> : CxxRandomAccessCollection {
                 ^
<stdin>:23948:54: note: scanning from here
 static func to_wstring(_ _Val: Int32) -> std.wstring
                                                     ^

Input file: <stdin>
Check file: S:\SourceCache\swift\test\Interop\Cxx\stdlib\msvcprt-module-interface.swift

******************** TEST 'Swift(windows-x86_64) :: Interop/Cxx/stdlib/overlay/custom-collection-module-interface.swift' FAILED ********************
S:\SourceCache\swift\test\Interop\Cxx\stdlib\overlay\custom-collection-module-interface.swift:17:11: error: CHECK: expected string not found in input
// CHECK: struct SimpleCollectionReadOnly : CxxRandomAccessCollection {
          ^
<stdin>:167:2: note: scanning from here
}
 ^
<stdin>:168:1: note: possible intended match here
struct SimpleCollectionReadOnly : CxxConvertibleToCollection {
^

Input file: <stdin>
Check file: S:\SourceCache\swift\test\Interop\Cxx\stdlib\overlay\custom-collection-module-interface.swift

******************** TEST 'Swift(windows-x86_64) :: Interop/Cxx/stdlib/overlay/custom-collection.swift' FAILED ********************
S:\SourceCache\swift\test\Interop\Cxx\stdlib\overlay\custom-collection.swift:32:17: error: value of type 'SimpleCollectionReadOnly' has no member 'first'
30 | CxxCollectionTestSuite.test("SimpleCollectionReadOnly as Swift.Collection") {
31 |   let c = SimpleCollectionReadOnly()
32 |   expectEqual(c.first, 1)
   |                 `- error: value of type 'SimpleCollectionReadOnly' has no member 'first'
33 |   expectEqual(c.last, 5)
34 |

******************** TEST 'Swift(windows-x86_64) :: Interop/Cxx/stdlib/overlay/custom-iterator-module-interface.swift' FAILED ********************
S:\SourceCache\swift\test\Interop\Cxx\stdlib\overlay\custom-iterator-module-interface.swift:22:11: error: CHECK: expected string not found in input
// CHECK: struct ConstRACIteratorRefPlusEq : UnsafeCxxRandomAccessIterator, UnsafeCxxInputIterator {
          ^
<stdin>:70:2: note: scanning from here
}
 ^
<stdin>:72:1: note: possible intended match here
struct ConstRACIteratorRefPlusEq : UnsafeCxxInputIterator {
^

Input file: <stdin>
Check file: S:\SourceCache\swift\test\Interop\Cxx\stdlib\overlay\custom-iterator-module-interface.swift

******************** TEST 'Swift(windows-x86_64) :: Interop/Cxx/stdlib/use-std-span.swift' FAILED ********************
S:\SourceCache\swift\test\Interop\Cxx\stdlib\use-std-span.swift:185:18: error: cannot use mutating getter on immutable value: 's' is a 'let' constant
180 |   var arr: [Int32] = [1, 2, 3]
181 |   arr.withUnsafeMutableBufferPointer{ ubpointer in
182 |     let s = initSpan(ubpointer.baseAddress!, ubpointer.count)
    |     `- note: change 'let' to 'var' to make it mutable
183 |     expectEqual(s.size(), 3)
184 |     expectFalse(s.empty())
185 |     expectEqual(s[0], 1)
    |                  `- error: cannot use mutating getter on immutable value: 's' is a 'let' constant
186 |     expectEqual(s[1], 2)
187 |     expectEqual(s[2], 3)

******************** TEST 'Swift(windows-x86_64) :: Interop/Cxx/stdlib/use-std-string.swift' FAILED ********************
S:\SourceCache\swift\test\Interop\Cxx\stdlib\use-std-string.swift:372:14: error: for-in loop requires 'std.string' (aka 'std.basic_string<CChar, char_traits<CChar>, allocator<CChar>>') to conform to 'Sequence'
370 |     let cxx1 = std.string()
371 |     var iterated = false
372 |     for _ in cxx1 {
    |              `- error: for-in loop requires 'std.string' (aka 'std.basic_string<CChar, char_traits<CChar>, allocator<CChar>>') to conform to 'Sequence'
373 |         iterated = true
374 |     }

******************** TEST 'Swift(windows-x86_64) :: Interop/Cxx/stdlib/use-std-vector.swift' FAILED ********************
S:\SourceCache\swift\test\Interop\Cxx\stdlib\use-std-vector.swift:32:20: error: cannot convert value of type '[Any]' to expected argument type 'std.allocator<CInt>'
 30 |
 31 | StdVectorTestSuite.test("VectorOfInt.init(sequence)") {
 32 |     let v = Vector([])
    |                    `- error: cannot convert value of type '[Any]' to expected argument type 'std.allocator<CInt>'
 33 |     expectEqual(v.size(), 0)
 34 |     expectTrue(v.empty())

S:\SourceCache\swift\test\Interop\Cxx\stdlib\use-std-vector.swift:155:15: error: value of type 'Vector' (aka 'std.vector<CInt, allocator<CInt>>') has no member 'map'
153 |     fill(vector: &v)
154 |
155 |     let a = v.map { $0 + 5 }
    |               `- error: value of type 'Vector' (aka 'std.vector<CInt, allocator<CInt>>') has no member 'map'
156 |     expectEqual(a, [6, 7, 8])
157 | }

S:\SourceCache\swift\test\Interop\Cxx\stdlib\use-std-vector.swift:174:14: error: for-in loop requires 'VectorSubclass' to conform to 'Sequence'
172 |
173 |     var count: CInt = 1
174 |     for e in v {
    |              `- error: for-in loop requires 'VectorSubclass' to conform to 'Sequence'
175 |         expectEqual(e, count)
176 |         count += 1

******************** TEST 'Swift(windows-x86_64) :: Interop/CxxToSwiftToCxx/span/span-execution.cpp' FAILED ********************
S:\b\5\tools\swift\test-windows-x86_64\Interop\CxxToSwiftToCxx\span\Output\span-execution.cpp.tmp/use-span.swift:12:15: error: cannot use mutating getter on immutable value: 'sp' is a 'let' constant
10 |
11 | public func printSpan(_ sp: Span) {
12 |   print("{\(sp[0]), \(sp[1]), \(sp[2])}")
   |               `- error: cannot use mutating getter on immutable value: 'sp' is a 'let' constant
13 | }
14 |

S:\b\5\tools\swift\test-windows-x86_64\Interop\CxxToSwiftToCxx\span\Output\span-execution.cpp.tmp/use-span.swift:28:19: error: value of type 'Span' (aka 'std.span<CInt, _CUnsignedLongLong_-1>') has no member 'map'
26 |
27 | public func mapSpan(_ sp: Span) {
28 |   let result = sp.map { $0 + 3 }
   |                   `- error: value of type 'Span' (aka 'std.span<CInt, _CUnsignedLongLong_-1>') has no member 'map'
29 |   print(result)
30 | }

rjmccall

LGTM

hjyamauchi · 2024-10-23T18:42:23Z

@swift-ci please test

hjyamauchi · 2024-10-23T20:33:58Z

@swift-ci please test

hjyamauchi · 2024-10-23T20:39:10Z

@rjmccall sorry, I accidentally uploaded a wrong patch that was missing the isSRetAfterThis refactoring part (though I had tested with the right patch.) This is the same code as posted above with the debugging asserts removed. Would you take a quick look again?

rjmccall

Ah, I thought it looked a little short. LGTM.

hjyamauchi · 2024-10-24T19:20:06Z

Thanks, John! I will look into the test failures.

In GenCall, fix the IR gen for C++ method calls as under MSVC as the calling conventions for free functions and C++ methods can be different. This also fixes the missing inreg (on sret arguments) issues on Windows ARM64. Also refactor to use CGFunctionInfo returnInfo isSretAfterThis to detect when to reorder the sret and the this arguments under MSVC. In ClagImporter, don't drop the return type for the compound assignment operators such as operator+= when the return value is a reference so that the CGFunctionInfo will be correctly indicate an indirect return for the compound assignment operators.

hjyamauchi · 2024-10-25T00:08:26Z

@swift-ci please test

hjyamauchi · 2024-10-25T16:47:26Z

@swift-ci please test Windows Platform

hjyamauchi · 2024-10-25T20:32:29Z

@swift-ci please test Windows Platform

hjyamauchi · 2024-10-25T21:11:39Z

@swift-ci please test Windows Platform

hjyamauchi · 2024-10-25T22:14:06Z

@swift-ci please test Windows Platform

hjyamauchi · 2024-10-28T15:44:28Z

@rjmccall Can you take a quick look after the test fix? (for the return type change, additional attributes like noalias, nocapture and a C++ struct type name change as a consequence of the code gen change.) The CI all passed.

rjmccall

The test changes look fine, although I'm confused about why we ever used 16-byte alignment for this type. I verified that MSVC only guarantees ordinary ABI alignment for sret results, as you'd normally expect.

hjyamauchi · 2024-10-28T19:07:30Z

Thanks, John! I am not sure why the 16-byte alignment there, either 🤔 but I agree with you. If I have a chance, I'll think about this. I'll merge this, and move onto cherrypicking this along with the other commits to 6 and 5.10 to make progress in resurrecting the windows arm64 compiler.

In GenCall, fix the IR gen for C++ method calls as under MSVC as the calling conventions for free functions and C++ methods can be different. This also fixes the missing inreg (on sret arguments) issues on Windows ARM64. Also refactor to use CGFunctionInfo returnInfo isSretAfterThis to detect when to reorder the sret and the this arguments under MSVC. In ClagImporter, don't drop the return type for the compound assignment operators such as operator+= when the return value is a reference so that the CGFunctionInfo will be correctly indicate an indirect return for the compound assignment operators. Cherrypick swiftlang#76324

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 and is a partial fix for Cherrypick swiftlang#76324

In GenCall, fix the IR gen for C++ method calls as under MSVC as the calling conventions for free functions and C++ methods can be different. This also fixes the missing inreg (on sret arguments) issues on Windows ARM64. Also refactor to use CGFunctionInfo returnInfo isSretAfterThis to detect when to reorder the sret and the this arguments under MSVC. In ClagImporter, don't drop the return type for the compound assignment operators such as operator+= when the return value is a reference so that the CGFunctionInfo will be correctly indicate an indirect return for the compound assignment operators. Cherrypick swiftlang#76324

hjyamauchi marked this pull request as ready for review September 10, 2024 16:41

hjyamauchi requested review from zoecarver, hyp, egorzhdan and rjmccall as code owners September 10, 2024 16:41

hjyamauchi requested review from compnerd and removed request for rjmccall and hyp September 10, 2024 16:41

hjyamauchi mentioned this pull request Sep 10, 2024

Add the inreg attribute to sreg when present. #76159

Merged

hjyamauchi requested review from rjmccall and hyp September 11, 2024 21:16

rjmccall reviewed Sep 11, 2024

View reviewed changes

hjyamauchi mentioned this pull request Sep 12, 2024

SwiftCompilerSources doesn't work on Windows ARM64 #74866

Open

hjyamauchi force-pushed the issue74866 branch from 53072ce to a31e89d Compare September 24, 2024 19:53

hjyamauchi requested a review from ahatanaka September 25, 2024 17:01

hjyamauchi force-pushed the issue74866 branch from f1d5c59 to 50ea617 Compare October 23, 2024 18:08

hjyamauchi requested review from beccadax and ian-twilightcoder as code owners October 23, 2024 18:08

rjmccall approved these changes Oct 23, 2024

View reviewed changes

hjyamauchi changed the title ~~Fix the IR gen for C++ method calls~~ Fix the IR gen for C++ method calls and refactor around CGFunctionInfo Oct 23, 2024

hjyamauchi force-pushed the issue74866 branch from 50ea617 to 0a4889b Compare October 23, 2024 20:33

rjmccall approved these changes Oct 24, 2024

View reviewed changes

hjyamauchi force-pushed the issue74866 branch from 0a4889b to fcc1f6b Compare October 24, 2024 22:41

rjmccall approved these changes Oct 28, 2024

View reviewed changes

hjyamauchi merged commit 3c50165 into swiftlang:main Oct 28, 2024
5 checks passed

hjyamauchi mentioned this pull request Oct 28, 2024

Cherrypick fixes for issue 74866 to release/6.0 #77261

Open

hjyamauchi mentioned this pull request Oct 29, 2024

Cherrypick fixes for issue 74866 to release/5.10 #77277

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

hjyamauchi commented Sep 7, 2024

hjyamauchi commented Sep 7, 2024

hjyamauchi commented Sep 7, 2024

hjyamauchi commented Sep 8, 2024

hjyamauchi commented Sep 9, 2024

hjyamauchi commented Sep 9, 2024

hjyamauchi commented Sep 10, 2024

rjmccall left a comment

hjyamauchi commented Sep 11, 2024

rjmccall commented Sep 12, 2024

egorzhdan commented Sep 18, 2024

hjyamauchi commented Sep 23, 2024

hjyamauchi commented Sep 24, 2024

compnerd commented Sep 24, 2024

hjyamauchi commented Sep 24, 2024

ahatanaka commented Sep 24, 2024

hjyamauchi commented Sep 25, 2024

hjyamauchi commented Oct 23, 2024 •

edited

Loading

rjmccall left a comment

hjyamauchi commented Oct 23, 2024

hjyamauchi commented Oct 23, 2024

hjyamauchi commented Oct 23, 2024

rjmccall left a comment

hjyamauchi commented Oct 24, 2024

hjyamauchi commented Oct 25, 2024

hjyamauchi commented Oct 25, 2024

hjyamauchi commented Oct 25, 2024

hjyamauchi commented Oct 25, 2024

hjyamauchi commented Oct 25, 2024

hjyamauchi commented Oct 28, 2024

rjmccall left a comment

hjyamauchi commented Oct 28, 2024

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

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

Conversation

hjyamauchi commented Sep 7, 2024

hjyamauchi commented Sep 7, 2024

hjyamauchi commented Sep 7, 2024

hjyamauchi commented Sep 8, 2024

hjyamauchi commented Sep 9, 2024

hjyamauchi commented Sep 9, 2024

hjyamauchi commented Sep 10, 2024

rjmccall left a comment

Choose a reason for hiding this comment

hjyamauchi commented Sep 11, 2024

rjmccall commented Sep 12, 2024

egorzhdan commented Sep 18, 2024

hjyamauchi commented Sep 23, 2024

hjyamauchi commented Sep 24, 2024

compnerd commented Sep 24, 2024

hjyamauchi commented Sep 24, 2024

ahatanaka commented Sep 24, 2024

hjyamauchi commented Sep 25, 2024

hjyamauchi commented Oct 23, 2024 • edited Loading

rjmccall left a comment

Choose a reason for hiding this comment

hjyamauchi commented Oct 23, 2024

hjyamauchi commented Oct 23, 2024

hjyamauchi commented Oct 23, 2024

rjmccall left a comment

Choose a reason for hiding this comment

hjyamauchi commented Oct 24, 2024

hjyamauchi commented Oct 25, 2024

hjyamauchi commented Oct 25, 2024

hjyamauchi commented Oct 25, 2024

hjyamauchi commented Oct 25, 2024

hjyamauchi commented Oct 25, 2024

hjyamauchi commented Oct 28, 2024

rjmccall left a comment

Choose a reason for hiding this comment

hjyamauchi commented Oct 28, 2024

hjyamauchi commented Oct 23, 2024 •

edited

Loading