Skip to content

ARM: Stop setting sincos_stret calling convention #147457

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

arsenm
Copy link
Contributor

@arsenm arsenm commented Jul 8, 2025

This was going out of its way to explicitly mark these as
ARM_AAPCS_VFP. This has been explicitly set since 8b40366,
where the commit message states that "sincos" (not sincos_stret)
has a special calling convention. However, that commit also sets
the calling convention for all libcalls to ARM_AAPCS_VFP, and
getEffectiveCallingConv returns the same for CCC anyway in tests
using isWatchABI triples.

The net result of this appears to be a change in behavior when
using -float-abi=soft with isWatchABI, which have no tests so
I assume this is a theoretical combination.

If I assert

  if (getTargetMachine().getTargetTriple().isWatchABI()) {
    assert(!useSoftFloat());
    assert(getEffectiveCallingConv(CallingConv::C, false) == CallingConv::ARM_AAPCS_VFP);
  }

Only 2 tests fail the second condition, which look like copy paste accidents
using v7k triples with linux and only needed a filler triple. This is a consequence
of strangely using the target architecture in place of the OS ABI check,
as was done in 042a6c1

This was going out of its way to explicitly mark these as
ARM_AAPCS_VFP. This has been explicitly set since 8b40366,
where the commit message states that "sincos" (not sincos_stret)
has a special calling convention. However, that commit also sets
the calling convention for all libcalls to ARM_AAPCS_VFP, and
getEffectiveCallingConv returns the same for CCC anyway in tests
using isWatchABI triples.

The net result of this appears to be a change in behavior when
using -float-abi=soft with isWatchABI, which have no tests so
I assume this is a theoretical combination.

If I assert
  if (getTargetMachine().getTargetTriple().isWatchABI()) {
    assert(!useSoftFloat());
    assert(getEffectiveCallingConv(CallingConv::C, false) == CallingConv::ARM_AAPCS_VFP);
  }

Only 2 tests fail the second condition, which look like copy paste accidents
using v7k triples with linux and only needed a filler triple. This is a consequence
of strangely using the target architecture in place of the OS ABI check,
as was done in 042a6c1
@arsenm arsenm added the backend:ARM label Jul 8, 2025 — with Graphite App
Copy link
Contributor Author

arsenm commented Jul 8, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

@llvmbot
Copy link
Member

llvmbot commented Jul 8, 2025

@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-backend-arm

Author: Matt Arsenault (arsenm)

Changes

This was going out of its way to explicitly mark these as
ARM_AAPCS_VFP. This has been explicitly set since 8b40366,
where the commit message states that "sincos" (not sincos_stret)
has a special calling convention. However, that commit also sets
the calling convention for all libcalls to ARM_AAPCS_VFP, and
getEffectiveCallingConv returns the same for CCC anyway in tests
using isWatchABI triples.

The net result of this appears to be a change in behavior when
using -float-abi=soft with isWatchABI, which have no tests so
I assume this is a theoretical combination.

If I assert
if (getTargetMachine().getTargetTriple().isWatchABI()) {
assert(!useSoftFloat());
assert(getEffectiveCallingConv(CallingConv::C, false) == CallingConv::ARM_AAPCS_VFP);
}

Only 2 tests fail the second condition, which look like copy paste accidents
using v7k triples with linux and only needed a filler triple. This is a consequence
of strangely using the target architecture in place of the OS ABI check,
as was done in 042a6c1


Full diff: https://github.com/llvm/llvm-project/pull/147457.diff

1 Files Affected:

  • (modified) llvm/lib/IR/RuntimeLibcalls.cpp (-6)
diff --git a/llvm/lib/IR/RuntimeLibcalls.cpp b/llvm/lib/IR/RuntimeLibcalls.cpp
index 2cbc2cdb79685..45b778d61d8e0 100644
--- a/llvm/lib/IR/RuntimeLibcalls.cpp
+++ b/llvm/lib/IR/RuntimeLibcalls.cpp
@@ -365,12 +365,6 @@ void RuntimeLibcallsInfo::initLibcalls(const Triple &TT,
     if (darwinHasSinCosStret(TT)) {
       setLibcallImpl(RTLIB::SINCOS_STRET_F32, RTLIB::__sincosf_stret);
       setLibcallImpl(RTLIB::SINCOS_STRET_F64, RTLIB::__sincos_stret);
-      if (TT.isWatchABI()) {
-        setLibcallImplCallingConv(RTLIB::__sincosf_stret,
-                                  CallingConv::ARM_AAPCS_VFP);
-        setLibcallImplCallingConv(RTLIB::__sincos_stret,
-                                  CallingConv::ARM_AAPCS_VFP);
-      }
     }
 
     if (darwinHasExp10(TT)) {

@arsenm arsenm marked this pull request as ready for review July 8, 2025 03:50
@llvmbot llvmbot added the llvm:ir label Jul 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants