This is an archive of the discontinued LLVM Phabricator instance.

[flang][runtime] Enabled HAS_FLOAT128 for builds with clang.
ClosedPublic

Authored by vzakhari on Sep 22 2022, 5:01 PM.

Details

Summary

I am building with clang, and I noticed for quite a while that
REAL(16) runtimes functions are not available. I did not know why
until I saw this typo.

Diff Detail

Event Timeline

vzakhari created this revision.Sep 22 2022, 5:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 22 2022, 5:01 PM
Herald added a subscriber: jdoerfert. · View Herald Transcript
vzakhari requested review of this revision.Sep 22 2022, 5:01 PM
klausler accepted this revision.Sep 23 2022, 9:04 AM
This revision is now accepted and ready to land.Sep 23 2022, 9:04 AM
brooks added a subscriber: brooks.Sep 28 2022, 4:49 PM

This breaks the build on at least x86_64-unknown-freebsd13.1 with:

In file included from /home/bed22/git/llvm-project/flang/lib/Optimizer/Builder/Runtime/Reduction.cpp:15:
In file included from /home/bed22/git/llvm-project/flang/include/flang/Runtime/reduction.h:15:
/home/bed22/git/llvm-project/flang/include/flang/Runtime/cpp-type.h:55:25: error: __float128 is not supported on this target
using CppFloat128Type = __float128;
                        ^
1 error generated.

This breaks the build on at least x86_64-unknown-freebsd13.1 with:

In file included from /home/bed22/git/llvm-project/flang/lib/Optimizer/Builder/Runtime/Reduction.cpp:15:
In file included from /home/bed22/git/llvm-project/flang/include/flang/Runtime/reduction.h:15:
/home/bed22/git/llvm-project/flang/include/flang/Runtime/cpp-type.h:55:25: error: __float128 is not supported on this target
using CppFloat128Type = __float128;
                        ^
1 error generated.

Thank you for reporting this! Can you please share details about the environment that you are using for the build and the cmake command?

This was on FreeBSD 13.1 amd64. The cmake command for the bisect was:

cmake -G Ninja -DLLVM_ENABLE_PROJECTS="clang;lld;flang" ../llvm

I've got things building again with:

--- flang/include/flang/Runtime/float128.h.orig
+++ flang/include/flang/Runtime/float128.h
@@ -22,7 +22,7 @@
 
 #undef HAS_FLOAT128
 #if __x86_64__
-#if __GNUC__ >= 7 || __clang_major__ >= 7
+#if !defined(__FreeBSD__) && (__GNUC__ >= 7 || __clang_major__ >= 7)
 #define HAS_FLOAT128 1
 #endif
 #elif defined __PPC__ && __GNUC__ >= 8

but it's not clear to me that this is the right approach. I wonder if there's a better way to determine that the compiler and runtime supports float128?

You should be building with clang - is that correct assumption? What clang version are you using? I just want to be able to reproduce it.

Please feel free to merge your change so that you are unblocked. I will try to detect __float128 support in CMake, but, first, I want to understand why the problem happens.

This is clang 13.0.0.

FreeBSD clang version 13.0.0 (git@github.com:llvm/llvm-project.git llvmorg-13.0.0-0-gd7b669b3a303)
Target: x86_64-unknown-freebsd13.1
Thread model: posix
InstalledDir: /usr/bin

I'm beginning to suspect this issue is a lack of runtime support for __float128, but I'm far from certain.

This is clang 13.0.0.

FreeBSD clang version 13.0.0 (git@github.com:llvm/llvm-project.git llvmorg-13.0.0-0-gd7b669b3a303)
Target: x86_64-unknown-freebsd13.1
Thread model: posix
InstalledDir: /usr/bin

I'm beginning to suspect this issue is a lack of runtime support for __float128, but I'm far from certain.

Thank you! I will try to reproduce it. Please let me know if you want me to merge the !defined(__FreeBSD__) workaround to unblock your work.

I'm looking into getting __float128 support enabled on FreeBSD (it's a trivial patch, but I'm not an expert in the area so am not sure if we're missing something in the runtime we need to fix before switching). It will be at least a year before all supported FreeBSD version include the change though so adjusting this guard makes sense. I did verify that both clang and GCC of appropriate vintage define __SIZEOF_FLOAT128__ on Linux so maybe we should just use that instead of version checks unless there are know-bad versions.

flang/include/flang/Runtime/float128.h
24–30

An alternative might be to drop the compiler version guards entirely in favor of #ifdef __SIZEOF_FLOAT128__

vzakhari added inline comments.Oct 3 2022, 11:37 AM
flang/include/flang/Runtime/float128.h
24–30

This seems reasonable to me.

BTW, I found this code in GCC's x86_64-pc-linux-gnu/bits/c++config.h:

/* Define if __float128 is supported on this host. */
#if defined(__FLOAT128__) || defined(__SIZEOF_FLOAT128__)
#define _GLIBCXX_USE_FLOAT128 1
#endif

Maybe we can use #if defined(__FLOAT128__) || defined(__SIZEOF_FLOAT128__) as well.

Hi, I have also started hitting build errors with __float128 support when building flang using libc++. When I try to build using libc++ on an x86_64-unknown-linux-gnu host I see an error that there is an ambiguous call to copysign (the error I am seeing is the same as is reported here: https://github.com/llvm/llvm-project/issues/57992). Based on the comment in the issue, it seems __float128 is not supported by libc++ at the moment and I think this issue was "hidden" by the typo fixed in this patch.

Would it be acceptable to additionally disable __float128 when building with libc++ for now or is there a better way of handling this? I'm happy to try to resolve this, but I was wondering if you had any suggestions on how this should be handled. Thanks!

Hi, I have also started hitting build errors with __float128 support when building flang using libc++. When I try to build using libc++ on an x86_64-unknown-linux-gnu host I see an error that there is an ambiguous call to copysign (the error I am seeing is the same as is reported here: https://github.com/llvm/llvm-project/issues/57992). Based on the comment in the issue, it seems __float128 is not supported by libc++ at the moment and I think this issue was "hidden" by the typo fixed in this patch.

Would it be acceptable to additionally disable __float128 when building with libc++ for now or is there a better way of handling this? I'm happy to try to resolve this, but I was wondering if you had any suggestions on how this should be handled. Thanks!

:( yes, I think it makes sense to disable F128 for libc++ builds, e.g. via checking for _LIBCPP_VERSION. So it looks like we need to guard it with #if (defined(__FLOAT128__) || defined(__SIZEOF_FLOAT128__)) && !defined(_LIBCPP_VERSION)