[compiler-rt] Enable the new ABI of `_Float16` for Darwin on X86

[pengfei]

Summary

This is a followup of D128571, which enables the new ABI for Darwin runtimes.

The change should be covered by extendhfsf2_test.c, truncdfhf2_test.c and truncsfhf2_test.c. No new tests needed.

[benlangmuir]

compiler-rt/lib/builtins/CMakeLists.txt

694

Right now this won't work, because it needs to be sunk down somewhere inside the calls made by darwin_add_builtin_libraries on the previous line. I see darwin_add_builtin_libraries sets some CFLAGS which seems promising, or else inside darwin_add_builtin_library maybe. Hopefully someone else with more darwin compiler-rt knowledge can be more precise.

Also, what's the reason for restricting to x86 arch here? The tests (on darwin and non-darwin) seem to allow "arm|aarch64|arm64" as well, so wouldn't that be out of sync again?

@t.p.northover can you clarify whether this should be set for arm/arm64?

[pengfei]

Sink the enablement into darwin_add_builtin_libraries.

compiler-rt/lib/builtins/CMakeLists.txt

694

Thanks @benlangmuir ! Yeah, I misunderstood the code here. I think your suggestion is the right way. Updated.

Also, what's the reason for restricting to x86 arch here? The tests (on darwin and non-darwin) seem to allow "arm|aarch64|arm64" as well, so wouldn't that be out of sync again?

I'm also confused here. The tests require the COMPILER_RT_HAS_FLOAT16 to be set for arm|aarch64|arm64 so that they can use _Float16 when available. However, the libraries are still built by uint16_t due to we don't enable the macro here. It should have failed the same way as X86, but it didn't.

I'm not sure if it's a bug or intended. For conservative, restricting to x86 won't affect arm|aarch64|arm64 targets. I'm glad to remove it if ARM folks can confirm it :)

[MaskRay]

An Apple reviewer shall review this. I can only confirm that the reduced complexity in CMake makes me happy:)

[benlangmuir]

I checked this on my arm64 mac:

• These test are already failing on main branch due to the mismatch with -DCOMPILER_RT_HAS_FLOAT16
• They are still failing with this patch (due to conditionalizing the macro definition on x86)
• If I remove the conditional and always define COMPILER_RT_HAS_FLOAT16 when building the builtins for darwin, the tests pass for me. I don't know which ABI we want here for arm[64], but it confirms we need to be consistent about the conditional define.

compiler-rt/cmake/Modules/CompilerRTDarwinUtils.cmake

410 ↗

(On Diff #441609)

${arch} isn't defined here. If we want this for all architectures on Darwin, we could remove the if. If we still want to restrict it to specific arches, this check can move to darwin_add_builtin_library and it can match against ${LIB_ARCH}.

[rjmccall]

Can someone describe what the actual problem is? I don't know what the "new ABI" is supposed to be and why Darwin is apparently incompatible with it, and nothing in the linked commit suggests it has OS dependencies.

I hope we're not doing something in the _Float16 ABI that would cause incompatibilities between a translation unit compiled with SSE2 only and one compiled with AVX512-FP16. If so, we need to disable SSE2 _Float16 support immediately and fix that, because that is not acceptable.

[rjmccall]

Okay, I've had this explained to me elsewhere. Darwin relies on having the soft FP16 promotion/truncation support available in compiler-rt to support __fp16, which we support on a broader set of targets than _Float16 is enabled for. So I believe Darwin should continue to use the soft ABI for those functions. __fp16 is a storage-only format, and so we do not need to support arithmetic operations on it. That is important because we presumably do need compiler-rt support for certain kinds of arithmetic on _Complex _Float16, and that support should presumably use the hard ABI. I don't know if compiler-rt is set up to configure those functions differently from the promotion/truncation functions.

[MaskRay]

In D128872#3625755, @rjmccall wrote:

Can someone describe what the actual problem is? I don't know what the "new ABI" is supposed to be and why Darwin is apparently incompatible with it, and nothing in the linked commit suggests it has OS dependencies.

Agree that the problem is very unclear. I just know that GCC made a change on 2021-09-01 that -msse2 enables using _Float16 in C/C++.
Before that, only -mavx512fp16 enables _Float16?

Neither D128571 nor D128872 clearly states the motivation and the problem and how compatibility issues are addressed. I accepted D128571 with some discomfort that the problem was not well explained (I clicked accept more because others accepted).

[pengfei]

Thanks @benlangmuir for checking it!

Agree that the problem is very unclear. I just know that GCC made a change on 2021-09-01 that -msse2 enables using _Float16 in C/C++.
Before that, only -mavx512fp16 enables _Float16?

Yes. The series of patches are used to be compatible with GCC. GCC12 has enable _Float16 for -msse2 and up. Before these patches, Clang only enables _Float16 on -mavx512fp16.

• D107082 is a backend change which switch fromes the old (undocumented) ABI to complying with psABI. @rjmccall I mean to "the changing to psABI" when I mentioned "new ABI". I intended to repeat terminology which used in https://discourse.llvm.org/t/how-to-build-compiler-rt-for-new-x86-half-float-abi/63366 for consistency.

Neither D128571 nor D128872 clearly states the motivation and the problem and how compatibility issues are addressed. I accepted D128571 with some discomfort that the problem was not well explained (I clicked accept more because others accepted).

• D128571 is a front end change to enable using _Float16 in C/C++. It's part of the enablement of GCC compatibility. I didn't hurry for it because D113107 does the same but complete job. The motivation for D128571 is described in the description, i.e., solving #56204 and https://discourse.llvm.org/t/how-to-build-compiler-rt-for-new-x86-half-float-abi/63366. A short summary: The change in D107082 results in the new code generation incompatible with compiler-rt. So we need to quick enable _Float16 to rebuild it.

why Darwin is apparently incompatible with it, and nothing in the linked commit suggests it has OS dependencies.

• D128872 is to address the last revert of D128571. I don't fully understand why Darwin incompatible with it. Here are something I know for now: 1) Darwin has some customization in codegen https://github.com/llvm/llvm-project/blob/main/llvm/lib/CodeGen/TargetLoweringBase.cpp#L157-L163, I think this is the reason of OS dependencies. 2) This is a simple a follow up of D92241. It is not the first time to solve the ABI issue of _Float16 in compiler-rt. I thought @MaskRay might have some background about that.

Hope I addressed all your questions. Thanks!

[pengfei]

Ping. Just found I didn't post my reply to @benlangmuir

compiler-rt/cmake/Modules/CompilerRTDarwinUtils.cmake

410 ↗

(On Diff #441609)

The comment seems contradictory to me. You have checked the patch makes arm64 mac failed, so it looks to me the ${arch} has taken effort.

I'll remove the if anyway given it works on arm64. Thanks for helping on that!

[benlangmuir]

compiler-rt/cmake/Modules/CompilerRTDarwinUtils.cmake

410 ↗

(On Diff #441609)

To clarify, what I tested locally on arm64:

• The tests fail before this patch
• The tests fail with this patch
• The tests pass if I remove the ${arch} check entirely.

This is all orthogonal to the fact that the arch check wasn't going to work here, which is because this particular macro is not called per-arch.

[pengfei]

compiler-rt/cmake/Modules/CompilerRTDarwinUtils.cmake

410 ↗

(On Diff #441609)

Thanks @benlangmuir ! I see the problem now. The ${arch} doesn't make sense before the initialization in line 431. But it still blocks assigning the correct flag!

So I think it proves the patch is on the right direction for arm64 and so should be for X86. Can we land it to check for that? Or do you still have other concerns?

[benlangmuir]

compiler-rt/cmake/Modules/CompilerRTDarwinUtils.cmake

410 ↗

(On Diff #441609)

Sorry, I should have been clear: no concerns from me! But I can't review the actual change in behaviour here, so please let someone else review that before merging. Thanks!

[pengfei]

Ping. Feel free to add Apple reviewer if you happen to know :)

compiler-rt/cmake/Modules/CompilerRTDarwinUtils.cmake

410 ↗

(On Diff #441609)

Got it. Thanks @benlangmuir !

[pengfei]

Ping. We'd better to land this for LLVM 15.

compiler-rt/lib/builtins/CMakeLists.txt

 
 add_custom_target(builtins)
 set_target_properties(builtins PROPERTIES FOLDER "Compiler-RT Misc")
 
 if (APPLE)
   add_subdirectory(Darwin-excludes)
   add_subdirectory(macho_embedded)
   darwin_add_builtin_libraries(${BUILTIN_SUPPORTED_OS})
+
+  if (${arch} MATCHES "i?86|x86_64|AMD64")
+    append_list_if(COMPILER_RT_HAS_FLOAT16 -DCOMPILER_RT_HAS_FLOAT16 BUILTIN_CFLAGS)
+  endif()
 else ()
   set(BUILTIN_CFLAGS "")
 
   if (COMPILER_RT_HAS_FCF_PROTECTION_FLAG)
     append_list_if(COMPILER_RT_ENABLE_CET -fcf-protection=full BUILTIN_CFLAGS)
   endif()
 
   append_list_if(COMPILER_RT_HAS_FLOAT16 -DCOMPILER_RT_HAS_FLOAT16 BUILTIN_CFLAGS)

compiler-rt/test/builtins/CMakeLists.txt

if (${arch} STREQUAL "armhf")
     string(REPLACE ";" " " BUILTINS_TEST_TARGET_CFLAGS "${BUILTINS_TEST_TARGET_CFLAGS}")
   endif()
 
   if (${arch} STREQUAL "riscv32")
     list(APPEND BUILTINS_TEST_TARGET_CFLAGS -fforce-enable-int128)
     string(REPLACE ";" " " BUILTINS_TEST_TARGET_CFLAGS "${BUILTINS_TEST_TARGET_CFLAGS}")
   endif()
 
-  if(APPLE)
-    # TODO: Support the new ABI on Apple platforms.
-    if (${arch} MATCHES "arm|aarch64|arm64" AND COMPILER_RT_HAS_FLOAT16)
-      list(APPEND BUILTINS_TEST_TARGET_CFLAGS -DCOMPILER_RT_HAS_FLOAT16)
-      string(REPLACE ";" " " BUILTINS_TEST_TARGET_CFLAGS "${BUILTINS_TEST_TARGET_CFLAGS}")
-    endif()
-  else()
-    if (${arch} MATCHES "arm|aarch64|arm64|i?86|x86_64|AMD64" AND COMPILER_RT_HAS_FLOAT16)
-      list(APPEND BUILTINS_TEST_TARGET_CFLAGS -DCOMPILER_RT_HAS_FLOAT16)
-      string(REPLACE ";" " " BUILTINS_TEST_TARGET_CFLAGS "${BUILTINS_TEST_TARGET_CFLAGS}")
-    endif()
-  endif()
 
   if(COMPILER_RT_ENABLE_CET)
     if(NOT arch MATCHES "i?86|x86_64|AMD64")
       message(SEND_ERROR "${arch} does not support CET")
     endif()
     if(COMPILER_RT_HAS_FCF_PROTECTION_FLAG)
       list(APPEND BUILTINS_TEST_TARGET_CFLAGS -fcf-protection=full)
       string(REPLACE ";" " " BUILTINS_TEST_TARGET_CFLAGS "${BUILTINS_TEST_TARGET_CFLAGS}")