Move SaturatedArithmetic from Blink to base

Move SaturatedArithmetic from Blink to base

This is a more efficient implementation than what gfx is doing in most
cases.  This CL moves it to base, updates coding style, and switches
gfx to using it.

TBR=thakis@chromium.org
BUG=659381
Committed: https://crrev.com/42cb2e5d1ee0e5a9e1073307b93df913d2df913f
Cr-Commit-Position: refs/heads/master@{#433474}

[enne (OOO), 2016-11-12 00:03:59]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-12 00:04:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-12 00:15:44]

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

[commit-bot: I haz the power, 2016-11-12 00:15:45]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[danakj, 2016-11-12 02:03:19]

danakj@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2016-11-12 02:03:19]

LGTM

https://codereview.chromium.org/2499783002/diff/1/base/numerics/saturated_ari...
File base/numerics/saturated_arithmetic.h (right):

https://codereview.chromium.org/2499783002/diff/1/base/numerics/saturated_ari...
base/numerics/saturated_arithmetic.h:67: template <int FractionalShift>
nit: should be kFractionalShift? or fractional_shift? it's a varname

[enne (OOO), 2016-11-14 21:03:47]

enne@chromium.org changed reviewers:
+ thakis@chromium.org

[enne (OOO), 2016-11-14 21:06:05]

enne@chromium.org changed reviewers:
+ esprehn@chromium.org

[enne (OOO), 2016-11-14 21:06:14]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[enne (OOO), 2016-11-14 21:06:35]

+esprehn for blink wtf
+thakis for tools/ubsan rename

[commit-bot: I haz the power, 2016-11-14 21:12:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-14 21:55:04]

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

[commit-bot: I haz the power, 2016-11-14 21:55:05]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[esprehn, 2016-11-14 22:26:06]

lgtm, I think we might want to rename kIntMaxForLayoutUnit?

https://codereview.chromium.org/2499783002/diff/40001/base/numerics/saturated...
File base/numerics/saturated_arithmetic.h (right):

https://codereview.chromium.org/2499783002/diff/40001/base/numerics/saturated...
base/numerics/saturated_arithmetic.h:71: const int kIntMaxForLayoutUnit =
Should probably give these better names not specific to layout unit?

[enne (OOO), 2016-11-15 19:22:24]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-15 19:23:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-15 19:34:33]

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

[commit-bot: I haz the power, 2016-11-15 19:34:34]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[enne (OOO), 2016-11-15 22:09:52]

Amazingly, this code used to compile in Blink only because the CPU(ARM) path is
never taken in these builds and so the ARM specific code doesn't appear to be
used.  This is true at least for the android_n5x_swarming_rel config that I was
reproing locally.

If I drop the CPU(ARM) condition so that the #ifdef is taken, then I get the
same "unknown mnemonic qadd" assembly errors that I'm seeing from base.  Oof.

[enne (OOO), 2016-11-15 22:11:19]

On 2016/11/15 at 22:09:52, enne wrote:
> Amazingly, this code used to compile in Blink only because the CPU(ARM) path
is never taken in these builds and so the ARM specific code doesn't appear to be
used.  This is true at least for the android_n5x_swarming_rel config that I was
reproing locally.
> 
> If I drop the CPU(ARM) condition so that the #ifdef is taken, then I get the
same "unknown mnemonic qadd" assembly errors that I'm seeing from base.  Oof.

[enne (OOO), 2016-11-15 22:11:43]

Oops, or maybe I need something more specific.  Blink uses CPU(ARM) and maybe
ARCH_CPU_ARM_FAMILY is not equivalent.

[danakj, 2016-11-15 22:22:27]

On Tue, Nov 15, 2016 at 2:11 PM, enne@chromium.org via
codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote:

> Oops, or maybe I need something more specific. Blink uses CPU(ARM) and
> maybe
> ARCH_CPU_ARM_FAMILY is not equivalent.
>

Ok let's enumate a bit more:

build/ does the following:
- defines ARCH_CPU_ARM_FAMILY if |__ARMEL__|
- defines ARCH_CPU_ARM_FAMILY if |__aarch64__|

blink does the following
- defines WTF_CPU_ARM if |arm| or |__arm__| or |ARM| or |_ARM_|
- defines WTF_CPU_ARM64 if |__aarch64__|

So blink code that does if CPU(ARM) || CPU(ARM64) works... on arm64
devices, the same as outside blink.

But blink code that does if CPU(ARM) does not branch on a local android
build for target_cpu="arm64" but does for target_cpu="arm"

So CPU(ARM) really means ARM32.. and this asm code here was only ever
enabled on arm32 devices. Intentionally? Maybe that's why it's not
compiling/



>
> https://codereview.chromium.org/2499783002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2016-11-15 22:22:27]

On Tue, Nov 15, 2016 at 2:11 PM, enne@chromium.org via
codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote:

> Oops, or maybe I need something more specific. Blink uses CPU(ARM) and
> maybe
> ARCH_CPU_ARM_FAMILY is not equivalent.
>

Ok let's enumate a bit more:

build/ does the following:
- defines ARCH_CPU_ARM_FAMILY if |__ARMEL__|
- defines ARCH_CPU_ARM_FAMILY if |__aarch64__|

blink does the following
- defines WTF_CPU_ARM if |arm| or |__arm__| or |ARM| or |_ARM_|
- defines WTF_CPU_ARM64 if |__aarch64__|

So blink code that does if CPU(ARM) || CPU(ARM64) works... on arm64
devices, the same as outside blink.

But blink code that does if CPU(ARM) does not branch on a local android
build for target_cpu="arm64" but does for target_cpu="arm"

So CPU(ARM) really means ARM32.. and this asm code here was only ever
enabled on arm32 devices. Intentionally? Maybe that's why it's not
compiling/



>
> https://codereview.chromium.org/2499783002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[esprehn, 2016-11-15 22:26:29]

I doubt that was intentional, we should use the qadd version on all arm
platforms.

[danakj, 2016-11-15 22:32:10]

On Tue, Nov 15, 2016 at 2:26 PM, <esprehn@chromium.org> wrote:

> I doubt that was intentional, we should use the qadd version on all arm
> platforms.
>

It doesn't appear to compile on arm64 in blink either, but we're not arm
experts and failing to understand why qadd is claiming to be an unknown
mnemonic. So maybe we open a bug and you might have some arm expertise to
lend to expanding this to arm64 or know people who do?

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[danakj, 2016-11-15 22:32:11]

On Tue, Nov 15, 2016 at 2:26 PM, <esprehn@chromium.org> wrote:

> I doubt that was intentional, we should use the qadd version on all arm
> platforms.
>

It doesn't appear to compile on arm64 in blink either, but we're not arm
experts and failing to understand why qadd is claiming to be an unknown
mnemonic. So maybe we open a bug and you might have some arm expertise to
lend to expanding this to arm64 or know people who do?

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[esprehn, 2016-11-15 22:55:31]

On 2016/11/15 at 22:32:11, danakj wrote:
> On Tue, Nov 15, 2016 at 2:26 PM, <esprehn@chromium.org> wrote:
> 
> > I doubt that was intentional, we should use the qadd version on all arm
> > platforms.
> >
> 
> It doesn't appear to compile on arm64 in blink either, but we're not arm
> experts and failing to understand why qadd is claiming to be an unknown
> mnemonic. So maybe we open a bug and you might have some arm expertise to
> lend to expanding this to arm64 or know people who do?
> 

That seems fine. I'm not sure what platforms we ship arm64 on anyway? We ship 32
bit for Nexus 5X, Pixel, and Chromebooks.

[enne (OOO), 2016-11-16 23:59:41]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 00:00:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[enne (OOO), 2016-11-17 00:01:23]

The CQ bit was unchecked by enne@chromium.org

[enne (OOO), 2016-11-17 00:01:38]

Description was changed from

==========
Move SaturatedArithmetic from Blink to base

This is a more efficient implementation than what gfx is doing in most
cases.  This CL moves it to base, updates coding style, and switches
gfx to using it.

BUG=659381
==========

to

==========
Move SaturatedArithmetic from Blink to base

This is a more efficient implementation than what gfx is doing in most
cases.  This CL moves it to base, updates coding style, and switches
gfx to using it.

TBR=thakis@chromium.org
BUG=659381
==========

[enne (OOO), 2016-11-17 00:02:00]

Ok, got this to compile finally.  TBRing thakis for ubsan rename.

[enne (OOO), 2016-11-17 00:02:08]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2016-11-17 00:02:09]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/2499783002/#ps180001
(title: "Now without nacl")

[commit-bot: I haz the power, 2016-11-17 00:04:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2016-11-17 00:05:26]

On Tue, Nov 15, 2016 at 2:55 PM, <esprehn@chromium.org> wrote:

> On 2016/11/15 at 22:32:11, danakj wrote:
> > On Tue, Nov 15, 2016 at 2:26 PM, <esprehn@chromium.org&gt; wrote:
> >
> > > I doubt that was intentional, we should use the qadd version on all arm
> > > platforms.
> > >
> >
> > It doesn't appear to compile on arm64 in blink either, but we're not arm
> > experts and failing to understand why qadd is claiming to be an unknown
> > mnemonic. So maybe we open a bug and you might have some arm expertise to
> > lend to expanding this to arm64 or know people who do?
> >
>
> That seems fine. I'm not sure what platforms we ship arm64 on anyway? We
> ship 32
> bit for Nexus 5X, Pixel, and Chromebooks.
>

FWIW The Nexus 5x android bots build with arm64.
https://bugs.chromium.org/p/chromium/issues/detail?id=662561#c8


>
> https://codereview.chromium.org/2499783002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2016-11-17 00:05:27]

On Tue, Nov 15, 2016 at 2:55 PM, <esprehn@chromium.org> wrote:

> On 2016/11/15 at 22:32:11, danakj wrote:
> > On Tue, Nov 15, 2016 at 2:26 PM, <esprehn@chromium.org&gt; wrote:
> >
> > > I doubt that was intentional, we should use the qadd version on all arm
> > > platforms.
> > >
> >
> > It doesn't appear to compile on arm64 in blink either, but we're not arm
> > experts and failing to understand why qadd is claiming to be an unknown
> > mnemonic. So maybe we open a bug and you might have some arm expertise to
> > lend to expanding this to arm64 or know people who do?
> >
>
> That seems fine. I'm not sure what platforms we ship arm64 on anyway? We
> ship 32
> bit for Nexus 5X, Pixel, and Chromebooks.
>

FWIW The Nexus 5x android bots build with arm64.
https://bugs.chromium.org/p/chromium/issues/detail?id=662561#c8


>
> https://codereview.chromium.org/2499783002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2016-11-17 01:35:25]

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

[commit-bot: I haz the power, 2016-11-17 01:35:26]

Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[enne (OOO), 2016-11-17 18:58:50]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2016-11-17 18:58:51]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/2499783002/#ps200001
(title: "Revert flag addition")

[commit-bot: I haz the power, 2016-11-17 18:59:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 21:18:57]

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

[commit-bot: I haz the power, 2016-11-17 21:18:59]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[enne (OOO), 2016-11-21 02:10:10]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2016-11-21 02:10:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-21 04:30:59]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1479694210607080,
"parent_rev": ["2f7e7a972395e6d7585d21d38a6cd7a57259a439", null], "commit_rev":
["a4ee8fbc712bf6e964be51bcb2a7caabda3aa9a0", null]}

[commit-bot: I haz the power, 2016-11-21 04:31:28]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-11-21 04:36:33]

Description was changed from

==========
Move SaturatedArithmetic from Blink to base

This is a more efficient implementation than what gfx is doing in most
cases.  This CL moves it to base, updates coding style, and switches
gfx to using it.

TBR=thakis@chromium.org
BUG=659381
==========

to

==========
Move SaturatedArithmetic from Blink to base

This is a more efficient implementation than what gfx is doing in most
cases.  This CL moves it to base, updates coding style, and switches
gfx to using it.

TBR=thakis@chromium.org
BUG=659381

Committed: https://crrev.com/42cb2e5d1ee0e5a9e1073307b93df913d2df913f
Cr-Commit-Position: refs/heads/master@{#433474}
==========

[commit-bot: I haz the power, 2016-11-21 04:36:36]

Patchset 11 (id:??) landed as
https://crrev.com/42cb2e5d1ee0e5a9e1073307b93df913d2df913f
Cr-Commit-Position: refs/heads/master@{#433474}