[RISCV] Force enable int128 for compiling long double routines

[mgrang]

Summary

For RISCV32, we must force enable int128 for compiling long double routines using the flag -fforce-enable-int128.

Related clang patch: https://reviews.llvm.org/D43105

[kito-cheng]

lib/builtins/CMakeLists.txt

505

Does it possible only append this flag for target which has sizeof (long double) == 16 and !defined(SIZEOF_INT128)?

[kito-cheng]

lib/builtins/CMakeLists.txt

505

Sorry I forgot to escape the code : defined(__SIZEOF_FLOAT128__) && !defined(__SIZEOF_INT128__)

[efriedma]

lib/builtins/CMakeLists.txt

505

I think it makes sense to always to force this, even if sizeof(long double) isn't 16; the 128-bit integer functions could be useful for other users.

[efriedma]

lib/builtins/CMakeLists.txt

505

Actually, thinking about it a bit more, I'm not sure it's a good idea to turn this on automatically for every target.

The key issue here is that for targets where __int128_t isn't legal, it doesn't have a defined ABI. clang will automatically come up with some ABI rules if you pass -fforce-enable-int128, but it might not be the same ABI that the author of a revision of the ABI document would choose. So if a future revision of the ABI does choose some ABI rules, we're stuck with a bunch of compiler-rt routines using the wrong rules.

For 32-bit RISCV specifically, we can avoid this risk by defining rules for int128_t in the ABI document now, even if we expect compilers won't support it by default. Not sure for other targets.

[compnerd]

Yeah, I think I would feel safer with this being limited to targets where we know that we have an explicit contract for __int128_t. Can you please limit this to the RISCV architecture for the time being?

[mgrang]

Limited patch only to RISCV target as per suggestions.

[efriedma]

RISCV ABI doc specifies the calling convention for __int128_t ("Scalars wider than 2✕XLEN are passed by reference and are replaced in the argument list with the address."). And llc, at least, appears to correctly honor that. So I think we're okay in terms of ABI stability, but someone should verify that clang emits appropriate IR.

[asb]

In D43106#1014975, @efriedma wrote:

RISCV ABI doc specifies the calling convention for __int128_t ("Scalars wider than 2✕XLEN are passed by reference and are replaced in the argument list with the address."). And llc, at least, appears to correctly honor that. So I think we're okay in terms of ABI stability, but someone should verify that clang emits appropriate IR.

Correct, the presence or absence of __int128_t will have no impact on the calling convention. Clang should handle it just fine, and D43106 should be updated to demonstrate this through a test - I'll add a more detailed comment there.

[kito-cheng]

Hi all:

I propose it should check in the foreach (arch ${BUILTIN_SUPPORTED_ARCH}), because COMPILER_RT_DEFAULT_TARGET_ARCH is not always set (I only set COMPILER_RT_DEFAULT_TARGET_TRIPLE instead of COMPILER_RT_DEFAULT_TARGET_ARCH in my build).

And that place seems more like right place to put target specific compilation flags.

diff --git a/lib/builtins/CMakeLists.txt b/lib/builtins/CMakeLists.txt
index 991108b..364a4f6 100644
--- a/lib/builtins/CMakeLists.txt
+++ b/lib/builtins/CMakeLists.txt
@@ -546,6 +546,12 @@ else ()
         list(APPEND BUILTIN_CFLAGS -fomit-frame-pointer -DCOMPILER_RT_ARMHF_TARGET)
       endif()
 
+      # For RISC-V 32 bits, we must force enable int128 for compiling long
+      # double routines.
+      if("${arch}" STREQUAL "riscv32")
+        list(APPEND BUILTIN_CFLAGS -fforce-enable-int128)
+      endif()
+
       add_compiler_rt_runtime(clang_rt.builtins
                               STATIC
                               ARCHS ${arch}

[mgrang]

In D43106#1017081, @kito-cheng wrote:

Hi all:

I propose it should check in the foreach (arch ${BUILTIN_SUPPORTED_ARCH}), because COMPILER_RT_DEFAULT_TARGET_ARCH is not always set (I only set COMPILER_RT_DEFAULT_TARGET_TRIPLE instead of COMPILER_RT_DEFAULT_TARGET_ARCH in my build).

And that place seems more like right place to put target specific compilation flags.

diff --git a/lib/builtins/CMakeLists.txt b/lib/builtins/CMakeLists.txt
index 991108b..364a4f6 100644
--- a/lib/builtins/CMakeLists.txt
+++ b/lib/builtins/CMakeLists.txt
@@ -546,6 +546,12 @@ else ()
         list(APPEND BUILTIN_CFLAGS -fomit-frame-pointer -DCOMPILER_RT_ARMHF_TARGET)
       endif()
 
+      # For RISC-V 32 bits, we must force enable int128 for compiling long
+      # double routines.
+      if("${arch}" STREQUAL "riscv32")
+        list(APPEND BUILTIN_CFLAGS -fforce-enable-int128)
+      endif()
+
       add_compiler_rt_runtime(clang_rt.builtins
                               STATIC
                               ARCHS ${arch}

So, do we want to enable this flag only based on a CMake macro like COMPILER_RT_HAS_FINT128_FLAG, or always add -fforce-enable-int128 to compiler-rt builds for RISCV32?

[kito-cheng]

So, do we want to enable this flag only based on a CMake macro like COMPILER_RT_HAS_FINT128_FLAG, or always add -fforce-enable-int128 to compiler-rt builds for RISCV32?

I think it reasonable since RISCV32 LLVM can't compile compiler-rt without this flag.

For RISCV32 GCC, here is no way to build compiler-rt since it don't support either int128_t or -fforce-enable-int128.

[kito-cheng]

LGTM, and I've tested this patch with https://reviews.llvm.org/D42958

lib/builtins/CMakeLists.txt

 if (APPLE)
   add_subdirectory(Darwin-excludes)
   add_subdirectory(macho_embedded)
   darwin_add_builtin_libraries(${BUILTIN_SUPPORTED_OS})
 else ()
   set(BUILTIN_CFLAGS "")
 
   append_list_if(COMPILER_RT_HAS_STD_C11_FLAG -std=c11 BUILTIN_CFLAGS)
+  append_list_if(COMPILER_RT_HAS_FINT128_FLAG -fforce-enable-int128 BUILTIN_CFLAGS)
 
   # These flags would normally be added to CMAKE_C_FLAGS by the llvm
   # cmake step. Add them manually if this is a standalone build.
   if(COMPILER_RT_STANDALONE_BUILD)
     append_list_if(COMPILER_RT_HAS_FPIC_FLAG -fPIC BUILTIN_CFLAGS)
     append_list_if(COMPILER_RT_HAS_FNO_BUILTIN_FLAG -fno-builtin BUILTIN_CFLAGS)
     append_list_if(COMPILER_RT_HAS_VISIBILITY_HIDDEN_FLAG -fvisibility=hidden BUILTIN_CFLAGS)
     if(NOT COMPILER_RT_DEBUG)