This is an archive of the discontinued LLVM Phabricator instance.

[hip] Claim builtin type `__float128` supported if the host target supports it.
ClosedPublic

Authored by hliao on Apr 20 2020, 12:36 PM.

Diff Detail

Event Timeline

hliao created this revision.Apr 20 2020, 12:36 PM

Currently if instructions of float128 get to amdgpu backend, are we going to crash?

hliao added a comment.EditedApr 20 2020, 2:20 PM

Currently if instructions of float128 get to amdgpu backend, are we going to crash?

As Float128Format is re-defined as double, there won't be any issue in the backend. But, it won't function as the developer expects. That's quite similar to long double in clang.

yaxunl accepted this revision.Apr 21 2020, 11:10 AM

LGTM. Thanks.

This revision is now accepted and ready to land.Apr 21 2020, 11:10 AM
This revision was automatically updated to reflect the committed changes.

Currently if instructions of float128 get to amdgpu backend, are we going to crash?

As Float128Format is re-defined as double, there won't be any issue in the backend. But, it won't function as the developer expects. That's quite similar to long double in clang.

I'd argue that it is quite similarly broken. Silently miscompiling the program is arguably the worst. Are you sure the OpenMP/Sycl way to deal with this is not better? Long story short,
allow the presence of a type that the aux-triple supports but do not allow the use of such a type. The implementation is not perfect yet, e.g., it doesn't allow the above typedef and sometimes a long double is manifested in IR, but for the most part it's functional: clang/test/OpenMP/nvptx_unsupported_type_messages.cpp and clang/test/SemaSYCL/float128.cpp after D74387. TBH, I believe an error is still better than treating long double and fp128 on the device side different and then hope that it somehow works.

FWIW, I landed here after I wrote D95665 to address PR48923. I think the ideas there and here are contrary and we should pick one.