107498 2013-01-21 20:46:17 -0800 TRANSFORMATION_MATRIX_USE_X86_64_SSE2 broken for 64-bit Windows builds 2013-01-22 13:21:37 -0800 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 jschuh jschuh benjamin jamesr webkit.review.bot oldest_to_newest 812568 0 jschuh 2013-01-21 20:46:17 -0800 Looks like a simple bug in this conditional macro: 72 #if CPU(X86_64) && !PLATFORM(WINDOWS) 73 #define TRANSFORMATION_MATRIX_USE_X86_64_SSE2 74 #endif The problem is that !PLATFORM(WINDOWS) doesn't do anything. So TRANSFORMATION_MATRIX_USE_X86_64_SSE2 gets defined but the compiler explodes on the alignment specifier. I assume it was meant to be PLATFORM(WIN), but COMPILER(MSVC) is probably more correct. I could just fix the macro to exclude Windows, but the SSE intrinsics are all supported on Win64. So, is there any reason I can't just add an #if with a __declspec to set the correct alignment and enable this for Win64? (I ask, because at the moment I'm just a few dependencies short of a build I can actually test with on Win64.) 812572 1 183881 jschuh 2013-01-21 20:55:52 -0800 Created attachment 183881 Patch for discussion. 812599 2 benjamin 2013-01-21 22:00:53 -0800 I am sorry. I intended to use OS(WINDOWS). We cannot enable that optimization as is for Windows because of alignment issues. Neither the allocator, nor the stack alignment gives us the right garanties for alignment. Stricto sensu, it is not correct on Unix either without a custom new() with an aligned allocation. But in practice we know the ABI and how our allocator works on the platforms we support. 812613 3 jschuh 2013-01-21 22:20:30 -0800 Stack and struct/class member alignment can be guaranteed with __declspec(align), as in the patch I posted. And the 64-bit Windows heap guarantees 16-byte alignment, as does Chromium's 64-bit TCMalloc port. That said, you're right that it would still be relying on quirks rather than explicit guarantees. So, if you're not comfortable with me enabling it for any Windows 64-bit build, do you have any objections to me doing so for just the 64-bit Windows Chromium port? (That is, assuming @jamesr isn't aware of something preventing this for 64-bit Chromium ports.) 812618 4 jamesr 2013-01-21 22:37:23 -0800 On Chromium Windows the allocator can be selected at runtime with an environment variable. Last I looked, we supported tcmalloc, jemalloc, and winheap at least. Is this true for the 64 bit build? Do we know the alignment properties of all of the possible allocators? 812633 5 benjamin 2013-01-21 23:02:52 -0800 (In reply to comment #3) > Stack and struct/class member alignment can be guaranteed with __declspec(align), as in the patch I posted. This is not true on x86, but according to http://msdn.microsoft.com/en-us/library/aa290049(v=vs.71).aspx, you are right for x86_64. > And the 64-bit Windows heap guarantees 16-byte alignment, as does Chromium's 64-bit TCMalloc port. > > That said, you're right that it would still be relying on quirks rather than explicit guarantees. So, if you're not comfortable with me enabling it for any Windows 64-bit build, do you have any objections to me doing so for just the 64-bit Windows Chromium port? (That is, assuming @jamesr isn't aware of something preventing this for 64-bit Chromium ports.) I have no objection against enabling this on Windows. I just don't know enough about Windows x86_64 to r+. If you enable this for Windows, can you introduce a new macro to do __attribute__((aligned (16))) || __declspec(align(16))? 812663 6 jamesr 2013-01-21 23:45:27 -0800 > > > And the 64-bit Windows heap guarantees 16-byte alignment, as does Chromium's 64-bit TCMalloc port. > > > > That said, you're right that it would still be relying on quirks rather than explicit guarantees. So, if you're not comfortable with me enabling it for any Windows 64-bit build, do you have any objections to me doing so for just the 64-bit Windows Chromium port? (That is, assuming @jamesr isn't aware of something preventing this for 64-bit Chromium ports.) > > I have no objection against enabling this on Windows. I just don't know enough about Windows x86_64 to r+. > > If you enable this for Windows, can you introduce a new macro to do __attribute__((aligned (16))) || __declspec(align(16))? __attribute goes after the type, __declspec goes before. You can write a function-like macro for members and locals, but it won't work for type declarations.I think standard practice is to have both pre and post macros. 813092 7 jschuh 2013-01-22 09:27:47 -0800 (In reply to comment #3) > I have no objection against enabling this on Windows. I just don't know enough about Windows x86_64 to r+. Gotcha. If I do propose turning it on I'll make sure to find an appropriate reviewer. Sadly, I don't think anyone other than myself is actually working a Win64 port, so I doubt I can get *that* appropriate of a reviewer. (In reply to comment #4) > On Chromium Windows the allocator can be selected at runtime with an environment variable. Yup, but on win64 we don't support custom allocators yet so it's winheap only. That said, the only thing I'm aware of us using the allocator switching for is winheap under instrumentation, so we're basically safe right now. > Last I looked, we supported tcmalloc, jemalloc, and winheap at least. Is this true for the 64 bit build? Do we know the alignment properties of all of the possible allocators? You forgot Windows LFH. :) And yes, they all have a minimum 16-byte alignment on 64-bit. After we get a basic build up and running we'll eventually enable the custom allocators and test each, just to be sure. (In reply to comment #6) (In reply to comment #5) > > If you enable this for Windows, can you introduce a new macro to do __attribute__((aligned (16))) || __declspec(align(16))? > > __attribute goes after the type, __declspec goes before. You can write a function-like macro for members and locals, but it won't work for type declarations.I think standard practice is to have both pre and post macros. So, add something like WTF_ALIGNED_TYPE_START(align) and WTF_ALIGNED_TYPE_END(align) to Alignment.h? Feels a bit horsey, but it's probably a better solution than anything more clever. 813305 8 183881 jamesr 2013-01-22 12:17:33 -0800 Comment on attachment 183881 Patch for discussion. I think this is fine. We can add a macro when we get a few more callers - at this point I think it's a bit premature. 813319 9 benjamin 2013-01-22 12:39:50 -0800 I was thinking of something like: #define WTF_ALIGNED(declaration, alignment) \ declaration __attribute__((aligned (alignment))); > I think this is fine. We can add a macro when we get a few more callers - at this point I think it's a bit premature. Fair enough, I don't have a problem with that. 813320 10 jschuh 2013-01-22 12:43:37 -0800 Since the verdict seems to be that this patch is fine, I'm just going to cq+. 813348 11 183881 webkit.review.bot 2013-01-22 13:21:34 -0800 Comment on attachment 183881 Patch for discussion. Clearing flags on attachment: 183881 Committed r140455: <http://trac.webkit.org/changeset/140455> 813349 12 webkit.review.bot 2013-01-22 13:21:37 -0800 All reviewed patches have been landed. Closing bug. 183881 2013-01-21 20:55:52 -0800 2013-01-22 13:21:34 -0800 Patch for discussion. bug-107498-20130121205247.patch text/plain 1669 jschuh SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE0MDM4MCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE4IEBACisyMDEzLTAxLTIxICBKdXN0aW4g U2NodWggIDxqc2NodWhAY2hyb21pdW0ub3JnPgorCisgICAgICAgIFRSQU5TRk9STUFUSU9OX01B VFJJWF9VU0VfWDg2XzY0X1NTRTIgYnJva2VuIGZvciA2NC1iaXQgV2luZG93cyBidWlsZHMKKyAg ICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEwNzQ5OAorCisg ICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorICAgICAgICAKKyAgICAgICAgRW5h YmxlIFRSQU5TRk9STUFUSU9OX01BVFJJWF9VU0VfWDg2XzY0X1NTRTIgZm9yIDY0LWJpdCBXaW5k b3dzLgorCisgICAgICAgIE5vIG5ldyB0ZXN0cy4gQ292ZXJlZCBieSBleGlzdGluZyB0ZXN0cy4K KworICAgICAgICAqIHBsYXRmb3JtL2dyYXBoaWNzL3RyYW5zZm9ybXMvVHJhbnNmb3JtYXRpb25N YXRyaXguaDoKKyAgICAgICAgKFdlYkNvcmUpOgorICAgICAgICAoVHJhbnNmb3JtYXRpb25NYXRy aXgpOgorCiAyMDEzLTAxLTIxICBEaXJrIFNjaHVsemUgIDxkc2NodWx6ZUBhZG9iZS5jb20+CiAK ICAgICAgICAgQWRkIGJ1aWxkIGZsYWcgZm9yIENhbnZhcydzIFBhdGggb2JqZWN0IChkaXNhYmxl ZCBieSBkZWZhdWx0KQpJbmRleDogU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvdHJh bnNmb3Jtcy9UcmFuc2Zvcm1hdGlvbk1hdHJpeC5oCj09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJD b3JlL3BsYXRmb3JtL2dyYXBoaWNzL3RyYW5zZm9ybXMvVHJhbnNmb3JtYXRpb25NYXRyaXguaAko cmV2aXNpb24gMTQwMzQ5KQorKysgU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvdHJh bnNmb3Jtcy9UcmFuc2Zvcm1hdGlvbk1hdHJpeC5oCSh3b3JraW5nIGNvcHkpCkBAIC02OSwxNSAr NjksMjAgQEAgY2xhc3MgTGF5b3V0UmVjdDsKIGNsYXNzIEZsb2F0UmVjdDsKIGNsYXNzIEZsb2F0 UXVhZDsKIAotI2lmIENQVShYODZfNjQpICYmICFQTEFURk9STShXSU5ET1dTKQorI2lmIENQVShY ODZfNjQpCiAjZGVmaW5lIFRSQU5TRk9STUFUSU9OX01BVFJJWF9VU0VfWDg2XzY0X1NTRTIKICNl bmRpZgogCiBjbGFzcyBUcmFuc2Zvcm1hdGlvbk1hdHJpeCB7CiAgICAgV1RGX01BS0VfRkFTVF9B TExPQ0FURUQ7CiBwdWJsaWM6CisKICNpZiBDUFUoQVBQTEVfQVJNVjdTKSB8fCBkZWZpbmVkKFRS QU5TRk9STUFUSU9OX01BVFJJWF9VU0VfWDg2XzY0X1NTRTIpCisjaWYgQ09NUElMRVIoTVNWQykK KyAgICBfX2RlY2xzcGVjKGFsaWduKDE2KSkgdHlwZWRlZiBkb3VibGUgTWF0cml4NFs0XVs0XTsK KyNlbHNlCiAgICAgdHlwZWRlZiBkb3VibGUgTWF0cml4NFs0XVs0XSBfX2F0dHJpYnV0ZV9fKChh bGlnbmVkICgxNikpKTsKKyNlbmRpZgogI2Vsc2UKICAgICB0eXBlZGVmIGRvdWJsZSBNYXRyaXg0 WzRdWzRdOwogI2VuZGlmCg==