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