[clang][CodeGen][MSVC] Match how MSVC returns vector types from member functions #157365
Conversation
The MSVC ABI almost always returns vector types directly, but on x86 and x86_64, there seems to be a special case for member functions, which return vector types indirectly. Fixes llvm#104.
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Henry Baba-Weiss (henrybw) ChangesThe MSVC ABI almost always returns vector types directly, but on x86 and x86_64, there seems to be a special case for member functions, which return vector types indirectly. Fixes #104. Full diff: https://github.com/llvm/llvm-project/pull/157365.diff 3 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index d1ef91b7e7c14..1be917221aa2c 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -90,6 +90,9 @@ C++ Specific Potentially Breaking Changes
ABI Changes in This Version
---------------------------
+- Fixed Microsoft calling convention for returning vector types from C++ member
+ functions. Such vector types should be returned indirectly. (GH#104)
+
AST Dumping Potentially Breaking Changes
----------------------------------------
- How nested name specifiers are dumped and printed changes, keeping track of clang AST changes.
diff --git a/clang/lib/CodeGen/MicrosoftCXXABI.cpp b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
index 88f0648660965..2d76eebcecd08 100644
--- a/clang/lib/CodeGen/MicrosoftCXXABI.cpp
+++ b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
@@ -1168,15 +1168,20 @@ static bool isTrivialForMSVC(const CXXRecordDecl *RD, QualType Ty,
}
bool MicrosoftCXXABI::classifyReturnType(CGFunctionInfo &FI) const {
- const CXXRecordDecl *RD = FI.getReturnType()->getAsCXXRecordDecl();
- if (!RD)
- return false;
-
- bool isTrivialForABI = RD->canPassInRegisters() &&
- isTrivialForMSVC(RD, FI.getReturnType(), CGM);
-
- // MSVC always returns structs indirectly from C++ instance methods.
- bool isIndirectReturn = !isTrivialForABI || FI.isInstanceMethod();
+ bool isIndirectReturn = false;
+ if (const CXXRecordDecl *RD = FI.getReturnType()->getAsCXXRecordDecl()) {
+ bool isTrivialForABI = RD->canPassInRegisters() &&
+ isTrivialForMSVC(RD, FI.getReturnType(), CGM);
+
+ // MSVC always returns structs indirectly from C++ instance methods.
+ isIndirectReturn = !isTrivialForABI || FI.isInstanceMethod();
+ } else if (isa<VectorType>(FI.getReturnType())) {
+ // On x86, MSVC seems to only return vector types indirectly from non-
+ // vectorcall C++ instance methods.
+ isIndirectReturn =
+ CGM.getTarget().getTriple().isX86() && FI.isInstanceMethod() &&
+ FI.getCallingConvention() != llvm::CallingConv::X86_VectorCall;
+ }
if (isIndirectReturn) {
CharUnits Align = CGM.getContext().getTypeAlignInChars(FI.getReturnType());
diff --git a/clang/test/CodeGenCXX/microsoft-abi-vector-types.cpp b/clang/test/CodeGenCXX/microsoft-abi-vector-types.cpp
new file mode 100644
index 0000000000000..e046fb4bb3169
--- /dev/null
+++ b/clang/test/CodeGenCXX/microsoft-abi-vector-types.cpp
@@ -0,0 +1,36 @@
+// RUN: %clang_cc1 -ffreestanding -emit-llvm %s -o - -triple=i686-pc-windows-msvc | FileCheck %s
+// RUN: %clang_cc1 -ffreestanding -emit-llvm %s -o - -triple=x86_64-pc-windows-msvc | FileCheck %s
+
+// To match the MSVC ABI, vector types must be returned indirectly from member
+// functions (as long as they do not use the vectorcall calling convention),
+// but must be returned directly everywhere else.
+
+#include <xmmintrin.h>
+
+struct Foo {
+ __m128 method_m128();
+ __m128 __vectorcall vectorcall_method_m128();
+};
+
+__m128 Foo::method_m128() {
+ return __m128{};
+// GH104
+// CHECK: store <4 x float>
+// CHECK: ret void
+}
+
+__m128 __vectorcall Foo::vectorcall_method_m128() {
+ return __m128{};
+// CHECK: ret <4 x float>
+}
+
+__m128 func_m128() {
+ return __m128{};
+// CHECK: ret <4 x float>
+}
+
+__m128 __vectorcall vectorcall_func_m128() {
+ return __m128{};
+// CHECK: ret <4 x float>
+}
+
|
|
@llvm/pr-subscribers-clang-codegen Author: Henry Baba-Weiss (henrybw) ChangesThe MSVC ABI almost always returns vector types directly, but on x86 and x86_64, there seems to be a special case for member functions, which return vector types indirectly. Fixes #104. Full diff: https://github.com/llvm/llvm-project/pull/157365.diff 3 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index d1ef91b7e7c14..1be917221aa2c 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -90,6 +90,9 @@ C++ Specific Potentially Breaking Changes
ABI Changes in This Version
---------------------------
+- Fixed Microsoft calling convention for returning vector types from C++ member
+ functions. Such vector types should be returned indirectly. (GH#104)
+
AST Dumping Potentially Breaking Changes
----------------------------------------
- How nested name specifiers are dumped and printed changes, keeping track of clang AST changes.
diff --git a/clang/lib/CodeGen/MicrosoftCXXABI.cpp b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
index 88f0648660965..2d76eebcecd08 100644
--- a/clang/lib/CodeGen/MicrosoftCXXABI.cpp
+++ b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
@@ -1168,15 +1168,20 @@ static bool isTrivialForMSVC(const CXXRecordDecl *RD, QualType Ty,
}
bool MicrosoftCXXABI::classifyReturnType(CGFunctionInfo &FI) const {
- const CXXRecordDecl *RD = FI.getReturnType()->getAsCXXRecordDecl();
- if (!RD)
- return false;
-
- bool isTrivialForABI = RD->canPassInRegisters() &&
- isTrivialForMSVC(RD, FI.getReturnType(), CGM);
-
- // MSVC always returns structs indirectly from C++ instance methods.
- bool isIndirectReturn = !isTrivialForABI || FI.isInstanceMethod();
+ bool isIndirectReturn = false;
+ if (const CXXRecordDecl *RD = FI.getReturnType()->getAsCXXRecordDecl()) {
+ bool isTrivialForABI = RD->canPassInRegisters() &&
+ isTrivialForMSVC(RD, FI.getReturnType(), CGM);
+
+ // MSVC always returns structs indirectly from C++ instance methods.
+ isIndirectReturn = !isTrivialForABI || FI.isInstanceMethod();
+ } else if (isa<VectorType>(FI.getReturnType())) {
+ // On x86, MSVC seems to only return vector types indirectly from non-
+ // vectorcall C++ instance methods.
+ isIndirectReturn =
+ CGM.getTarget().getTriple().isX86() && FI.isInstanceMethod() &&
+ FI.getCallingConvention() != llvm::CallingConv::X86_VectorCall;
+ }
if (isIndirectReturn) {
CharUnits Align = CGM.getContext().getTypeAlignInChars(FI.getReturnType());
diff --git a/clang/test/CodeGenCXX/microsoft-abi-vector-types.cpp b/clang/test/CodeGenCXX/microsoft-abi-vector-types.cpp
new file mode 100644
index 0000000000000..e046fb4bb3169
--- /dev/null
+++ b/clang/test/CodeGenCXX/microsoft-abi-vector-types.cpp
@@ -0,0 +1,36 @@
+// RUN: %clang_cc1 -ffreestanding -emit-llvm %s -o - -triple=i686-pc-windows-msvc | FileCheck %s
+// RUN: %clang_cc1 -ffreestanding -emit-llvm %s -o - -triple=x86_64-pc-windows-msvc | FileCheck %s
+
+// To match the MSVC ABI, vector types must be returned indirectly from member
+// functions (as long as they do not use the vectorcall calling convention),
+// but must be returned directly everywhere else.
+
+#include <xmmintrin.h>
+
+struct Foo {
+ __m128 method_m128();
+ __m128 __vectorcall vectorcall_method_m128();
+};
+
+__m128 Foo::method_m128() {
+ return __m128{};
+// GH104
+// CHECK: store <4 x float>
+// CHECK: ret void
+}
+
+__m128 __vectorcall Foo::vectorcall_method_m128() {
+ return __m128{};
+// CHECK: ret <4 x float>
+}
+
+__m128 func_m128() {
+ return __m128{};
+// CHECK: ret <4 x float>
+}
+
+__m128 __vectorcall vectorcall_func_m128() {
+ return __m128{};
+// CHECK: ret <4 x float>
+}
+
|
…ctly [clang][CodeGen][MSVC] Return vector types from methods indirectly The MSVC ABI almost always returns vector types directly, but on x86 and x86_64, there seems to be a special case for member functions, which return vector types indirectly. This is an ABI change and has the potential to cause backward compatibility issues with previous Clang releases. Fixes llvm#104.
llvmbot
added
the
clang:frontend
Thanks for the review! Good catch: indeed, this change will break ABI compatibility and should respect I wasn't sure if this warranted adding a test case to |
There was a problem hiding this comment.
I worry that the real invariant here is that instance methods must always pass this in the first argument register (RCX), and we've only covered a few of the cases that enable indirect return. Like, I wonder if we can hit this via member pointers.
Regardless, I don't want to expand scope, I'd rather approve the PR and handle it in a follow-up. Please update the test and we can land this.
| // functions (as long as they do not use the vectorcall calling convention), | ||
| // but must be returned directly everywhere else. | ||
|
|
||
| #include <xmmintrin.h> |
There was a problem hiding this comment.
Please extend this test to cover arm64. I fed it to an AI tool and it did the straightforward rewrite, which I think you can adapt with some ifdefs to make this a single test case:
https://godbolt.org/z/xWon6163j
This demonstrates that ARM64 passes vectors directly. Honestly, I found that surprising, because if you look into arm(64)_neon.h, you see that MSVC defines vectors as "intrinsic_type" structs, so it makes sense that "records" go down the path of "instance methods return structs indirectly".
There was a problem hiding this comment.
Done. (I was also surprised by MSVC returning vectors directly on ARM64, but that's also why I constrained this behavior to only apply to x86/x86-64 targets.)
|
…ctly Add ARM64 coverage to the test.
…ctly Simplify the test by conditionally defining the platform's vector type.
To follow up on this, I dug into this some more, and found a couple of concerning discoveries:
(Note: I'm not as familiar with ARM64 as I am with x86/x86-64, so if the ABI discrepancy there is actually user error on my part, then my apologies.) Given the discrepancy between how Anyway, given these findings, do we still think it's a good idea to land this PR? I'm wary of causing further churn in the Clang ABI, because my patch amounts to a partial fix, and I'd be happy to back this out in favor of a more complete fix. (This is my first time contributing to LLVM, so I'm not sure what the right call here is.) |
| // On x86, MSVC seems to only return vector types indirectly from non- | ||
| // vectorcall C++ instance methods. | ||
| isIndirectReturn = | ||
| CGM.getTarget().getTriple().isX86() && FI.isInstanceMethod() && |
There was a problem hiding this comment.
I think it is worth expanding this out into if blocks. I think you can put the ABI ver check into the else if condition above, and combine the vector call check with a size check for > 64 bits to be ABI compatible with __vectorcall __m64. Please add a test for that as well.
|
Thanks for researching the |
…ctly [clang][CodeGen][MSVC] Match how MSVC returns vector types from member functions The MSVC ABI usually returns vector types directly, but on x86 and x86-64, there seems to be a special case for C++ member functions, which return vector types indirectly (with the exception of member functions using the `__vectorcall` calling convention, which return vector types > 64 bits directly). This is an ABI change and has the potential to cause backward compatibility issues with previous Clang releases. Fixes llvm#104.
…ctly Rework the test to also cover 64-bit vector types.
…ctly Tweak wording of ABI note and update release note to say Clang now matches MSVC.
henrybw
changed the title
Thanks for the reviews! I have updated the PR to handle the Looks like I'll also need to rebase this before landing (my changes to the Clang ABI versions header have a merge conflict), but I wanted to get this latest iteration out for review first before rebasing, so I don't blow away the context of the existing review comments. |
…ctly Specify x86/x86-64 in release note.
|
ping |
4 participants
The MSVC ABI usually returns vector types directly, but on x86 and x86-64, there seems to be a special case for C++ member functions, which return vector types indirectly (with the exception of member functions using the
__vectorcallcalling convention, which return vector types > 64 bits directly).This is an ABI change and has the potential to cause backward compatibility issues with previous Clang releases.
Fixes #104.