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

hjyamauchi · hjyamauchi · commit b61997eec341 · 2024-11-14T17:50:46.000-08:00
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 commit fcc1f6b Cherrypick PR #76324
diff --git a/lib/ClangImporter/ImportType.cpp b/lib/ClangImporter/ImportType.cpp
@@ -2101,10 +2101,11 @@ ImportedType ClangImporter::Implementation::importFunctionReturnType(
   // C++ operators +=, -=, *=, /= may return a reference to self. This is not
   // idiomatic in Swift, let's drop these return values.
   clang::OverloadedOperatorKind op = clangDecl->getOverloadedOperator();
-  if (op == clang::OverloadedOperatorKind::OO_PlusEqual ||
-      op == clang::OverloadedOperatorKind::OO_MinusEqual ||
-      op == clang::OverloadedOperatorKind::OO_StarEqual ||
-      op == clang::OverloadedOperatorKind::OO_SlashEqual)
+  if ((op == clang::OverloadedOperatorKind::OO_PlusEqual ||
+       op == clang::OverloadedOperatorKind::OO_MinusEqual ||
+       op == clang::OverloadedOperatorKind::OO_StarEqual ||
+       op == clang::OverloadedOperatorKind::OO_SlashEqual) &&
+      clangDecl->getReturnType()->isReferenceType())
     return {SwiftContext.getVoidType(), false};
 
   // Fix up optionality.
diff --git a/lib/IRGen/GenCall.cpp b/lib/IRGen/GenCall.cpp
@@ -1433,9 +1433,15 @@ void SignatureExpansion::expandExternalSignatureTypes() {
   // Generate function info for this signature.
   auto extInfo = clang::FunctionType::ExtInfo();
 
-  auto &FI = clang::CodeGen::arrangeFreeFunctionCall(IGM.ClangCodeGen->CGM(),
-                                             clangResultTy, paramTys, extInfo,
-                                             clang::CodeGen::RequiredArgs::All);
+  bool isCXXMethod =
+      FnType->getRepresentation() == SILFunctionTypeRepresentation::CXXMethod;
+  auto &FI = isCXXMethod ?
+      clang::CodeGen::arrangeCXXMethodCall(IGM.ClangCodeGen->CGM(),
+          clangResultTy, paramTys, extInfo, {},
+          clang::CodeGen::RequiredArgs::All) :
+      clang::CodeGen::arrangeFreeFunctionCall(IGM.ClangCodeGen->CGM(),
+          clangResultTy, paramTys, extInfo, {},
+          clang::CodeGen::RequiredArgs::All);
   ForeignInfo.ClangInfo = &FI;
 
   assert(FI.arg_size() == paramTys.size() &&
@@ -1557,9 +1563,7 @@ void SignatureExpansion::expandExternalSignatureTypes() {
   if (returnInfo.isIndirect()) {
     auto resultType = getSILFuncConventions().getSingleSILResultType(
         IGM.getMaximalTypeExpansionContext());
-    if (IGM.Triple.isWindowsMSVCEnvironment() &&
-        FnType->getRepresentation() ==
-            SILFunctionTypeRepresentation::CXXMethod) {
+    if (returnInfo.isSRetAfterThis()) {
       // Windows ABI places `this` before the
       // returned indirect values.
       emitArg(0);
@@ -2406,11 +2410,12 @@ class SyncCallEmission final : public CallEmission {
 
         // Windows ABI places `this` before the
         // returned indirect values.
-        bool isThisFirst = IGF.IGM.Triple.isWindowsMSVCEnvironment();
-        if (!isThisFirst)
+        auto &returnInfo =
+            getCallee().getForeignInfo().ClangInfo->getReturnInfo();
+        if (returnInfo.isIndirect() && !returnInfo.isSRetAfterThis())
           passIndirectResults();
         adjusted.add(arg);
-        if (isThisFirst)
+        if (returnInfo.isIndirect() && returnInfo.isSRetAfterThis())
           passIndirectResults();
       }
 
@@ -2950,9 +2955,10 @@ void CallEmission::emitToUnmappedMemory(Address result) {
   assert(LastArgWritten == 1 && "emitting unnaturally to indirect result");
 
   Args[0] = result.getAddress();
-  if (IGF.IGM.Triple.isWindowsMSVCEnvironment() &&
-      getCallee().getRepresentation() ==
-          SILFunctionTypeRepresentation::CXXMethod &&
+
+  auto *FI = getCallee().getForeignInfo().ClangInfo;
+  if (FI && FI->getReturnInfo().isIndirect() &&
+      FI->getReturnInfo().isSRetAfterThis() &&
       Args[1] == getCallee().getCXXMethodSelf()) {
     // C++ methods in MSVC ABI pass `this` before the
     // indirectly returned value.
@@ -3335,10 +3341,10 @@ void CallEmission::emitToExplosion(Explosion &out, bool isOutlined) {
       emitToMemory(temp, substResultTI, isOutlined);
       return;
     }
-    if (IGF.IGM.Triple.isWindowsMSVCEnvironment() &&
-        getCallee().getRepresentation() ==
-            SILFunctionTypeRepresentation::CXXMethod &&
-        substResultType.isVoid()) {
+
+    auto *FI = getCallee().getForeignInfo().ClangInfo;
+    if (FI && FI->getReturnInfo().isIndirect() &&
+        FI->getReturnInfo().isSRetAfterThis() && substResultType.isVoid()) {
       // Some C++ methods return a value but are imported as
       // returning `Void` (e.g. `operator +=`). In this case
       // we should allocate the correct temp indirect return
diff --git a/test/Interop/Cxx/class/method/Inputs/inreg-sret.h b/test/Interop/Cxx/class/method/Inputs/inreg-sret.h
@@ -0,0 +1,11 @@
+#ifndef TEST_INTEROP_CXX_CLASS_METHOD_INREG_SRET_H
+#define TEST_INTEROP_CXX_CLASS_METHOD_INREG_SRET_H
+
+struct OptionalBridgedBasicBlock {
+};
+
+struct BridgedFunction {
+  OptionalBridgedBasicBlock getFirstBlock() const { return {}; }
+};
+
+#endif // TEST_INTEROP_CXX_CLASS_METHOD_INREG_SRET_H
diff --git a/test/Interop/Cxx/class/method/Inputs/module.modulemap b/test/Interop/Cxx/class/method/Inputs/module.modulemap
@@ -14,3 +14,8 @@ module UnsafeProjections {
   
   export *
 }
+
+module InRegSRet {
+  header "inreg-sret.h"
+  requires cplusplus
+}
diff --git a/test/Interop/Cxx/class/method/inreg-sret.swift b/test/Interop/Cxx/class/method/inreg-sret.swift
@@ -0,0 +1,28 @@
+// RUN: %target-swift-emit-irgen -I %S/Inputs -cxx-interoperability-mode=default %s | %FileCheck %s -check-prefix=CHECK -check-prefix=CHECK-%target-cpu
+
+// REQUIRES: OS=windows-msvc
+
+import InRegSRet
+
+final public class BasicBlock {
+}
+
+extension OptionalBridgedBasicBlock {
+  public var block: BasicBlock? { nil }
+}
+
+final public class Function {
+  public var bridged: BridgedFunction {
+    BridgedFunction()
+  }
+
+  public var firstBlock : BasicBlock? { bridged.getFirstBlock().block }
+}
+
+// Check that inreg on the sret isn't missing
+
+// CHECK-x86_64: call void @"?getFirstBlock@BridgedFunction@@QEBA?AUOptionalBridgedBasicBlock@@XZ"(ptr {{.*}}, ptr noalias nocapture sret(%TSo25OptionalBridgedBasicBlockV) {{.*}})
+// CHECK-aarch64: call void @"?getFirstBlock@BridgedFunction@@QEBA?AUOptionalBridgedBasicBlock@@XZ"(ptr {{.*}}, ptr inreg noalias nocapture sret(%TSo25OptionalBridgedBasicBlockV) {{.*}})
+
+// CHECK-x86_64: define {{.*}} void @"?getFirstBlock@BridgedFunction@@QEBA?AUOptionalBridgedBasicBlock@@XZ"(ptr {{.*}} %{{.*}}, ptr noalias sret(%struct.OptionalBridgedBasicBlock) {{.*}} %{{.*}})
+// CHECK-aarch64: define {{.*}} void @"?getFirstBlock@BridgedFunction@@QEBA?AUOptionalBridgedBasicBlock@@XZ"(ptr {{.*}} %{{.*}}, ptr inreg noalias sret(%struct.OptionalBridgedBasicBlock) {{.*}} %{{.*}})
diff --git a/test/Interop/Cxx/class/method/msvc-abi-return-indirect-trivial-record.swift b/test/Interop/Cxx/class/method/msvc-abi-return-indirect-trivial-record.swift
@@ -57,14 +57,15 @@ public func test(_ result: VecStr) -> CInt {
 }
 
 // CHECK: swiftcc i32 @"$s4Test4testys5Int32VSo0014vecstr_yuJCataVF"({{.*}} %[[RESULT:.*]])
-// CHECK: call void @"?begin@?$vec@Vstr@@@@QEBA?AVIt@@XZ"(ptr %[[RESULT]], ptr sret{{.*}}
+// CHECK: call void @"?begin@?$vec@Vstr@@@@QEBA?AVIt@@XZ"(ptr %[[RESULT]], ptr {{.*}} sret{{.*}}
 
 public func passTempForIndirectRetToVoidCall() {
     var lhs = LoadableIntWrapper(value: 2)
     let rhs = LoadableIntWrapper(value: 2)
     lhs += rhs
 }
 
-// CHECK: void @"$sSo18LoadableIntWrapperV2peoiyyABz_ABtFZ"(ptr
-// CHECK: %[[OPRESULT:.*]] = alloca %struct.LoadableIntWrapper, align 16
-// CHECK: call void @"??YLoadableIntWrapper@@QEAA?AU0@U0@@Z"(ptr {{.*}}, ptr sret(%struct.LoadableIntWrapper) %[[OPRESULT]], i32
+// CHECK: i32 @"$sSo18LoadableIntWrapperV2peoiyA2Bz_ABtFZ"(ptr
+// CHECK: %[[OPRESULT:.*]] = alloca %TSo18LoadableIntWrapperV, align 4
+// CHECK-x86_64: call void @"??YLoadableIntWrapper@@QEAA?AU0@U0@@Z"(ptr {{.*}}, ptr {{.*}} sret(%TSo18LoadableIntWrapperV) %[[OPRESULT]], i32
+// CHECK-aarch64: call void @"??YLoadableIntWrapper@@QEAA?AU0@U0@@Z"(ptr {{.*}}, ptr {{.*}} sret(%TSo18LoadableIntWrapperV) %[[OPRESULT]], i64

Original file line number	Diff line number	Diff line change
`@@ -14,3 +14,8 @@ module UnsafeProjections {`
`14`	`14`
`15`	`15`	`export *`
`16`	`16`	`}`
	`17`	`+`
	`18`	`+module InRegSRet {`
	`19`	`+ header "inreg-sret.h"`
	`20`	`+ requires cplusplus`
	`21`	`+}`