Page MenuHomePhabricator

[RISCV] Enable __int128_t and __uint128_t through clang flag
ClosedPublic

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

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

mgrang created this revision.Feb 8 2018, 5:49 PM

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 added a comment.EditedFeb 8 2018, 6:33 PM

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

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".

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 added inline comments.Feb 11 2018, 7:26 PM
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 added inline comments.Feb 14 2018, 3:01 PM
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.

mgrang updated this revision to Diff 134745.Feb 16 2018, 3:23 PM
mgrang edited the summary of this revision. (Show Details)
efriedma added inline comments.Feb 16 2018, 5:59 PM
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 added a comment.Feb 17 2018, 6:07 AM

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 this revision to Diff 135112.Feb 20 2018, 11:19 AM

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

mgrang marked an inline comment as done.Feb 20 2018, 11:20 AM
efriedma accepted this revision.Feb 20 2018, 1:10 PM

LGTM

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

This revision is now accepted and ready to land.Feb 20 2018, 1:10 PM
asb added a comment.Feb 22 2018, 6:07 AM

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 updated this revision to Diff 135493.Feb 22 2018, 11:51 AM
mgrang retitled this revision from [RISCV] Enable __int128_t and uint128_t through clang flag to [RISCV] Enable __int128_t and __uint128_t through clang flag.
mgrang edited the summary of this revision. (Show Details)

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

asb accepted this revision.Feb 22 2018, 3:06 PM

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 updated this revision to Diff 135540.Feb 22 2018, 4:01 PM

Thanks @asb. I have addressed your comments.

asb added inline comments.Feb 22 2018, 4:09 PM
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 updated this revision to Diff 135553.Feb 22 2018, 4:20 PM

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

This revision was automatically updated to reflect the committed changes.