This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by pengfei on Jun 29 2022, 8:20 PM.

Details

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.

Diff Detail

Event Timeline

pengfei created this revision.Jun 29 2022, 8:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2022, 8:20 PM
pengfei requested review of this revision.Jun 29 2022, 8:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2022, 8:20 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
benlangmuir added inline comments.Jun 30 2022, 9:11 AM
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 updated this revision to Diff 441609.Jun 30 2022, 11:56 PM

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 :)

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

MaskRay resigned from this revision.Jul 1 2022, 10:35 AM

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}.

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.

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 added a subscriber: MaskRay.EditedJul 1 2022, 1:50 PM

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 updated this revision to Diff 441833.Jul 1 2022, 6:07 PM

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.

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).

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

Hope I addressed all your questions. Thanks!

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 added inline comments.Jul 6 2022, 10:18 AM
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 added inline comments.Jul 6 2022, 6:05 PM
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 added inline comments.Jul 7 2022, 10:04 AM
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!

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 !

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

ldionne resigned from this revision.Sep 11 2023, 2:44 PM
pengfei abandoned this revision.Sep 11 2023, 7:14 PM