llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
29.06k stars 11.98k forks source link

Calling convention differs from MSVC for member functions returning SIMD types #104

Open lhallam opened 4 years ago

lhallam commented 4 years ago
#include "xmmintrin.h"

class Foo {
public:
  __declspec(noinline) __m128 bar() { 
    return __m128{}; 
  }
};

clang-cl returns in xmm0, msvc takes a second hidden parameter to write to, so when the caller and callee come from objects created by different compilers any integer parameters are offset and the return value is incorrect.

The visual studio documentation (https://docs.microsoft.com/en-us/cpp/build/x64-calling-convention?view=vs-2019#return-values) seem to imply that clang's behaviour is correct, unless __m128 etc are considered user defined types.

llvmbot commented 2 years ago

@llvm/issue-subscribers-backend-x86

tru commented 3 months ago

We just got bit by this as well where we linked to a MSVC compiled library that returned a SIMD type. I would like to fix this so that since I think not being compatible with the MSVC ABI is a bug.

If someone can point me in the right direction of where to look I can try to put up a PR.

cc @rnk @efriedma-quic @aganea

efriedma-quic commented 3 months ago

MicrosoftCXXABI::classifyReturnType in clang/lib/CodeGen/MicrosoftCXXABI.cpp is the code that handles this sort of thing. Assuming I'm understanding the issue correctly; this is specifically for instance methods of C++ classes, right?

sylvain-audi commented 3 months ago

Here is a simplified repro: https://godbolt.org/z/TM6rW738v

rnk commented 3 months ago

I think this is a better godbolt reproducer: https://godbolt.org/z/adxj84Tjq

The __m128 naming is significant, because that is a struct definition provided by xmmintrin.h, which is different between MSVC and Clang.

We could return all vector types from C++ instance methods indirectly, but that includes user-defined vector types, which are not required to be ABI-compatible with MSVC. We probably can't reliably differentiate vector types provided by intrinsic headers, so that's probably the correct solution. Code is here: https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/MicrosoftCXXABI.cpp#L1177

Maybe this is a good first issue for a potential contributor.

llvmbot commented 3 months ago

Hi!

This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:

  1. Check that no other contributor has already been assigned to this issue. If you believe that no one is actually working on it despite an assignment, ping the person. After one week without a response, the assignee may be changed.
  2. In the comments of this issue, request for it to be assigned to you, or just create a pull request after following the steps below. Mention this issue in the description of the pull request.
  3. Fix the issue locally.
  4. Run the test suite locally. Remember that the subdirectories under test/ create fine-grained testing targets, so you can e.g. use make check-clang-ast to only run Clang's AST tests.
  5. Create a Git commit.
  6. Run git clang-format HEAD~1 to format your changes.
  7. Open a pull request to the upstream repository on GitHub. Detailed instructions can be found in GitHub's documentation. Mention this issue in the description of the pull request.

If you have any further questions about this issue, don't hesitate to ask via a comment in the thread below.

llvmbot commented 3 months ago

@llvm/issue-subscribers-good-first-issue

Author: Lewis Hallam (lhallam)

```c++ #include "xmmintrin.h" class Foo { public: __declspec(noinline) __m128 bar() { return __m128{}; } }; ``` `clang-cl` returns in `xmm0`, msvc takes a second hidden parameter to write to, so when the caller and callee come from objects created by different compilers any integer parameters are offset and the return value is incorrect. The visual studio documentation (https://docs.microsoft.com/en-us/cpp/build/x64-calling-convention?view=vs-2019#return-values) seem to imply that clang's behaviour is correct, unless __m128 etc are considered user defined types.