[compiler-rt] r250365 - builtins: Expand out floating point exponents for MSVC

Thu Oct 22 20:13:03 PDT 2015

Why not wait until November when Microsoft releases a clang-based compiler?

On Thu, Oct 22, 2015 at 8:08 PM, Saleem Abdulrasool via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> On Thu, Oct 22, 2015 at 8:13 AM, Stephen Canon <scanon at apple.com> wrote:
>
>> This is a bit ridiculous, and dangerous too.
>>
>> Hexadecimal FP constants are an essential tool for numerical library
>> work.  This change obfuscates the code.  The comments help somewhat, but
>> there’s no way for a casual reader to see that the values are the same.
>> They need to either take your word for it, or check each one.  The hex
>> constants are clear and unambiguous.
>>
>> Decimal-binary conversions are hard to get right, so this is also prone
>> to introducing bugs (MSVC itself has been documented to have bugs in
>> decimal-to-binary conversion routines, the very routines you’re depending
>> on here:
>> http://www.exploringbinary.com/incorrectly-rounded-conversions-in-visual-c-plus-plus/).
>> A bug introduced in that way would likely give people slightly wrong
>> answers for a long time before it was noticed.
>>
>> MSVC support is great, but at some point obfuscating code and risking
>> bugs to support a compiler that so willfully refuses to support 25-year-old
>> basic engineering tools is ridiculous.  If we really need to avoid
>> hexadecimal FP to support MSVC (ugh), *please* specify their representation
>> as hexadecimal integer literals rather than their value as decimal
>> literals, to avoid any risk of decimal-to-binary conversion producing the
>> wrong value with MSVC or some other compiler.
>
>
> I wholeheartedly agree that its unfortunate that we still have to deal
> with the lack of hex FP.
>
> To be honest, I was slightly torn on the best approach here: whether we
> should duplicate code, and just branch this as something like:
>
> #if defined(_MSC_VER)
> static const double twop52 = 4503599627370496.0;
> #else
> static const double twop52 = 0x1.0p52;
> #endif
>
> This obviously has some risk of diverging, and introduces some legibility
> issues in one sense, but also improves it in an another sense (okay, I
> admit it, I really just prefer the hex FP, and I hope that someday MSVC
> will support it and then we can drop that condition).  I would love to hear
> your take on this.  I agree, at the very least, we should change this to
> hex integer literals.  (We could do both as well!  Branch and use the hex
> constants for MSVC.)
>
>
>> – Steve
>>
>> > On Oct 15, 2015, at 12:26 AM, Saleem Abdulrasool via llvm-commits <
>> llvm-commits at lists.llvm.org> wrote:
>> >
>> > Author: compnerd
>> > Date: Wed Oct 14 23:26:19 2015
>> > New Revision: 250365
>> >
>> > URL: http://llvm.org/viewvc/llvm-project?rev=250365&view=rev
>> > Log:
>> > builtins: Expand out floating point exponents for MSVC
>> >
>> > MSVC 2013 doesnt support C99 fully, including the hexidecimal floating
>> point
>> > representation.  Use the expanded value to permit building with it.
>> >
>> > Patch by Tee Hao Wei!
>> >
>> > Modified:
>> >    compiler-rt/trunk/lib/builtins/floatdidf.c
>> >    compiler-rt/trunk/lib/builtins/floatundidf.c
>> >
>> > Modified: compiler-rt/trunk/lib/builtins/floatdidf.c
>> > URL:
>> http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/builtins/floatdidf.c?rev=250365&r1=250364&r2=250365&view=diff
>> >
>> ==============================================================================
>> > --- compiler-rt/trunk/lib/builtins/floatdidf.c (original)
>> > +++ compiler-rt/trunk/lib/builtins/floatdidf.c Wed Oct 14 23:26:19 2015
>> > @@ -32,8 +32,8 @@ ARM_EABI_FNALIAS(l2d, floatdidf)
>> > COMPILER_RT_ABI double
>> > __floatdidf(di_int a)
>> > {
>> > -     static const double twop52 = 0x1.0p52;
>> > -     static const double twop32 = 0x1.0p32;
>> > +     static const double twop52 = 4503599627370496.0; // 0x1.0p52
>> > +     static const double twop32 = 4294967296.0; // 0x1.0p32
>> >
>> >       union { int64_t x; double d; } low = { .d = twop52 };
>> >
>> >
>> > Modified: compiler-rt/trunk/lib/builtins/floatundidf.c
>> > URL:
>> http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/builtins/floatundidf.c?rev=250365&r1=250364&r2=250365&view=diff
>> >
>> ==============================================================================
>> > --- compiler-rt/trunk/lib/builtins/floatundidf.c (original)
>> > +++ compiler-rt/trunk/lib/builtins/floatundidf.c Wed Oct 14 23:26:19
>> 2015
>> > @@ -32,9 +32,9 @@ ARM_EABI_FNALIAS(ul2d, floatundidf)
>> > COMPILER_RT_ABI double
>> > __floatundidf(du_int a)
>> > {
>> > -     static const double twop52 = 0x1.0p52;
>> > -     static const double twop84 = 0x1.0p84;
>> > -     static const double twop84_plus_twop52 = 0x1.00000001p84;
>> > +     static const double twop52 = 4503599627370496.0; // 0x1.0p52
>> > +     static const double twop84 = 19342813113834066795298816.0; //
>> 0x1.0p84
>> > +     static const double twop84_plus_twop52 =
>> 19342813118337666422669312.0; // 0x1.00000001p84
>> >
>> >       union { uint64_t x; double d; } high = { .d = twop84 };
>> >       union { uint64_t x; double d; } low = { .d = twop52 };
>> >
>> >
>> > _______________________________________________
>> > llvm-commits mailing list
>> > llvm-commits at lists.llvm.org
>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>>
>
>
> --
> Saleem Abdulrasool
> compnerd (at) compnerd (dot) org
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151022/80964847/attachment.html>