[PATCH] D97318: [clang][CodeGen] Allow fp16 arg pass by register

[Pengfei Wang via Phabricator via cfe-commits]

pengfei added a comment.

I don't think the patch is doing right.



================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:2821
+    } else if (k == BuiltinType::Float16 || k == BuiltinType::Half) {
+      // AMD64 does not support operations on _Float16 or __fp16 other than
+      // load and store. For load/store operations, _Float16 and __fp16 is
----------------
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


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:2824
+      // which is equivalent to 16 bit integer. We need this to interop with
+      // gcc where 16 bit integer is used in place of _Float16 or __fp16.
+      Lo = Integer;
----------------
yaxunl wrote:
> pengfei wrote:
> > 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?
> 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.
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 :)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97318/new/

https://reviews.llvm.org/D97318