Page MenuHomePhabricator

[RISCV] Force enable int128 for compiling long double routines
ClosedPublic

Authored by mgrang on Feb 8 2018, 5:51 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

mgrang created this revision.Feb 8 2018, 5:51 PM
Herald added subscribers: Restricted Project, llvm-commits, hintonda, mgorny. · View Herald TranscriptFeb 8 2018, 5:51 PM
kito-cheng added inline comments.Feb 11 2018, 6:53 PM
lib/builtins/CMakeLists.txt
505 ↗(On Diff #133547)

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

kito-cheng added inline comments.Feb 11 2018, 9:13 PM
lib/builtins/CMakeLists.txt
505 ↗(On Diff #133547)

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

efriedma added inline comments.
lib/builtins/CMakeLists.txt
505 ↗(On Diff #133547)

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.

mgrang updated this revision to Diff 134744.Feb 16 2018, 3:22 PM
mgrang edited the summary of this revision. (Show Details)
efriedma added inline comments.
lib/builtins/CMakeLists.txt
505 ↗(On Diff #133547)

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.

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 updated this revision to Diff 135295.Feb 21 2018, 10:44 AM
mgrang retitled this revision from [RISCV] Enable -fuse-int128 through cmake flag COMPILER_RT_HAS_FINT128_FLAG to [RISCV] Enable -fforce-enable-int128 through cmake flag COMPILER_RT_HAS_FINT128_FLAG.

Limited patch only to RISCV target as per suggestions.

mgrang marked an inline comment as done.Feb 21 2018, 10:44 AM

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 added a comment.Feb 22 2018, 5:38 AM

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.

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}

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?

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.

mgrang updated this revision to Diff 135940.Feb 26 2018, 11:50 AM
mgrang retitled this revision from [RISCV] Enable -fforce-enable-int128 through cmake flag COMPILER_RT_HAS_FINT128_FLAG to [RISCV] Force enable int128 for compiling long double routines.
mgrang edited the summary of this revision. (Show Details)
kito-cheng accepted this revision.Feb 27 2018, 9:37 PM

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

This revision is now accepted and ready to land.Feb 27 2018, 9:37 PM
This revision was automatically updated to reflect the committed changes.