[RISCV] Enable __int128_t and __uint128_t through clang flag

[mgrang]

Summary

If the flag -fforce-enable-int128 is passed, it will enable support for int128_t and uint128_t types.
This flag can then be used to build compiler-rt for RISCV32.

[mgrang]

Related compiler-rt patch: https://reviews.llvm.org/D43106

[efriedma]

This flag can then be used to build compiler-rt for RISCV32.

Can you give a little more context for why this is necessary? As far as I know, compiler-rt generally supports building for targets which don't have int128_t. (If all riscv targets are supposed to support int128_t, you can just turn it on without adding a flag.)

lib/Basic/Targets/RISCV.h

85 ↗

(On Diff #133546)

Maybe make this a cross-platform flag, rather than riscv-specific?

test/Driver/types.c

4 ↗

(On Diff #133546)

This test won't work unless the riscv backend is enabled; maybe use -fsyntax-only?

[kito-cheng]

Hi Eli:

We need that because compiler-rt implement 128 bits soft floating point with int128_t, and RISC-V need that but RV32 doesn't support int128_t, we know it can be just return true to support that. but we don't want to bring any ABI contemptible issue between GCC and Clang/LLVM.

Here is another direction is make GCC support int128_t for RV32, but it's hard to support that in GCC for all 32 bits target[1].

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60846

[efriedma]

So you want int128_t for compiler-rt itself, so you can use the soft-float implementation, but you want to make int128_t opt-in to avoid the possibility of someone getting a link error trying to link code built with clang against libgcc.a? That seems a little convoluted, but I guess it's okay.

Not sure I like the name "-fuse-int128"; doesn't really indicate what it's doing. Maybe "-fforce-enable-int128".

[kito-cheng]

Hi Eli:

but you want to make int128_t opt-in to avoid the possibility of someone getting a link error trying to link code built with clang against libgcc.a?

Yes, that's the problem we want to avoid, and we actually get the problem if we built libc (newlib) with clang/llvm and used by GCC.

[kito-cheng]

lib/Basic/Targets/RISCV.h

85 ↗

(On Diff #133546)

+1, then we can make all other 32 bits target to able easier support float128 too :)

[apazos]

lib/Basic/Targets/RISCV.h

85 ↗

(On Diff #133546)

OK... so we can move the option check to the TargetInfo class, and the target-specific implementation overrides it as needed.

[efriedma]

include/clang/Driver/Options.td

843 ↗

(On Diff #134745)

We should have an inverse flag -fno-force-enable-int128, like we do for other "-f" flags. (Not sure anyone is actually likely to use it, but better to have it in case someone needs it.)

[asb]

In D43105#1003709, @efriedma wrote:

So you want int128_t for compiler-rt itself, so you can use the soft-float implementation, but you want to make int128_t opt-in to avoid the possibility of someone getting a link error trying to link code built with clang against libgcc.a? That seems a little convoluted, but I guess it's okay.

That's one particular problem you might encounter if Clang and gcc for RISCV32 made different choices about enabling int128 by default. More generally, we want to ensure the C environment for both GCC and Clang is as close as possible, as any difference like this has the potential for causing difficult to debug differences in behaviour that confuse end users.

[mgrang]

Updated patch to add -fno version of the flag -fforce-enable-int128.

[efriedma]

LGTM

Please post a follow-up to add this to the 7.0 release notes.

[asb]

As @efriedma noted in D43106, it would be good to have some test coverage for ABI lowering. I'd suggest updating test/Driver/riscv32-abi.c with something like:

#ifdef __SIZEOF_INT128__
// CHECK-FORCEINT128-LABEL: define i128 @f_scalar_5(i128 %x)
__int128_t f_scalar_5(__int128_t x) { return x; }
#endif

You could also check the updated defines in in test/Preprocessor/init.c (seems it should just be the addition of __SIZEOF_INT128__).

[mgrang]

Added tests for ABI lowering and preprocessor defines as per comments.

[asb]

I've added two suggestions on further tweaking the tests which I think it would be worth adopting. Looks good to me.

test/CodeGen/riscv32-abi.c

424–430 ↗

(On Diff #135493)

I'd probably insert this around line 26 (just after f_scalar_4) and put the RUN line at the very top of the file with -check-prefixes=CHECK,CHECK-FORCEINT128. This would demonstrate and verify that all other ABI lowering remains totally unaffected, which probably can't hurt. Note that $CHECKPREFIX-LABEL is a FileCheck feature, so the prefix should be defined without -LABEL.

test/Preprocessor/init.c

10213–10216 ↗

(On Diff #135493)

It would be slightly better to define this in a similar way to the RISCV32-LINUX checks, so that the test also picks up the fact that other defines (such as _INTMAX_WIDTH__) remain unmodified.

[mgrang]

Thanks @asb. I have addressed your comments.

[asb]

test/CodeGen/riscv32-abi.c

2–3 ↗

(On Diff #135540)

Nit: this is slightly different to what I suggested. -check-prefixes=CHECK,CHECK-FORCEINT128 would mean that all the other lowerings are rechecked with -fforce-eanble-int128 and are seen to remain unchanged.

[mgrang]

After addressing comments, I have now committed this patch. Thanks for all the reviews!

cfe/trunk/include/clang/Basic/TargetInfo.h

public:
 
   /// getLongLongWidth/Align - Return the size of 'signed long long' and
   /// 'unsigned long long' for this target, in bits.
   unsigned getLongLongWidth() const { return LongLongWidth; }
   unsigned getLongLongAlign() const { return LongLongAlign; }
 
   /// \brief Determine whether the __int128 type is supported on this target.
   virtual bool hasInt128Type() const {
-    return getPointerWidth(0) >= 64;
+    return (getPointerWidth(0) >= 64) || getTargetOpts().ForceEnableInt128;
   } // FIXME
 
   /// \brief Determine whether the __float128 type is supported on this target.
   virtual bool hasFloat128Type() const { return HasFloat128; }
 
   /// \brief Return the alignment that is suitable for storing any
   /// object with a fundamental alignment requirement.
   unsigned getSuitableAlign() const { return SuitableAlign; }

cfe/trunk/include/clang/Basic/TargetOptions.h

public:
   std::vector<std::string> Features;
 
   /// Supported OpenCL extensions and optional core features.
   OpenCLOptions SupportedOpenCLOptions;
 
   /// \brief The list of OpenCL extensions to enable or disable, as written on
   /// the command line.
   std::vector<std::string> OpenCLExtensionsAsWritten;
+
+  /// \brief If given, enables support for __int128_t and __uint128_t types.
+  bool ForceEnableInt128;
 };
 
 }  // end namespace clang
 
 #endif

cfe/trunk/include/clang/Driver/Options.td

def fmath_errno : Flag<["-"], "fmath-errno">, Group<f_Group>, Flags<[CC1Option]>,
   HelpText<"Require math functions to indicate errors by setting errno">;
 def fno_math_errno : Flag<["-"], "fno-math-errno">, Group<f_Group>;
 def fbracket_depth_EQ : Joined<["-"], "fbracket-depth=">, Group<f_Group>;
 def fsignaling_math : Flag<["-"], "fsignaling-math">, Group<f_Group>;
 def fno_signaling_math : Flag<["-"], "fno-signaling-math">, Group<f_Group>;
 def fjump_tables : Flag<["-"], "fjump-tables">, Group<f_Group>;
 def fno_jump_tables : Flag<["-"], "fno-jump-tables">, Group<f_Group>, Flags<[CC1Option]>,
   HelpText<"Do not use jump tables for lowering switches">;
+def fforce_enable_int128 : Flag<["-"], "fforce-enable-int128">,
+  Group<f_Group>, Flags<[CC1Option]>,
+  HelpText<"Enable support for int128_t type">;
+def fno_force_enable_int128 : Flag<["-"], "fno-force-enable-int128">,
+  Group<f_Group>, Flags<[CC1Option]>,
+  HelpText<"Disable support for int128_t type">;
 
 // Begin sanitizer flags. These should all be core options exposed in all driver
 // modes.
 let Flags = [CC1Option, CoreOption] in {
 
 def fsanitize_EQ : CommaJoined<["-"], "fsanitize=">, Group<f_clang_Group>,
                    MetaVarName<"<check>">,
                    HelpText<"Turn on runtime checks for various forms of undefined "

cfe/trunk/lib/Basic/Targets/Mips.h

static const TargetInfo::GCCRegAlias NewABIRegAliases[] = {
         {{"gp"}, "$28"}, {{"sp", "$sp"}, "$29"}, {{"fp", "$fp"}, "$30"},
         {{"ra"}, "$31"}
     };
     if (ABI == "o32")
       return llvm::makeArrayRef(O32RegAliases);
     return llvm::makeArrayRef(NewABIRegAliases);
   }
 
-  bool hasInt128Type() const override { return ABI == "n32" || ABI == "n64"; }
+  bool hasInt128Type() const override {
+    return (ABI == "n32" || ABI == "n64") || getTargetOpts().ForceEnableInt128;
+  }
 
   bool validateTarget(DiagnosticsEngine &Diags) const override;
 };
 } // namespace targets
 } // namespace clang
 
 #endif // LLVM_CLANG_LIB_BASIC_TARGETS_MIPS_H

cfe/trunk/lib/Driver/ToolChains/Clang.cpp

if (A->getOption().matches(options::OPT_fexperimental_isel)) {
         else
           D.Diag(diag::warn_drv_experimental_isel_incomplete_opt);
       }
     } else {
       CmdArgs.push_back("-global-isel=0");
     }
   }
 
+  if (Arg *A = Args.getLastArg(options::OPT_fforce_enable_int128,
+                               options::OPT_fno_force_enable_int128)) {
+    if (A->getOption().matches(options::OPT_fforce_enable_int128))
+      CmdArgs.push_back("-fforce-enable-int128");
+  }
+
   // Finally add the compile command to the compilation.
   if (Args.hasArg(options::OPT__SLASH_fallback) &&
       Output.getType() == types::TY_Object &&
       (InputType == types::TY_C || InputType == types::TY_CXX)) {
     auto CLCommand =
         getCLFallback()->GetCommand(C, JA, Output, Inputs, Args, LinkingOutput);
     C.addCommand(llvm::make_unique<FallbackCommand>(
         JA, *this, Exec, CmdArgs, Inputs, std::move(CLCommand)));

cfe/trunk/lib/Frontend/CompilerInvocation.cpp

static void ParseTargetArgs(TargetOptions &Opts, ArgList &Args,
   Opts.FPMath = Args.getLastArgValue(OPT_mfpmath);
   Opts.FeaturesAsWritten = Args.getAllArgValues(OPT_target_feature);
   Opts.LinkerVersion = Args.getLastArgValue(OPT_target_linker_version);
   Opts.Triple = llvm::Triple::normalize(Args.getLastArgValue(OPT_triple));
   // Use the default target triple if unspecified.
   if (Opts.Triple.empty())
     Opts.Triple = llvm::sys::getDefaultTargetTriple();
   Opts.OpenCLExtensionsAsWritten = Args.getAllArgValues(OPT_cl_ext_EQ);
+  Opts.ForceEnableInt128 = Args.hasArg(OPT_fforce_enable_int128);
 }
 
 bool CompilerInvocation::CreateFromArgs(CompilerInvocation &Res,
                                         const char *const *ArgBegin,
                                         const char *const *ArgEnd,
                                         DiagnosticsEngine &Diags) {
   bool Success = true;
 

cfe/trunk/test/CodeGen/riscv32-abi.c

 // RUN: %clang_cc1 -triple riscv32 -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -triple riscv32 -emit-llvm -fforce-enable-int128 %s -o - \
+// RUN: | FileCheck %s -check-prefixes=CHECK,CHECK-FORCEINT128
 
 #include <stddef.h>
 #include <stdint.h>
 
 // CHECK-LABEL: define void @f_void()
 void f_void(void) {}
 
 // Scalar arguments and return values smaller than the word size are extended

 uint8_t f_scalar_2(uint8_t x) { return x; }
 
 // CHECK-LABEL: define i32 @f_scalar_3(i32 %x)
 int32_t f_scalar_3(int32_t x) { return x; }
 
 // CHECK-LABEL: define i64 @f_scalar_4(i64 %x)
 int64_t f_scalar_4(int64_t x) { return x; }
 
+#ifdef __SIZEOF_INT128__
+// CHECK-FORCEINT128-LABEL: define i128 @f_scalar_5(i128 %x)
+__int128_t f_scalar_5(__int128_t x) { return x; }
+#endif
+
 // CHECK-LABEL: define float @f_fp_scalar_1(float %x)
 float f_fp_scalar_1(float x) { return x; }
 
 // CHECK-LABEL: define double @f_fp_scalar_2(double %x)
 double f_fp_scalar_2(double x) { return x; }
 
 // Scalars larger than 2*xlen are passed/returned indirect. However, the
 // RISC-V LLVM backend can handle this fine, so the function doesn't need to

cfe/trunk/test/Driver/types.c

+// Check whether __int128_t and __uint128_t are supported.
+
+// RUN: not %clang -c --target=riscv32-unknown-linux-gnu -fsyntax-only %s \
+// RUN: 2>&1 | FileCheck %s
+
+// RUN: %clang -c --target=riscv32-unknown-linux-gnu -fsyntax-only %s \
+// RUN: -fno-force-enable-int128 -fforce-enable-int128
+
+// RUN: not %clang -c --target=riscv32-unknown-linux-gnu -fsyntax-only %s \
+// RUN: -fforce-enable-int128 -fno-force-enable-int128
+
+void a() {
+  __int128_t s;
+  __uint128_t t;
+}
+
+// CHECK: error: use of undeclared identifier '__int128_t'
+// CHECK: error: use of undeclared identifier '__uint128_t'

cfe/trunk/test/Preprocessor/init.c

 // ARM-DARWIN-BAREMETAL-64: #define __INTPTR_TYPE__ long int
 // ARM-DARWIN-BAREMETAL-64: #define __PTRDIFF_TYPE__ long int
 // ARM-DARWIN-BAREMETAL-64: #define __SIZE_TYPE__ long unsigned int
 
 // RUN: %clang_cc1 -E -dM -ffreestanding -triple=riscv32 < /dev/null \
 // RUN:   | FileCheck -match-full-lines -check-prefix=RISCV32 %s
 // RUN: %clang_cc1 -E -dM -ffreestanding -triple=riscv32-unknown-linux < /dev/null \
 // RUN:   | FileCheck -match-full-lines -check-prefixes=RISCV32,RISCV32-LINUX %s
+// RUN: %clang_cc1 -E -dM -ffreestanding -triple=riscv32 \
+// RUN: -fforce-enable-int128 < /dev/null | FileCheck -match-full-lines \
+// RUN: -check-prefixes=RISCV32,RISCV32-INT128 %s
 // RISCV32: #define _ILP32 1
 // RISCV32: #define __ATOMIC_ACQUIRE 2
 // RISCV32: #define __ATOMIC_ACQ_REL 4
 // RISCV32: #define __ATOMIC_CONSUME 1
 // RISCV32: #define __ATOMIC_RELAXED 0
 // RISCV32: #define __ATOMIC_RELEASE 3
 // RISCV32: #define __ATOMIC_SEQ_CST 5
 // RISCV32: #define __BIGGEST_ALIGNMENT__ 16

 // RISCV32: #define __PTRDIFF_TYPE__ int
 // RISCV32: #define __PTRDIFF_WIDTH__ 32
 // RISCV32: #define __SCHAR_MAX__ 127
 // RISCV32: #define __SHRT_MAX__ 32767
 // RISCV32: #define __SIG_ATOMIC_MAX__ 2147483647
 // RISCV32: #define __SIG_ATOMIC_WIDTH__ 32
 // RISCV32: #define __SIZEOF_DOUBLE__ 8
 // RISCV32: #define __SIZEOF_FLOAT__ 4
+// RISCV32-INT128: #define __SIZEOF_INT128__ 16
 // RISCV32: #define __SIZEOF_INT__ 4
 // RISCV32: #define __SIZEOF_LONG_DOUBLE__ 16
 // RISCV32: #define __SIZEOF_LONG_LONG__ 8
 // RISCV32: #define __SIZEOF_LONG__ 4
 // RISCV32: #define __SIZEOF_POINTER__ 4
 // RISCV32: #define __SIZEOF_PTRDIFF_T__ 4
 // RISCV32: #define __SIZEOF_SHORT__ 2
 // RISCV32: #define __SIZEOF_SIZE_T__ 4