<div dir="rtl"><div dir="ltr">Thanks, I have replied there. </div></div><div class="gmail_extra"><br><div class="gmail_quote"><div dir="ltr">2015-09-19 13:33 GMT+03:00 Hal Finkel <span dir="ltr"><<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></span>:</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">FYI: <a href="https://llvm.org/bugs/show_bug.cgi?id=24398" rel="noreferrer" target="_blank">https://llvm.org/bugs/show_bug.cgi?id=24398</a> was just reopened pointing to a lack of resolution here.<br>
<br>
 -Hal<br>
<div><div class="h5"><br>
----- Original Message -----<br>
> From: "Yaron Keren via cfe-commits" <<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a>><br>
> To: "Martell Malone" <<a href="mailto:martellmalone@gmail.com">martellmalone@gmail.com</a>><br>
> Cc: "Richard Smith" <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>>, "cfe-commits" <<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a>><br>
> Sent: Friday, August 21, 2015 2:47:50 AM<br>
> Subject: Re: r245459 - According to i686 ABI, long double size on x86 is 12 bytes not 16 bytes.<br>
><br>
><br>
><br>
><br>
> The testcase from r245459 was not reverted and still in SVN.<br>
><br>
><br>
><br>
><br>
><br>
> 2015-08-21 2:05 GMT+03:00 Martell Malone < <a href="mailto:martellmalone@gmail.com">martellmalone@gmail.com</a> ><br>
> :<br>
><br>
><br>
><br>
><br>
><br>
> I feel very silly now.<br>
> After testing the testcase again on svn it still works.<br>
> It appears the OP was looking for this patch to go onto the 3.6<br>
> branch and was applying my patch to that.<br>
><br>
><br>
> I'll know in future to recheck the testcase afterwards myself in<br>
> future.<br>
><br>
><br>
> Apologies for the noise guys.<br>
><br>
><br>
> Yaron I think the test case from r245459 would be useful to ensure it<br>
> is never broken in the future?<br>
><br>
> Would you be able to recommit the test case?<br>
><br>
><br>
><br>
> Kind Regards<br>
><br>
> Martell<br>
><br>
><br>
><br>
><br>
><br>
><br>
><br>
> On Thu, Aug 20, 2015 at 3:57 PM, Martell Malone <<br>
> <a href="mailto:martellmalone@gmail.com">martellmalone@gmail.com</a> > wrote:<br>
><br>
><br>
><br>
><br>
><br>
> There is no testcase for PR24398 nor the OP reporting the problem was<br>
> actually solved. Martell?<br>
> I'm just re-looking through it now.<br>
><br>
><br>
> X86TargetInfo sets LongDoubleFormat =<br>
> &llvm::APFloat::x87DoubleExtended;<br>
> X86_64TargetInfo then sets LongDoubleWidth = LongDoubleAlign = 128 ;<br>
</div></div>> X86_32 TargetInfo then sets LongDoubleWidth = 96 ; LongDoubleAlign =<br>
<span class="">> 32 ;<br>
><br>
><br>
> From this I can see that the patch I committed actually doesn't<br>
> change anything but only breaks mingw x86.<br>
><br>
><br>
> I can only see these values changed in Microsoft*TargetInfo classes<br>
> which is not a parent of MINGW<br>
><br>
><br>
> When I submitted the patch I just wanted to explicitly set the values<br>
> in MinGWX86_64TargetInfo to ensure it was in fact that.<br>
><br>
> It seemed as though it was not inheriting the value from the root<br>
> parent class somehow.<br>
><br>
><br>
><br>
> I was told on irc that it did fix the bug which is even stranger.<br>
> I'm actually at a bit of a loss as to what the proper fix to this is<br>
> then.<br>
><br>
> Apologies for breaking mingw i686 long double.<br>
><br>
><br>
> I will do up a test case and try to find the actual cause of the long<br>
> double bug and reopen the issue.<br>
><br>
><br>
><br>
><br>
><br>
> On Thu, Aug 20, 2015 at 3:09 PM, Yaron Keren < <a href="mailto:yaron.keren@gmail.com">yaron.keren@gmail.com</a><br>
> > wrote:<br>
><br>
><br>
><br>
><br>
> Hi, I've just done this exactly this in r245618 (32 bit) and r245620<br>
> (64 bits).<br>
><br>
><br>
> mingw i686 long double values were correct before r245084 and wrong<br>
> after it.<br>
> mingw x86_64 long double values were not modified at all by r245084<br>
> for the reason you stated, so I agree and do not see how this<br>
</span>> non-change can solve anything . There is no testcase for PR24398 nor<br>
<div><div class="h5">> the OP reporting the problem was actually solved. Martell?<br>
><br>
><br>
> About PR24398, long double support was in clang long ago and b oth<br>
> code examples compile and run correctly with current svn (without<br>
> r245084) and gcc version 5.1.0 (i686-posix-dwarf-rev0, Built by<br>
> MinGW-W64 project).<br>
> It's not x86_64 but as said r245084 didn't actually modify x86_64<br>
> configuration.<br>
><br>
><br>
><br>
><br>
><br>
><br>
><br>
><br>
><br>
><br>
><br>
> 2015-08-21 0:52 GMT+03:00 Richard Smith < <a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a> > :<br>
><br>
><br>
> OK, so here's the problem:<br>
><br>
><br>
> The right way to fix this seems to be to delete the assignments to<br>
> LongDouble* from the MinGWX86_32TargetInfo constructor; the<br>
> X86_32TargetInfo and X86TargetInfo base classes already set them to<br>
> the right values. Likewise we can delete the assignments to<br>
> LongDouble* from the MinGWX86_64TargetInfo constructor for the same<br>
> reason.<br>
><br>
><br>
> But... that completely reverts Martell's r245084, which apparently<br>
> fixed PR24398. So you two need to figure out what the actual problem<br>
> is here, and what the right fix is. r245084 didn't provide any test<br>
> cases, and had no apparent effect other than to break long double<br>
> for mingw32; did it really fix PR24398 (and if so, how)?<br>
><br>
><br>
><br>
><br>
><br>
> On Thu, Aug 20, 2015 at 2:27 PM, Yaron Keren < <a href="mailto:yaron.keren@gmail.com">yaron.keren@gmail.com</a><br>
> > wrote:<br>
><br>
><br>
><br>
><br>
> OK, based on testing, mingw i686 aligns long doubles to 4 bytes:<br>
><br>
><br>
><br>
><br>
> sh-4.3$ cat < a.cpp<br>
> #include <iostream><br>
> int main() {<br>
> struct {<br>
> char c[1];<br>
> long double d;<br>
> } s;<br>
> std::cout<<&s.c<<std::endl;<br>
> std::cout<<&s.d<<std::endl;<br>
> }<br>
> sh-4.3$ g++ a.cpp&&./a.exe<br>
> 0x28fea0<br>
> 0x28fea4<br>
><br>
><br>
> I'll fix that.<br>
><br>
><br>
><br>
><br>
><br>
><br>
><br>
> 2015-08-21 0:13 GMT+03:00 Richard Smith < <a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a> > :<br>
><br>
><br>
><br>
><br>
><br>
><br>
> On Wed, Aug 19, 2015 at 11:42 AM, Yaron Keren < <a href="mailto:yaron.keren@gmail.com">yaron.keren@gmail.com</a><br>
> > wrote:<br>
><br>
><br>
><br>
><br>
> Yes, it looks like a legacy issue. Documentation says so:<br>
><br>
><br>
><br>
> <a href="https://gcc.gnu.org/onlinedocs/gcc-4.8.2/gcc/i386-and-x86-64-Options.html" rel="noreferrer" target="_blank">https://gcc.gnu.org/onlinedocs/gcc-4.8.2/gcc/i386-and-x86-64-Options.html</a><br>
><br>
><br>
><br>
> -m96bit-long-double -m128bit-long-double These switches control the<br>
> size of long double type. The i386 application binary interface<br>
> specifies the size to be 96 bits, so -m96bit-long-double is the<br>
> default in 32-bit mode.<br>
><br>
> Modern architectures (Pentium and newer) prefer long double to be<br>
> aligned to an 8- or 16-byte boundary. In arrays or structures<br>
> conforming to the ABI, this is not possible. So specifying<br>
> -m128bit-long-double aligns long double to a 16-byte boundary by<br>
> padding the long double with an additional 32-bit zero.<br>
><br>
> In the x86-64 compiler, -m128bit-long-double is the default choice as<br>
> its ABI specifies that long double is aligned on 16-byte boundary.<br>
><br>
> Notice that neither of these options enable any extra precision over<br>
> the x87 standard of 80 bits for a long double .<br>
><br>
> Warning: if you override the default value for your target ABI, this<br>
> changes the size of structures and arrays containing long double<br>
> variables, as well as modifying the function calling convention for<br>
</div></div>> functions taking long double . Hence they are not binary-compatible<br>
<div><div class="h5">> with code compiled without that switch.<br>
><br>
><br>
> And practical testing agrees:<br>
><br>
><br>
><br>
><br>
> sh-4.3$ cat < a.cpp<br>
> #include <iostream><br>
> int main() {<br>
> long double a;<br>
> std::cout<<sizeof(a)<<std::endl;<br>
> }<br>
> sh-4.3$ g++ -v<br>
> Using built-in specs.<br>
> COLLECT_GCC=C:\mingw32\bin\g++.exe<br>
> COLLECT_LTO_WRAPPER=C:/mingw32/bin/../libexec/gcc/i686-w64-mingw32/5.1.0/lto-wrapper.exe<br>
> Target: i686-w64-mingw32<br>
> Configured with: ../../../src/gcc-5.1.0/configure<br>
> --host=i686-w64-mingw32 --build=i686-w64-mingw32<br>
> --target=i686-w64-mingw32 --prefix=/mingw32<br>
> --with-sysroot=/c/mingw510/i686-510-posix-dwarf-rt_v4-rev0/mingw32<br>
> --with-gxx-include-dir=/mingw32/i686-w64-mingw32/include/c++<br>
> --enable-shared --enable-static --disable-multilib<br>
> --enable-languages=c,c++,fortran,objc,obj-c++,lto<br>
> --enable-libstdcxx-time=yes --enable-threads=posix --enable-libgomp<br>
> --enable-libatomic --enable-lto --enable-graphite<br>
> --enable-checking=release --enable-fully-dynamic-string<br>
> --enable-version-specific-runtime-libs --disable-sjlj-exceptions<br>
> --with-dwarf2 --disable-isl-version-check --disable-libstdcxx-pch<br>
> --disable-libstdcxx-debug --enable-bootstrap --disable-rpath<br>
> --disable-win32-registry --disable-nls --disable-werror<br>
> --disable-symvers --with-gnu-as --with-gnu-ld --with-arch=i686<br>
> --with-tune=generic --with-libiconv --with-system-zlib<br>
> --with-gmp=/c/mingw510/prerequisites/i686-w64-mingw32-static<br>
> --with-mpfr=/c/mingw510/prerequisites/i686-w64-mingw32-static<br>
> --with-mpc=/c/mingw510/prerequisites/i686-w64-mingw32-static<br>
> --with-isl=/c/mingw510/prerequisites/i686-w64-mingw32-static<br>
> --with-pkgversion='i686-posix-dwarf-rev0, Built by MinGW-W64<br>
</div></div>> project' --with-bugurl= <a href="http://sourceforge.net/projects/mingw-w64" rel="noreferrer" target="_blank">http://sourceforge.net/projects/mingw-w64</a><br>
<div class="HOEnZb"><div class="h5">> CFLAGS='-O2 -pipe<br>
> -I/c/mingw510/i686-510-posix-dwarf-rt_v4-rev0/mingw32/opt/include<br>
> -I/c/mingw510/prerequisites/i686-zlib-static/include<br>
> -I/c/mingw510/prerequisites/i686-w64-mingw32-static/include'<br>
> CXXFLAGS='-O2 -pipe<br>
> -I/c/mingw510/i686-510-posix-dwarf-rt_v4-rev0/mingw32/opt/include<br>
> -I/c/mingw510/prerequisites/i686-zlib-static/include<br>
> -I/c/mingw510/prerequisites/i686-w64-mingw32-static/include'<br>
> CPPFLAGS= LDFLAGS='-pipe<br>
> -L/c/mingw510/i686-510-posix-dwarf-rt_v4-rev0/mingw32/opt/lib<br>
> -L/c/mingw510/prerequisites/i686-zlib-static/lib<br>
> -L/c/mingw510/prerequisites/i686-w64-mingw32-static/lib<br>
> -Wl,--large-address-aware'<br>
> Thread model: posix<br>
> gcc version 5.1.0 (i686-posix-dwarf-rev0, Built by MinGW-W64 project)<br>
><br>
><br>
> sh-4.3$ g++ a.cpp<br>
> sh-4.3$ ./a.exe<br>
><br>
> 12<br>
><br>
><br>
> Without the patch clang outputs 16 and seg faults on a boost::math<br>
> example.<br>
><br>
><br>
> None of that answers my question. Our default GCC-compatible behavior<br>
> for x86_32 long double is to give it 4-byte alignment, 12-byte size,<br>
> and this matches the behavior I observe with GCC. Does MinGW<br>
> /really/ deviate from this and give long double a 16-byte alignment?<br>
><br>
><br>
><br>
><br>
><br>
><br>
><br>
><br>
><br>
><br>
> 2015-08-19 21:29 GMT+03:00 Richard Smith < <a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a> > :<br>
><br>
><br>
><br>
><br>
><br>
><br>
> On Wed, Aug 19, 2015 at 10:02 AM, Yaron Keren via cfe-commits <<br>
> <a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a> > wrote:<br>
><br>
><br>
> Author: yrnkrn<br>
> Date: Wed Aug 19 12:02:32 2015<br>
> New Revision: 245459<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=245459&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=245459&view=rev</a><br>
> Log:<br>
> According to i686 ABI, long double size on x86 is 12 bytes not 16<br>
> bytes.<br>
> See<br>
> <a href="https://gcc.gnu.org/onlinedocs/gcc-3.2/gcc/i386-and-x86-64-Options.html" rel="noreferrer" target="_blank">https://gcc.gnu.org/onlinedocs/gcc-3.2/gcc/i386-and-x86-64-Options.html</a><br>
><br>
><br>
> Added:<br>
> cfe/trunk/test/CodeGen/mingw-long-double-size.c<br>
> Modified:<br>
> cfe/trunk/lib/Basic/Targets.cpp<br>
><br>
> Modified: cfe/trunk/lib/Basic/Targets.cpp<br>
> URL:<br>
> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/Targets.cpp?rev=245459&r1=245458&r2=245459&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/Targets.cpp?rev=245459&r1=245458&r2=245459&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/lib/Basic/Targets.cpp (original)<br>
> +++ cfe/trunk/lib/Basic/Targets.cpp Wed Aug 19 12:02:32 2015<br>
> @@ -3785,7 +3785,8 @@ class MinGWX86_32TargetInfo : public Win<br>
> public:<br>
> MinGWX86_32TargetInfo(const llvm::Triple &Triple)<br>
> : WindowsX86_32TargetInfo(Triple) {<br>
> - LongDoubleWidth = LongDoubleAlign = 128;<br>
> + LongDoubleWidth = 96;<br>
> + LongDoubleAlign = 128;<br>
><br>
><br>
><br>
> Is this really correct? It's deeply suspicious that the size is not a<br>
> multiple of the alignment.<br>
><br>
><br>
> LongDoubleFormat = &llvm::APFloat::x87DoubleExtended;<br>
> }<br>
> void getTargetDefines(const LangOptions &Opts,<br>
><br>
> Added: cfe/trunk/test/CodeGen/mingw-long-double-size.c<br>
> URL:<br>
> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/mingw-long-double-size.c?rev=245459&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/mingw-long-double-size.c?rev=245459&view=auto</a><br>
> ==============================================================================<br>
> --- cfe/trunk/test/CodeGen/mingw-long-double-size.c (added)<br>
> +++ cfe/trunk/test/CodeGen/mingw-long-double-size.c Wed Aug 19<br>
> 12:02:32 2015<br>
> @@ -0,0 +1,5 @@<br>
> +// RUN: %clang_cc1 -triple i686-pc-windows-gnu -S %s -o - |<br>
> FileCheck %s -check-prefix=CHECK_I686<br>
> +// CHECK_I686: lda,12<br>
> +// RUN: %clang_cc1 -triple x86_64-pc-windows-gnu -S %s -o - |<br>
> FileCheck %s -check-prefix=CHECK_X86_64<br>
> +// CHECK_X86_64: lda,16<br>
> +long double lda;<br>
><br>
><br>
> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
><br>
><br>
><br>
><br>
><br>
><br>
> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
><br>
<br>
</div></div><span class="HOEnZb"><font color="#888888">--<br>
Hal Finkel<br>
Assistant Computational Scientist<br>
Leadership Computing Facility<br>
Argonne National Laboratory<br>
</font></span></blockquote></div><br></div>