Page MenuHomePhabricator

[builtins] Enable CRT_HAS_128BIT for MSVC
Needs RevisionPublic

Authored by rongjiecomputer on Jan 7 2018, 6:22 PM.

Details

Summary

This allow clang/MSVC to use __int128 and get runtime functions from clang_rt.builtins-<arch>.lib.

The following tests are temporarily disabled for MSVC as both pure MSVC and clang/MSVC do not have __float128 and 80-bit precision long double. Fix them when clang/MSVC exposes __float128 and 80-bit precision long double.

  • fixunsxfti_test.c
  • fixxfti_test.c
  • floattixf_test.c
  • floatuntixf_test.c

Diff Detail

Event Timeline

rongjiecomputer created this revision.Jan 7 2018, 6:22 PM
Herald added subscribers: Restricted Project, aheejin. · View Herald TranscriptJan 7 2018, 6:22 PM
rongjiecomputer edited the summary of this revision. (Show Details)Jan 7 2018, 6:53 PM
joerg added a subscriber: joerg.Jan 7 2018, 11:51 PM

This works in 32bit mode as well? I'm suprised.

@joerg You are right. I have checked that __int128 is not available when compile with -target i386-windows-msvc.

Added extra guard with _WIN64.

Gentle ping after 7 days.

rnk accepted this revision.Jan 17 2018, 10:03 AM

Thanks, looks good. I will apply this.

This revision is now accepted and ready to land.Jan 17 2018, 10:03 AM
rnk requested changes to this revision.Jan 17 2018, 1:24 PM

I get these test failures in check-builtins now, so I can't merge this: https://reviews.llvm.org/P8060

This revision now requires changes to proceed.Jan 17 2018, 1:24 PM

There is a compile warning for char assumption_3[sizeof(long double)*CHAR_BIT == 128] = {0} in these codes because long double == double in Clang + MSVC:

  • fixunsxfti_test.c
  • fixxfti_test.c
  • floattixf_test.c
  • floatuntixf_test.c

mulvti3_test.c hit a compilerrt_abort:

..\lib\builtins\mulvti3.c:50: abort in __mulvti3

Summary of other test failures:

fixunsdfti_test.c
error in __fixunsdfti(0X1.FFFFFFFFFFFFFP+63) = 0x0000000000000000FFFFFFFFFFFFF800, expected 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFF800

fixunssfti_test.c
error in __fixunssfti(0X1.FFFFFE0000000P+63) = 0x0000000000000000FFFFFF0000000000, expected 0xFFFFFFFFFFFFFFFFFFFFFF0000000000

fixunsxfti_test.c
error in __fixunsxfti(0X1.0000000000000P+0) = 0x00000000000000000000000000000000, expected 0x00000000000000000000000000000001

fixxfti_test.c
error in __fixxfti(0X1.0000000000000P+0) = 0x00000000000000000000000000000000, expected 0x00000000000000000000000000000001

floattixf_test.c
error in __floattixf(0x00000000000000000000000000000001) = 0X1.0000000000000P-1022, expected 0X1.0000000000000P+0

floatuntisf_test.c
error in __floatuntisf(0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFE) = inf, expected 0x1.0000000000000p+64

floatuntixf_test.c
error in __floatuntixf(0x00000000000000000000000000000001) = 0X1.0000000000000P-1022, expected 0X1.0000000000000P+0

modti3_test.c
error in __modti3: 0xFFFFFFFFFFFFFFFF8000000000000000 % 0x00000000000000000000000000000003 = 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFE, expected 0x00000000000000000000000000000002

muloti4_test.c
error in __muloti4: overflow=1 expected=0
error in __muloti4: 0x000000000000000000000000B504F333 * 0x000000000000000000000000B504F333 = 0x00000000000000007FFFFFFE9EA1DC29, expected 0x00000000000000007FFFFFFE9EA1DC29

Since the tests work on other platforms, I am guessing the tests fail for clang-cl due to Clang trying to imitate MSVC's bugs. I am not sure how to make them work, so I might have to abandon this patch.

Thanks for your time to review this.

rongjiecomputer edited the summary of this revision. (Show Details)

Fix several bugs that only appear on clang/MSVC.

Disable the following tests for MSVC due to lack of __float128 and 80-bit precision long double

  • fixunsxfti_test.c
  • fixxfti_test.c
  • floattixf_test.c
  • floatuntixf_test.c
ninja -C build check-builtins
ninja: Entering directory `build'
[0/1] Running the Builtins tests
-- Testing: 194 tests, 4 threads --
Testing: 0 .. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90..
Testing Time: 357.44s
  Expected Passes    : 167
  Expected Failures  : 2
  Unsupported Tests  : 25

@rnk I decided to continue to work on this patch, can you take a look?

Explanation:

1.) Change the suffix of some hex literals from LL to ULL to work around the following clang/MSVC's "bug":

http://coliru.stacked-crooked.com/a/e090d6db12ac32aa

On GCC/Mingw and GCC/Linux, the output is:

Hello
0000000000000000FFFFFFFFFFFFF800
0000000000000000FFFFFFFFFFFFF800

But on Clang/MSVC, it is:

World
FFFFFFFFFFFFFFFFFFFFFFFFFFFFF800
0000000000000000FFFFFFFFFFFFF800

2.) Change __int128's / operations to explicit __divti3 function calls, also to work around clang/MSVC's bug.

3.) Disable the following failing tests (all related to __float128/long double which clang/MSVC does not have):

  • fixunsxfti_test.c
  • fixxfti_test.c
  • floattixf_test.c
  • floatuntixf_test.c

All *xf* builtin functions won't work as they require long double to be 128 bits, but I don't bother to
disable them right now as clang/MSVC won't emit calls to these functions anyway (until __float128 is exposed).

I plan to start a discussion in cfe-dev about exposing __float128 and long double for clang/MSVC.
clang/MSVC already exposes __int128, I think exposing __float128 should be low-risk.

Do we really need to follow all the bugs in pure MSVC?

joerg requested changes to this revision.Feb 19 2018, 9:14 AM
joerg added inline comments.
lib/builtins/muloti4.c
56

I find this and related changes to be not acceptable. It hard-codes the assumption that the division is not lowered natively by the compiler, which is IMO not valid.

This revision now requires changes to proceed.Feb 19 2018, 9:14 AM
lib/builtins/muloti4.c
56

By "related change", I assume it means changes in muloti4.c, mulvti3.c and int_lib.h only.

clang/MSVC has a very weird bug. clang/MSVC does lower ti_int '/' to __divti3. For large ti_int values, if the values are known in compile time (forced by constexpr), / will produce correct answer, but if the values are only known in runtime, / will sometime return 0. / still works for small runtime values. __divti3 always works.

Also, [1], [2] etc directly calls __divsi3 instead of letting compiler to lower int division. I do not see why explicitly calling __divti3 is so harmful here.

[1]: https://github.com/llvm-mirror/compiler-rt/blob/master/lib/builtins/divmodsi4.c#L22
[2]: https://github.com/llvm-mirror/compiler-rt/blob/master/lib/builtins/modsi3.c#L22

rnk resigned from this revision.Mar 5 2018, 1:12 PM

I'm not a compiler-rt person and I can't find time for this.

joerg added a comment.Mar 5 2018, 2:01 PM

The difference is that modsi3 etc are all paired instructions. A backend should not be lowering to one of them if a real division instruction exists and it should be consistent in the lowering.

orivej added a subscriber: orivej.Aug 11 2018, 3:25 PM

Hello,

I landed here after having discussed https://bugs.llvm.org/show_bug.cgi?id=25305

Can someone please provide feedback on the status of this PR please?

Thank you

I am not working on this CL anymore. Changes need to be made in LLVM first to make passing of __int128 and long double
works with Microsoft ABI and I don't have such knowledge to make it happen.

After LLVM and Clang are fixed, then this CL should work.

Some links that might help future contributors who want to try this: