Page MenuHomePhabricator

[Builtins] Fix test cases for xf/ti
Needs ReviewPublic

Authored by weimingz on Apr 1 2017, 4:33 PM.

Details

Summary

guard the tests that assume 80-bit floating point type representation with macro,
so targets that assumes __fp128 (IEEE quad-precision floating type) will be skipped.

This patch address PR 32260 (https://bugs.llvm.org//show_bug.cgi?id=32260)

Diff Detail

Event Timeline

weimingz created this revision.Apr 1 2017, 4:33 PM

After we agree on the patch, __fixxfti() will be fixed similarly.

joerg edited edge metadata.Apr 2 2017, 1:20 AM

This doesn't make sense to me. XF *is* for modes with Intel extended precision. IEEE 128bit floats should be TF.

I see. Should we mark the test as "Unsupported" for aarch64?

joerg added a comment.Apr 2 2017, 5:22 AM

That would be perfectly reasonable. Some other places already tag it with HAS_80_BIT_LONG_DOUBLE, but I don't know what is responsible for setting that macro.

weimingz updated this revision to Diff 93913.Apr 3 2017, 12:28 PM
weimingz edited the summary of this revision. (Show Details)

I guess HAS_80_BIT_LONG_DOUBLE is passed as some build-time flags. I can't find it anywhere in the compiler-rt source tree that defines it.

I think we should guard it by checking "LDBL_MANT_DIG".

weimingz updated this revision to Diff 93918.Apr 3 2017, 12:32 PM
joerg added a comment.Apr 3 2017, 1:32 PM

Please do not drop checks for CRT_HAS_128BIT -- the code requirse__ int128_t and doesn't work on platforms without.

If the platform doesn't support it, the LDBL_MANT_DIG will be 53 (refer to clang/test/Preprocessor/init.c )

If the platform doesn't support it, the LDBL_MANT_DIG will be 53

CRT_HAS_128BIT refers to support for 128-bit integers (__int128_t), which is unrelated to the size of long double.

weimingz updated this revision to Diff 93950.Apr 3 2017, 2:36 PM

Change to HAS_80_BIT_LONG_DOUBLE just like other tests.
Also, if HAS_80_BIT_LONG_DOUBLE is not defined, we can set it if CRT_HAS_128BIT && LDBL_MANT_DIG == 64. So even if the build doesn't specify it, the test won't be skipped by mistake.

efriedma added inline comments.Apr 3 2017, 2:44 PM
lib/builtins/int_types.h
72 ↗(On Diff #93950)

32-bit x86 Linux/Mac/Windows/etc. targets have 80-bit long double, but no __int128_t support.

weimingz added inline comments.Apr 3 2017, 3:14 PM
lib/builtins/int_types.h
72 ↗(On Diff #93950)

the lib routines like fixunsxfti.c, floattixf.c etc are guarded by CRT_HAS_128BIT(that is LP64) macro.

efriedma added inline comments.Apr 3 2017, 3:31 PM
lib/builtins/int_types.h
72 ↗(On Diff #93950)

The routines you mention are guarded by CRT_HAS_128BIT, but other 80-bit float routines aren't, e.g. __fixxfdi.

weimingz updated this revision to Diff 93961.Apr 3 2017, 3:49 PM

seems no way to detect if 80-bit ldbl is supported or not. Leave it to the build time flags.

In my view, LDBL_MANT_DIG == 64 implies HAS_80_BIT_LONG_DOUBLE, but need t to exclude MIPS as it clang BE doesn't support it.

There are two kinds of systems with 80-bit long double: 64-bit x86, and 32-bit x86. On 32-bit x86 with 80-bit long double, CRT_HAS_128BIT is false, and __LDBL_MANT_DIG__ is 64. On 64-bit x86 with 80-bit long double, CRT_HAS_128BIT is true, and __LDBL_MANT_DIG__ is 64.

For the 80-bit long double tests which don't use 128-bit integers, you want to run them for all systems where LDBL_MANT_DIG is 64. For the 80-bit long double tests which do use 128-bit integers, you want to run them only on systems where CRT_HAS_128BIT is set and __LDBL_MANT_DIG__ is 64.

weimingz updated this revision to Diff 93967.Apr 3 2017, 4:21 PM

these tests are the only 4 tests that use 128 bit long long.
Other tests that uses HAS_80_BIT_LONG_DOUBLE should be replaced with __LDBL_MANT_DIG__ == 64

joerg added a comment.Apr 4 2017, 4:32 AM

Actually, there are three platforms that use: i386, amd64 and m68k. The latter might not currently be supported by LLVM, but it still exists.
__int64_t and therefore CRT_HAS_128BIT are supported by all 64bit platforms and only those. The two features are orthogonal.

In short, the combination of "TI mode" and "XF mode" is only amd64, "XF mode" applies to i386, amd64 and m68k.

weimingz retitled this revision from [Builtins] Add IEEE 754 support for __fixunsxfti to [Builtins] Fix test cases for xf/ti.Apr 5 2017, 11:31 PM

Thanks for the explanation. The current patch should address the problem, right?