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

Thu Oct 22 21:20:33 PDT 2015

On Thu, Oct 22, 2015 at 8:13 PM, David Majnemer <david.majnemer at gmail.com>
wrote:

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

It will still be a while before we can say that the minimum requirement for
building is Visual Studio 2016 (AIUI, the Nov release is a CTP).

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

-- 
Saleem Abdulrasool
compnerd (at) compnerd (dot) org
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151022/f3b1a58a/attachment.html>