Skip to content

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
@arsenm
Copy link
Contributor Author

arsenm commented Jul 14, 2025

ping

Copy link
Collaborator

@efriedma-quic efriedma-quic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Ideally, someone from Apple would look at this... but it's unlikely anyone cares; 32-bit WatchOS is dead.

@arsenm arsenm merged commit d1db176 into main Jul 14, 2025
14 checks passed
@arsenm arsenm deleted the users/arsenm/arm/stop-explicitly-setting-sincos-stret-calling-conv branch July 14, 2025 23:30
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.

4 participants