<div dir="ltr">Why not wait until November when Microsoft releases a clang-based compiler?</div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Oct 22, 2015 at 8:08 PM, Saleem Abdulrasool via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><span class="">On Thu, Oct 22, 2015 at 8:13 AM, Stephen Canon <span dir="ltr"><<a href="mailto:scanon@apple.com" target="_blank">scanon@apple.com</a>></span> wrote:<br></span><div class="gmail_extra"><div class="gmail_quote"><span class=""><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">This is a bit ridiculous, and dangerous too.<br>
<br>
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.<br>
<br>
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: <a href="http://www.exploringbinary.com/incorrectly-rounded-conversions-in-visual-c-plus-plus/" rel="noreferrer" target="_blank">http://www.exploringbinary.com/incorrectly-rounded-conversions-in-visual-c-plus-plus/</a>). A bug introduced in that way would likely give people slightly wrong answers for a long time before it was noticed.<br>
<br>
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.</blockquote></span><div><div><br></div><div>I wholeheartedly agree that its unfortunate that we still have to deal with the lack of hex FP.</div><div><br></div><div>To be honest, I was slightly torn on the best approach here: whether we should duplicate code, and just branch this as something like:</div><div><br></div><div>#if defined(_MSC_VER)</div><span class=""><div><span style="font-size:13px">static const double twop52 = </span><span style="font-size:13px">4503599627370496.0</span>;</div></span><div>#else</div><span class=""><div><span style="font-size:13px">static const double twop52 = 0x1.0p52;</span><br></div></span><div>#endif</div><div><br></div><div>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.)</div></div><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
– Steve<br>
<div><div><br>
> On Oct 15, 2015, at 12:26 AM, Saleem Abdulrasool via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br>
><br>
> Author: compnerd<br>
> Date: Wed Oct 14 23:26:19 2015<br>
> New Revision: 250365<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=250365&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=250365&view=rev</a><br>
> Log:<br>
> builtins: Expand out floating point exponents for MSVC<br>
><br>
> MSVC 2013 doesnt support C99 fully, including the hexidecimal floating point<br>
> representation. Use the expanded value to permit building with it.<br>
><br>
> Patch by Tee Hao Wei!<br>
><br>
> Modified:<br>
> compiler-rt/trunk/lib/builtins/floatdidf.c<br>
> compiler-rt/trunk/lib/builtins/floatundidf.c<br>
><br>
> Modified: compiler-rt/trunk/lib/builtins/floatdidf.c<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/builtins/floatdidf.c?rev=250365&r1=250364&r2=250365&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/builtins/floatdidf.c?rev=250365&r1=250364&r2=250365&view=diff</a><br>
> ==============================================================================<br>
> --- compiler-rt/trunk/lib/builtins/floatdidf.c (original)<br>
> +++ compiler-rt/trunk/lib/builtins/floatdidf.c Wed Oct 14 23:26:19 2015<br>
> @@ -32,8 +32,8 @@ ARM_EABI_FNALIAS(l2d, floatdidf)<br>
> COMPILER_RT_ABI double<br>
> __floatdidf(di_int a)<br>
> {<br>
> - static const double twop52 = 0x1.0p52;<br>
> - static const double twop32 = 0x1.0p32;<br>
> + static const double twop52 = 4503599627370496.0; // 0x1.0p52<br>
> + static const double twop32 = 4294967296.0; // 0x1.0p32<br>
><br>
> union { int64_t x; double d; } low = { .d = twop52 };<br>
><br>
><br>
> Modified: compiler-rt/trunk/lib/builtins/floatundidf.c<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/builtins/floatundidf.c?rev=250365&r1=250364&r2=250365&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/builtins/floatundidf.c?rev=250365&r1=250364&r2=250365&view=diff</a><br>
> ==============================================================================<br>
> --- compiler-rt/trunk/lib/builtins/floatundidf.c (original)<br>
> +++ compiler-rt/trunk/lib/builtins/floatundidf.c Wed Oct 14 23:26:19 2015<br>
> @@ -32,9 +32,9 @@ ARM_EABI_FNALIAS(ul2d, floatundidf)<br>
> COMPILER_RT_ABI double<br>
> __floatundidf(du_int a)<br>
> {<br>
> - static const double twop52 = 0x1.0p52;<br>
> - static const double twop84 = 0x1.0p84;<br>
> - static const double twop84_plus_twop52 = 0x1.00000001p84;<br>
> + static const double twop52 = 4503599627370496.0; // 0x1.0p52<br>
> + static const double twop84 = 19342813113834066795298816.0; // 0x1.0p84<br>
> + static const double twop84_plus_twop52 = 19342813118337666422669312.0; // 0x1.00000001p84<br>
><br>
> union { uint64_t x; double d; } high = { .d = twop84 };<br>
> union { uint64_t x; double d; } low = { .d = twop52 };<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
<br>
</div></div></blockquote></div></div></div><span class="HOEnZb"><font color="#888888"><br><br clear="all"><div><br></div>-- <br><div>Saleem Abdulrasool<br>compnerd (at) compnerd (dot) org</div>
</font></span></div></div>
<br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div>