Bug 101084 – Speed up TransformationMatrix::multiply() on modern ARM

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Benjamin Poulain]

Matrix multiplication can be done more efficiently with modern CPUs.

[Benjamin Poulain]

Created attachment 172112 [details]
Patch

[WebKit Review Bot]

Attachment 172112 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Pl..." exit_code: 1
Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:977:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1105:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1106:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1107:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1110:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1111:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1112:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 7 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Peter Beverloo (cr-android ews)]

Comment on attachment 172112 [details]
Patch

Attachment 172112 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14703011

[Benjamin Poulain]

The Android errors just look weird. I think I'll just #ifdef PLATFORM(IOS) unless someone see something wrong.

[Dominik Röttsches (drott)]

Would have been nice if this would have been done in combination with bug 52490 instead of so many #ifdef's.

[Benjamin Poulain]

(In reply to comment #5)
> Would have been nice if this would have been done in combination with bug 52490 instead of so many #ifdef's.

This is unrelated. I think you are mixing port and architecture.

[Gavin Barraclough]

Comment on attachment 172112 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172112&action=review

R+ assuming that for loop is closed somewhere!

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:978
> +        "mov        r3, %[leftMatrix]\n\t"

rather than manually allocating r3, maybe it would be nicer to have a leftMatrixIn, leftMatrixOut, let the compiler pick.

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1108
> +    for (unsigned i = 0; i < 4; ++i) {

I don't see this for loop being closed?

[Benjamin Poulain]

> > Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:978
> > +        "mov        r3, %[leftMatrix]\n\t"
> 
> rather than manually allocating r3, maybe it would be nicer to have a leftMatrixIn, leftMatrixOut, let the compiler pick.

I actually did that initially, but the compiler just figured that was the same variable and never generated the mov :(

> > Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1108
> > +    for (unsigned i = 0; i < 4; ++i) {
> 
> I don't see this for loop being closed?

Good catch! Looks like a copy-paste mistake.

[Benjamin Poulain]

Created attachment 172472 [details]
Patch

[WebKit Review Bot]

Attachment 172472 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Pl..." exit_code: 1
Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:977:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:977:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1105:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1106:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1107:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1110:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1111:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1112:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 8 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Peter Beverloo (cr-android ews)]

Comment on attachment 172472 [details]
Patch

Attachment 172472 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14665450

[Benjamin Poulain]

Ok, Android is limited to d0->d15, that makes sense on some CPUs. I'll #ifdef for iOS for now, then the code can be enabled CPU by CPU.

[Zoltan Herczeg]

We worked on the ARM-NEON support for transformation matrix some time ago, but the community said this is not a bottleneck. That time we didn't find any good benchmark for it. Do you have any performance progression results? Perhaps we should continue the work on the NEON support as well.

[Benjamin Poulain]

Committed r133653: <http://trac.webkit.org/changeset/133653>

[Benjamin Poulain]

(In reply to comment #13)
> We worked on the ARM-NEON support for transformation matrix some time ago, but the community said this is not a bottleneck. That time we didn't find any good benchmark for it. Do you have any performance progression results? Perhaps we should continue the work on the NEON support as well.

I am not completely sure what you are referring to, but if it is https://bugs.webkit.org/show_bug.cgi?id=98913, I am also strongly against it. It is a really bad idea.

Also check the other related patches that have landed recently. Other parts have improved, making TransformationMatrix more important than before.

[Dominik Röttsches (drott)]

(In reply to comment #6)
> (In reply to comment #5)
> > Would have been nice if this would have been done in combination with bug 52490 instead of so many #ifdef's.
> 
> This is unrelated. I think you are mixing port and architecture.

I don't think it's unrelated. I think the point of bug 52490 is to have different implementations for TransformationMatrix, either separated by port or by architecture or whatever distinction needed, instead of ending up with big #ifdef switches in TransformationMatrix.cpp.