Bug 16925

Summary:

How to fix VectorBuffer's alignment FIXME

Product:

WebKit

Reporter:

Marc-Antoine Ruel <maruel>

Component:

Web Template Framework

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, ddkilzer, ggaren, mjs

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch which resolves this problem.	darin: review-
Revised patch	darin: review+

Marc-Antoine Ruel

Reported 2008-01-18 13:18:35 PST

To remove the FIXME for VectorBuffer<>::m_inlineBuffer at line 381, you should define the alignment, which is a compiler-specific attribute. That would help for Vector<double> and would be needed to use with SSE primitives. For GCC, I think that would be: char __attribute__((aligned(__alignof__(T)))) m_inlineBuffer[m_inlineBufferSize]; For MSVC, I think that would be: char __declspec(align(__alignof(T))) m_inlineBuffer[m_inlineBufferSize]; For sure, a macro would need to be defined to not directly clutter the code. Since I don't know where to put the necessary macros, I'll let more experimented folks figure that out.

Attachments
Patch which resolves this problem. (3.27 KB, patch) 2008-09-16 02:02 PDT, Paul Pedriana	darin: review-	Details Formatted Diff Diff
Revised patch (2.89 KB, patch) 2008-09-16 14:18 PDT, Paul Pedriana	darin: review+	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Alexey Proskuryakov

Comment 1 2008-07-08 08:43:44 PDT

See also: bug 19775.

Maciej Stachowiak

Comment 2 2008-09-07 04:56:19 PDT

I think more portable than this approach would be to union the char array with a one-element array of type T.

Maciej Stachowiak

Comment 3 2008-09-07 04:58:26 PDT

Oliver reminds me that this doesn't work if T has a constructor. alignof it is then.

Paul Pedriana

Comment 4 2008-09-16 02:02:40 PDT

Created attachment 23467 [details] Patch which resolves this problem. This was recently discussed on the webkit-dev mailing list at a thread starting here: https://lists.webkit.org/pipermail/webkit-dev/2008-September/004777.html

Darin Adler

Comment 5 2008-09-16 09:51:16 PDT

Comment on attachment 23467 [details] Patch which resolves this problem. Looks good! Thanks for tackling this. +#define WTF_ALIGN_OF(type) __alignof__(type) +#define WTF_ALIGNED(variable_type, variable, n) variable_type variable __attribute__((aligned(n))) These macros are fine, but Platform.h is the wrong header for them. It's a configuration header only and not the place to put macros. We could add a new header like <wtf/AlwaysInline.h> or find some other header to put them in. For now, it would be fine for them to be in Vector.h with the intention of moving them elsewhere later. + typedef char aligned_buffer_char; This is the wrong naming style for WebKit code. The type should be named something like AlignedBufferChar. + template <size_t size, size_t alignment> + struct aligned_buffer { aligned_buffer_char buffer[size]; }; Same comment here. We should fix the "wrong header" and naming issues, and then someone should test to be sure this doesn't slow down performance, and then we can land this.

David Kilzer (:ddkilzer)

Comment 6 2008-09-16 10:53:36 PDT

(In reply to comment #4) > Created an attachment (id=23467) [edit] > Patch which resolves this problem. A couple additional comments: 1. Please use the COMPILER() macros when checking for compilers (except for version checks). A new macro will need to be added for Metrowerks. 2. I think alignment macros need to be added for the mingw compiler before the patch lands. I think one of the existing ports uses it (Qt for Windows?), and it would break their build if it was not supported.

Paul Pedriana

Comment 7 2008-09-16 14:18:06 PDT

Created attachment 23487 [details] Revised patch This patch: - moves the align macros back to vector.h - renames aligned_buffer_char to AlignedBufferChar - renamed aligned_buffer to aligned_buffer - used #if COMPILER(GCC) instead of #if defined(__GNUC___) - adds COMPILER(MINGW) - removes the reference to __MWERKS__ for now.

Darin Adler

Comment 8 2008-09-21 13:18:05 PDT

Comment on attachment 23487 [details] Revised patch r=me

Darin Adler

Comment 9 2008-09-21 13:18:25 PDT

http://trac.webkit.org/changeset/36740

Note You need to log in before you can comment on or make changes to this bug.