Bug 16925 - How to fix VectorBuffer's alignment FIXME
Summary: How to fix VectorBuffer's alignment FIXME
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-01-18 13:18 PST by Marc-Antoine Ruel
Modified: 2008-09-21 13:18 PDT (History)
4 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Marc-Antoine Ruel 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.
Comment 1 Alexey Proskuryakov 2008-07-08 08:43:44 PDT
See also: bug 19775.
Comment 2 Maciej Stachowiak 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.
Comment 3 Maciej Stachowiak 2008-09-07 04:58:26 PDT
Oliver reminds me that this doesn't work if T has a constructor. alignof it is then.
Comment 4 Paul Pedriana 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
Comment 5 Darin Adler 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.
Comment 6 David Kilzer (:ddkilzer) 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.
Comment 7 Paul Pedriana 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.
Comment 8 Darin Adler 2008-09-21 13:18:05 PDT
Comment on attachment 23487 [details]
Revised patch

r=me
Comment 9 Darin Adler 2008-09-21 13:18:25 PDT
http://trac.webkit.org/changeset/36740