RESOLVED FIXED 49112
Fix Accelerate.framework issues in VectorMath 32-bit ppc & i386 vs. other architectures (64-bit, ARM, etc.) 
https://bugs.webkit.org/show_bug.cgi?id=49112
Summary Fix Accelerate.framework issues in VectorMath 32-bit ppc & i386 vs. other arc...
Chris Rogers
Reported 2010-11-05 16:35:00 PDT
Fix 32-bit vs. 64-bit Accelerate.framework issues in VectorMath
Attachments
Patch (1.93 KB, patch)
2010-11-05 16:37 PDT, Chris Rogers 		no flags
DetailsFormatted DiffDiff
Patch (2.50 KB, patch)
2010-11-08 12:00 PST, Chris Rogers 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Chris Rogers
Comment 1 2010-11-05 16:37:30 PDT
Created attachment 73139 [details] Patch
Maciej Stachowiak
Comment 2 2010-11-08 00:43:05 PST
Comment on attachment 73139 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=73139&action=review What is the effect of this patch? Does the current code fail to build on 32-bit, or does it build but behave differently when changed this way? Is there a reason we can't use the same code path for both 32-bit and 64-bit platforms? Also of note: ARM is a 32-bit platform not covered by your macros. Is it ok for it to go down the same code path as 64-bit? I think either your macro test is wrong (if ARM should be included with 32-bit)) or the ChangeLog is wrong (if the issue is specific to just i386 and ppc, rather than all 32-bit platforms). > WebCore/platform/audio/VectorMath.cpp:41 > +// In 32-bit mode __ppc__ or __i386__ is defined and <vecLib/vDSP_translate.h> is included which defines macros, so we must handle this case differently. This comment does not seem useful. It says the 32-bit case is handled differently, but does not say why.
Chris Rogers
Comment 3 2010-11-08 10:21:09 PST
Maciej, thanks for having a look. The current code builds on 32-bit but goes into a loop and crashes when vadd() calls back into itself recursively because of a macro which is defined only in the __ppc__ and __i386__ case. The vDSP_translate.h file defines these macros, but only gets included in the 32-bit case. Even though my vadd() and vsmul() functions are in a namespace, the macro screws things up, and it seems unfortunate that Accelerate.h is defining these kinds of macros. The ARM case should be OK, since I'm using exactly the same compile-time conditional as the framework is using.
Chris Rogers
Comment 4 2010-11-08 12:00:30 PST
Created attachment 73261 [details] Patch
Chris Rogers
Comment 5 2010-11-08 12:01:29 PST
Changed name of this bug and uploaded a new patch with a more descriptive comment.
Kenneth Russell
Comment 6 2010-11-11 14:50:03 PST
Comment on attachment 73261 [details] Patch Looks good to me.
WebKit Commit Bot
Comment 7 2010-11-11 18:49:43 PST
Comment on attachment 73261 [details] Patch Clearing flags on attachment: 73261 Committed r71877: <http://trac.webkit.org/changeset/71877>
WebKit Commit Bot
Comment 8 2010-11-11 18:49:49 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.