Some inline ARM assembly for saturated arithmetic, a small speed-up for

Some inline ARM assembly for saturated arithmetic and a small speed-up
for the C version of saturated add and subtract. Created a unit test for
setting a saturated value.

Benchmarking using rasterize_and_record_micro.key_mobile_sites on a
Nexus7 recorded a consistent drop in RasterizeAndRecordMicro:record_time
from 5.18ms to 4.93ms (~5% improvement).

BUG=325746
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177208

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177300

[picksi, 2014-06-19 23:06:31]

This is a new CL that replaces the previous CL for saturated arithmetic work.
This new CL includes:

The ARM code moved into a new file inside an Asm folder
A fast saturated set function
A couple of new unit tests for the set function

[Stephen Chennney, 2014-06-20 14:46:56]

I'm happy with the way you're going about this.

Please expand the CL description to include information about the performance
gains you see from this, including which benchmarks and devices.

+eseidel to offer opinion on where to put the asm files in the wtf tree. I'm OK
with the way it is but I don't feel I own wtf. In fact, I don't own WTF so Eric
or someone else will have to l-g-t-m.

In general we've been trying to make sure that .h files are included in gyp
targets so that they are in the file list for IDEs (Xcode and Visual Studio). To
do that for the new asm file you'll have to modify wtf.gyp, add a target for arm
that explicitly includes the file. That's non trivial and I defer to Eric as to
whether or not it's worth the effort.

https://codereview.chromium.org/346913004/diff/20001/Source/wtf/SaturatedArit...
File Source/wtf/SaturatedArithmetic.h (right):

https://codereview.chromium.org/346913004/diff/20001/Source/wtf/SaturatedArit...
Source/wtf/SaturatedArithmetic.h:56: result = std::numeric_limits<int>::max() +
(ua >> 31);
No braces and immediately "return std::num..."

https://codereview.chromium.org/346913004/diff/20001/Source/wtf/SaturatedArit...
Source/wtf/SaturatedArithmetic.h:72: result = std::numeric_limits<int>::max() +
(ua >> 31);
Ditto.

https://codereview.chromium.org/346913004/diff/20001/Source/wtf/SaturatedArit...
Source/wtf/SaturatedArithmetic.h:101: }
No braces on single line statements in Blink.

https://codereview.chromium.org/346913004/diff/20001/Source/wtf/SaturatedArit...
Source/wtf/SaturatedArithmetic.h:105: }
Ditto.

https://codereview.chromium.org/346913004/diff/20001/Source/wtf/SaturatedArit...
Source/wtf/SaturatedArithmetic.h:118: }
Ditto,

https://codereview.chromium.org/346913004/diff/20001/Source/wtf/asm/Saturated...
File Source/wtf/asm/SaturatedArithmeticAsm.h (right):

https://codereview.chromium.org/346913004/diff/20001/Source/wtf/asm/Saturated...
Source/wtf/asm/SaturatedArithmeticAsm.h:2: * Copyright (c) 2014, Google Inc. All
rights reserved.
We're using a much simpler header block now for new files. See
https://sites.google.com/a/chromium.org/dev/blink/coding-style#TOC-License

[eseidel, 2014-06-20 16:30:20]

https://codereview.chromium.org/346913004/diff/20001/Source/wtf/SaturatedArit...
File Source/wtf/SaturatedArithmetic.h (right):

https://codereview.chromium.org/346913004/diff/20001/Source/wtf/SaturatedArit...
Source/wtf/SaturatedArithmetic.h:42: #include "wtf/asm/SaturatedArithmeticAsm.h"
I probably would have called it wtf/SaturatedArithmaticARM.h, or something to
indicate what flavor of ASM it is.

[eseidel, 2014-06-20 16:31:39]

Honestly, you could evne just put the ARM ASM in the original header, I doubt
we're going to have much ASM in blink.  It's historically rarely been worth it.

[picksi, 2014-06-23 15:30:44]

I've left the ARM code in a separate file to keep the header file less
cluttered. I've named it SaturatedArithmeticARM.h as Eric suggested, and added
this file to the gypi.

I am using CPU(ARM) instead of CPU(ARM_TRADITIONAL) as this appears to be more
consistently defined.

The CL description now has some hard performance numbers in it.

Simon

https://codereview.chromium.org/346913004/diff/20001/Source/wtf/SaturatedArit...
File Source/wtf/SaturatedArithmetic.h (right):

https://codereview.chromium.org/346913004/diff/20001/Source/wtf/SaturatedArit...
Source/wtf/SaturatedArithmetic.h:42: #include "wtf/asm/SaturatedArithmeticAsm.h"
On 2014/06/20 16:30:20, eseidel wrote:
> I probably would have called it wtf/SaturatedArithmaticARM.h, or something to
> indicate what flavor of ASM it is.

Done.

https://codereview.chromium.org/346913004/diff/20001/Source/wtf/SaturatedArit...
Source/wtf/SaturatedArithmetic.h:56: result = std::numeric_limits<int>::max() +
(ua >> 31);
On 2014/06/20 14:46:56, Stephen Chenney wrote:
> No braces and immediately "return std::num..."

Done.

https://codereview.chromium.org/346913004/diff/20001/Source/wtf/SaturatedArit...
Source/wtf/SaturatedArithmetic.h:72: result = std::numeric_limits<int>::max() +
(ua >> 31);
On 2014/06/20 14:46:56, Stephen Chenney wrote:
> Ditto.

Done.

https://codereview.chromium.org/346913004/diff/20001/Source/wtf/SaturatedArit...
Source/wtf/SaturatedArithmetic.h:101: }
On 2014/06/20 14:46:56, Stephen Chenney wrote:
> No braces on single line statements in Blink.

Done.

https://codereview.chromium.org/346913004/diff/20001/Source/wtf/SaturatedArit...
Source/wtf/SaturatedArithmetic.h:105: }
On 2014/06/20 14:46:56, Stephen Chenney wrote:
> Ditto.

Done.

https://codereview.chromium.org/346913004/diff/20001/Source/wtf/SaturatedArit...
Source/wtf/SaturatedArithmetic.h:118: }
On 2014/06/20 14:46:56, Stephen Chenney wrote:
> Ditto,

Done.

https://codereview.chromium.org/346913004/diff/20001/Source/wtf/asm/Saturated...
File Source/wtf/asm/SaturatedArithmeticAsm.h (right):

https://codereview.chromium.org/346913004/diff/20001/Source/wtf/asm/Saturated...
Source/wtf/asm/SaturatedArithmeticAsm.h:2: * Copyright (c) 2014, Google Inc. All
rights reserved.
On 2014/06/20 14:46:56, Stephen Chenney wrote:
> We're using a much simpler header block now for new files. See
> https://sites.google.com/a/chromium.org/dev/blink/coding-style#TOC-License

Done.

[picksi, 2014-06-27 08:41:42]

Hi, are you happy with the changes I've made here?

Thanks!

Simon

[Stephen Chennney, 2014-06-27 14:21:54]

LGTM but you'll need Eric or someone with wtf ownership to actually commit.

[Stephen Chennney, 2014-06-27 14:23:49]

I'm not seeing the CL description update. I believe you'll need to "Edit Issue"
this issue to change it.

[picksi, 2014-06-27 14:33:39]

On 2014/06/27 14:23:49, Stephen Chenney wrote:
> I'm not seeing the CL description update. I believe you'll need to "Edit
Issue"
> this issue to change it.

Thanks, I've now updated the description.

[eseidel, 2014-06-27 16:40:15]

lgtm.  I would like eae who I believe originally wrote the saturated arith logic
in WTF to review this if possible.

[eae, 2014-06-27 17:07:01]

Very nice, thanks for doing this! Been on my todo list for ages.

LGTM++

https://codereview.chromium.org/346913004/diff/40001/Source/wtf/SaturatedArit...
File Source/wtf/SaturatedArithmetic.h (right):

https://codereview.chromium.org/346913004/diff/40001/Source/wtf/SaturatedArit...
Source/wtf/SaturatedArithmetic.h:40: // If we're building ARM on GCC we replace
the C++ versions with some
This comment seems entirely redundant.

https://codereview.chromium.org/346913004/diff/40001/Source/wtf/SaturatedArit...
Source/wtf/SaturatedArithmetic.h:76: inline int
getMaxSaturatedSetResultForTesting(int FractionalShift)
fractionalShift

[picksi, 2014-06-30 08:49:44]

The CQ bit was checked by picksi@google.com

[commit-bot: I haz the power, 2014-06-30 08:50:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/picksi@chromium.org/346913004/40001

[commit-bot: I haz the power, 2014-06-30 09:30:15]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/13538)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/14538)

[commit-bot: I haz the power, 2014-06-30 09:39:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-30 09:39:10]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)

[picksi, 2014-06-30 11:34:02]

I have fixed a LayoutUnit unit test that was failing for non-ARM build. I'll go
ahead and land the patch now.

[picksi, 2014-06-30 11:34:09]

The CQ bit was checked by picksi@google.com

[commit-bot: I haz the power, 2014-06-30 11:34:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/picksi@chromium.org/346913004/60001

[commit-bot: I haz the power, 2014-06-30 12:41:13]

Change committed as 177208

[Timothy Loh, 2014-06-30 13:57:12]

A revert of this CL has been created in
https://codereview.chromium.org/337323004/ by timloh@chromium.org.

The reason for reverting is: Looks like this isn't compiling on
android_clang_dbg (needed for the roll into chromium); reverting for now.

http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui....

[picksi, 2014-07-01 10:36:54]

Clang build fixed (signed/unsigned comparison in unit test)

[picksi, 2014-07-01 10:49:54]

The CQ bit was checked by picksi@google.com

[commit-bot: I haz the power, 2014-07-01 10:50:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/picksi@chromium.org/346913004/100001

[commit-bot: I haz the power, 2014-07-01 11:54:41]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/14825)

[commit-bot: I haz the power, 2014-07-01 12:27:39]

Change committed as 177300