Page MenuHomePhabricator

[X86] Support `_Float16` on SSE2 and up
ClosedPublic

Authored by pengfei on Jun 24 2022, 8:31 PM.

Diff Detail

Event Timeline

pengfei created this revision.Jun 24 2022, 8:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 24 2022, 8:31 PM
Herald added a subscriber: jsji. · View Herald Transcript
pengfei requested review of this revision.Jun 24 2022, 8:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 24 2022, 8:31 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
bkramer accepted this revision.Jun 25 2022, 5:08 AM

Thanks

This revision is now accepted and ready to land.Jun 25 2022, 5:08 AM
rjmccall added inline comments.Jun 25 2022, 8:47 AM
clang/docs/LanguageExtensions.rst
746–752

Could you take the wording I suggested from the other patch? You'll need to drop the part about avoiding intermediate truncations, but it's important to still document the practical/performance difference when AVX512-FP16 is not available, even if the observable behavior is the same.

clang/docs/ReleaseNotes.rst
523
clang/test/SemaCXX/Float16.cpp
8

This test (and Float16.c) should continue to have positive and negative examples even if generic x86_64 is no longer negative. Generic i386 should still be negative, for example.

pengfei updated this revision to Diff 440016.Jun 25 2022, 6:14 PM
pengfei marked 3 inline comments as done.

Address review comments. Thanks @rjmccall !

rjmccall accepted this revision.Jun 26 2022, 11:33 PM

Thank you, LGTM.

zahiraam accepted this revision.Jun 27 2022, 4:51 AM

LGTM.

bgraur added a subscriber: bgraur.Jun 27 2022, 5:04 AM
This revision was automatically updated to reflect the committed changes.
vitalybuka reopened this revision.Jun 27 2022, 12:43 PM
This revision is now accepted and ready to land.Jun 27 2022, 12:43 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 27 2022, 11:40 PM
Herald added subscribers: Restricted Project, Enna1, mgorny. · View Herald Transcript

Thanks @vitalybuka ! I believe the fail was caused by missing COMPILER_RT_HAS_FLOAT16 in these tests. Relanded.

This broke some compiler-rt tests on Darwin:
https://green.lab.llvm.org/green/job/clang-stage1-RA/29920/

Test Result (3 failures / +3)
Builtins-x86_64-darwin.Builtins-x86_64-darwin.extendhfsf2_test.c
Builtins-x86_64-darwin.Builtins-x86_64-darwin.truncdfhf2_test.c
Builtins-x86_64-darwin.Builtins-x86_64-darwin.truncsfhf2_test.c

Reverted in eab2a06f0fde due to the Darwin test failures.

@pengfei could you fix the Darwin tests as well? And a general comment regarding the ongoing _Float16 effort: I think that this change should have been a part of https://reviews.llvm.org/D107082 to make it possible to build a consistently working toolchain. Thus, if this commit can't be landed in a reasonable time, I'd suggest reverting https://reviews.llvm.org/D107082.

Thanks @benlangmuir for the revert. The problem seems Darwin supports the _Float16 type already but with a different ABI. I have no idea how to solve the problem ATM. Post a question on discourse: https://discourse.llvm.org/t/compiler-rt-tests-fail-on-darwin-stage1-build-after-the-abi-change-of-half-type-on-x86/63508

@pengfei could you fix the Darwin tests as well? And a general comment regarding the ongoing _Float16 effort: I think that this change should have been a part of https://reviews.llvm.org/D107082 to make it possible to build a consistently working toolchain. Thus, if this commit can't be landed in a reasonable time, I'd suggest reverting https://reviews.llvm.org/D107082.

@alexfh I'm working on that. I'm asking suggestion on solving it in a better way, but at least we can disable the test for Darwin (maybe just for stage1 if possible) since it's expected due to the ABI change.

pengfei reopened this revision.Jun 29 2022, 5:17 PM
This revision is now accepted and ready to land.Jun 29 2022, 5:17 PM
pengfei updated this revision to Diff 441222.Jun 29 2022, 6:33 PM

Disable extendhfsf2/truncsfhf2 tests on Darwin to avoid the fail.

@pengfei could you fix the Darwin tests as well? And a general comment regarding the ongoing _Float16 effort: I think that this change should have been a part of https://reviews.llvm.org/D107082 to make it possible to build a consistently working toolchain. Thus, if this commit can't be landed in a reasonable time, I'd suggest reverting https://reviews.llvm.org/D107082.

@alexfh I'm working on that. I'm asking suggestion on solving it in a better way, but at least we can disable the test for Darwin (maybe just for stage1 if possible) since it's expected due to the ABI change.

Disabled these tests for Darwin. I'll reland the patch in one day if no objections.

pengfei updated this revision to Diff 441236.Jun 29 2022, 7:54 PM

Exclude the ABI change on Darwin platform. Will enable it by a followup.

MaskRay requested changes to this revision.Jun 29 2022, 10:59 PM
MaskRay added a subscriber: MaskRay.
MaskRay added inline comments.
clang/test/CodeGen/X86/Float16-arithmetic.c
2

Use one line for RUN. This isn't long. Delete excess spaces.

x86_64-unknown-unknown can be simplified as x86_64

3

Delete --check-prefixes=CHECK. This is the default.

5

The test only has negative patterns. Such a test is prone to be stale without being noticed.

Add some positive CHECK lines.

This revision now requires changes to proceed.Jun 29 2022, 10:59 PM
MaskRay added inline comments.Jun 29 2022, 11:01 PM
clang/lib/Basic/Targets/X86.cpp
357

_Float16

for x86 convey no extra information since this file is for x86.

pengfei updated this revision to Diff 441272.Jun 29 2022, 11:14 PM

Address review comments. Thanks @MaskRay !

MaskRay accepted this revision.Jun 29 2022, 11:52 PM
MaskRay added inline comments.
clang/lib/Basic/Targets/X86.cpp
357

Thinking again: The comment just repeats what the code does. So it can be deleted.

This revision is now accepted and ready to land.Jun 29 2022, 11:52 PM
pengfei added inline comments.Jun 29 2022, 11:55 PM
clang/lib/Basic/Targets/X86.cpp
357

Yeah, I had the same feeling when updating. Will delete, thanks! :)

This revision was landed with ongoing or failed builds.Jun 30 2022, 2:21 AM
This revision was automatically updated to reflect the committed changes.