Page MenuHomePhabricator

[clang][CodeGen] Allow fp16 arg pass by register
AbandonedPublic

Authored by yaxunl on Feb 23 2021, 11:45 AM.

Details

Summary

HIP supports _Float16 and __fp16 types. In x86_64 host they are for storage
only. Since they have the same size and alignment as int16, they are supposed
to be passed by value in the same way as int16. Currently clang pass them
by stack when included in a struct, which is not efficient. This also causes
interoperability difficulty with gcc. On gcc since there is no _Float16 type,
int16 is used as replacement for _Float16 for passing arguments, which is
passed by register.

This patch changes x86_64 target codegen info so that _Float16 and __fp16
in structs can be passed by register.

Diff Detail

Event Timeline

yaxunl created this revision.Feb 23 2021, 11:45 AM
yaxunl requested review of this revision.Feb 23 2021, 11:45 AM
tra added a subscriber: tra.Feb 23 2021, 12:00 PM
tra added inline comments.
clang/lib/CodeGen/TargetInfo.cpp
2825

Do we need to set Hi, too? We do set it for int128.

yaxunl marked an inline comment as done.Feb 23 2021, 12:03 PM
yaxunl added inline comments.
clang/lib/CodeGen/TargetInfo.cpp
2825

Seems not. By default Hi is NoClass. I tried setting Hi to Integer and it resulted in an extra int64 argument of no use.

pengfei requested changes to this revision.Feb 23 2021, 6:02 PM
pengfei added a reviewer: erichkeane.
pengfei added inline comments.
clang/lib/CodeGen/TargetInfo.cpp
2821

It's true that AMD64 does not support _Float16, but __fp16 is supported on every target.
See https://clang.llvm.org/docs/LanguageExtensions.html#half-precision-floating-point

2822

__fp16 is storage format while _Float16 isn't.

2824

The GCC should take _Floatn as floating types, see https://gcc.gnu.org/onlinedocs/gcc-8.2.0/gcc/Floating-Types.html#Floating-Types
Can you provide the information how gcc uses as 16 bit integer for _Float16?

This revision now requires changes to proceed.Feb 23 2021, 6:02 PM
yaxunl marked 3 inline comments as done.Feb 23 2021, 7:00 PM
yaxunl added inline comments.
clang/lib/CodeGen/TargetInfo.cpp
2821

will fix comments

2822

will fix comments

2824

GCC does not allow _Float16 or __fp16 to be used for x86_64.

HIP language allows _Float16 or fp16 type to be used with x86_64. To be able to passing _Float16 or fp16 values between C++ using gcc and HIP using clang, users often write code like this:

struct half_t {
#ifdef __GNUC__
uint16_t x;
#else
_Float16 x;
#endif
};

void fun1(half_t x);

And usually fun1 is defined in HIP and compiled by clang to object file, then they call it in C++ which is compiled by gcc. Then the caller will pass half_t by register. However clang passes half_t by stack. This causes issue.

yaxunl updated this revision to Diff 325972.Feb 23 2021, 8:49 PM
yaxunl marked 2 inline comments as done.
yaxunl edited the summary of this revision. (Show Details)

revised comments and fixed test

I don't think the patch is doing right.

clang/lib/CodeGen/TargetInfo.cpp
2821

This is still not correct I think. As Clang dos says, _Float16 is not support (including load and store) unless ABI defines it.
We cannot add it before there's clear definition in the ABI.
See https://github.com/hjl-tools/x86-psABI/wiki/x86-64-psABI-1.0.pdf

2824

It looks to me this is a user scenario issue. You are using different types between GCC and Clang. There's nothing surprised if they aren't interoperable. You should use uint16_t in fun1 too instead of changing the behavior of the floating type.
Changing type behavior is dangerous, you will result in backward compatiblity problems. That's also the reason why we need a well-defined ABI :)

yaxunl abandoned this revision.Feb 25 2021, 7:03 AM
yaxunl marked an inline comment as done.
yaxunl added inline comments.
clang/lib/CodeGen/TargetInfo.cpp
2824

That makes sense. Thanks.

hliao added a subscriber: hliao.Feb 25 2021, 8:24 AM
hliao added inline comments.
clang/lib/CodeGen/TargetInfo.cpp
2821

That's quite outdated spec. Please check the latest one from https://gitlab.com/x86-psABIs/x86-64-ABI, which classify _Float16 as SSE.

hliao added inline comments.Feb 25 2021, 8:28 AM
clang/lib/CodeGen/TargetInfo.cpp
2821

That's quite outdated spec. Please check the latest one from https://gitlab.com/x86-psABIs/x86-64-ABI, which classify _Float16 as SSE.

From the log

commit 71d1183e7bb95e9f8ad732e0f2b5a4f127796e2a (origin/usr/hjl/_Float16)
Author: H.J. Lu <hjl.tools@gmail.com>
Date: Wed Feb 20 05:45:39 2019 -0800

Add optional _Float16 support

Pass and return _Float16 values in XMM registers.

diff --git a/x86-64-ABI/low-level-sys-info.tex b/x86-64-ABI/low-level-sys-info.tex
index ca84fff..0b06c56 100644

  • a/x86-64-ABI/low-level-sys-info.tex

+++ b/x86-64-ABI/low-level-sys-info.tex
@@ -25,7 +25,8 @@ object, and the term \emph{\textindex{\sixteenbyte{}}} refers to a
\subsubsection{Fundamental Types}

Figure~\ref{basic-types} shows the correspondence between ISO C's
-scalar types and the processor's. \code{int128}, \code{float80},
+scalar types and the processor's. \code{int128}, \code{_Float16},
+\code{
float80},
\code{float128}, \code{m64}, \code{m128}, \code{m256} and
\code{__m512} types are optional.

yaxunl marked an inline comment as done.Feb 25 2021, 8:42 AM

So should we revive this patch by classifying _Float16 as SSE, then it will conform to the ABI. And we should be compatible with gcc assuming it also follows the ABI. @pengfei

So should we revive this patch by classifying _Float16 as SSE, then it will conform to the ABI. And we should be compatible with gcc assuming it also follows the ABI. @pengfei

Thanks @hliao for the information. Yes, we should classify _Float16 as SSE. Sorry for the misleading.