Saturated arithmetic changed to use templates to fix build-issues

Saturated arithmetic changed to use templates to fix build-issues

BUG=325746

[picksi, 2014-07-09 10:12:37]

Unfortunately when building the inline ARM with chromium.gyp_env as:

{ 'GYP_DEFINES': 'OS=android use_goma=1 android_full_debug=1', }

the build fails (thanks to Brian White for spotting this!). It looks like the
pre-processor fails to turn the const ints into literals that the inline asm
requires.

The only way I could get this to compile correctly was to convert the functions
into templates. It's ugly and pushes some implementation details a bit higher
than I would like, but I don't know of any alternative solutions.

[Sami, 2014-07-09 13:59:26]

Could you link to a failing build so we get a better idea of what the bug was?

https://codereview.chromium.org/378253003/diff/1/Source/platform/LayoutUnit.h
File Source/platform/LayoutUnit.h (right):

https://codereview.chromium.org/378253003/diff/1/Source/platform/LayoutUnit.h...
Source/platform/LayoutUnit.h:229: 32-kLayoutUnitFractionalBits,
Please add spaces around the '-'.

https://codereview.chromium.org/378253003/diff/1/Source/platform/LayoutUnit.h...
Source/platform/LayoutUnit.h:241: 31-kLayoutUnitFractionalBits,
Ditto.

https://codereview.chromium.org/378253003/diff/1/Source/wtf/SaturatedArithmet...
File Source/wtf/SaturatedArithmetic.h (right):

https://codereview.chromium.org/378253003/diff/1/Source/wtf/SaturatedArithmet...
Source/wtf/SaturatedArithmetic.h:89: template<size_t saturate, size_t
fractionalShift>
Could you not calculate |saturate| from |fractionalShift| or do both need to be
separate constants that come from the expansion site?

https://codereview.chromium.org/378253003/diff/1/Source/wtf/SaturatedArithmet...
File Source/wtf/SaturatedArithmeticTest.cpp (right):

https://codereview.chromium.org/378253003/diff/1/Source/wtf/SaturatedArithmet...
Source/wtf/SaturatedArithmeticTest.cpp:113: int v0 =
saturatedSetSigned<32-kFractionBits, kFractionBits>(0);
Ditto. Same for the tests below.

[picksi, 2014-07-09 14:27:53]

I've fixed the white space issues.

There is no bad-build example I can link to, as none of the build-bots are
configured 'correctly' to exhibit this behaviour.

It can be fairly easily repro'd locally by setting chromium.gyp_env to:

{ 'GYP_DEFINES': 'OS=android use_goma=1 android_full_debug=1', }"

and then running:

ninja -j200 -k10 -C out/Debug' content_shell_apk

It fails with the delightfully woolly error:

asm operand 2 probably doesn't match constraints

https://codereview.chromium.org/378253003/diff/1/Source/platform/LayoutUnit.h
File Source/platform/LayoutUnit.h (right):

https://codereview.chromium.org/378253003/diff/1/Source/platform/LayoutUnit.h...
Source/platform/LayoutUnit.h:229: 32-kLayoutUnitFractionalBits,
On 2014/07/09 13:59:26, Sami wrote:
> Please add spaces around the '-'.

Done.

https://codereview.chromium.org/378253003/diff/1/Source/platform/LayoutUnit.h...
Source/platform/LayoutUnit.h:241: 31-kLayoutUnitFractionalBits,
On 2014/07/09 13:59:26, Sami wrote:
> Ditto.

Done.

https://codereview.chromium.org/378253003/diff/1/Source/wtf/SaturatedArithmet...
File Source/wtf/SaturatedArithmetic.h (right):

https://codereview.chromium.org/378253003/diff/1/Source/wtf/SaturatedArithmet...
Source/wtf/SaturatedArithmetic.h:89: template<size_t saturate, size_t
fractionalShift>
In the previous version I did exactly this, but with android_full_debug=1 in
chromium.gyp_env it refused to work. I could only get it to compile when both
parameters came into the function as templated-parameters.

The "saturate" parameter is not actually needed by the C++ version and is only
here to create a common interface for the C++ and ARM version, so the calling
code can remain implementation agnostic.

https://codereview.chromium.org/378253003/diff/1/Source/wtf/SaturatedArithmet...
File Source/wtf/SaturatedArithmeticTest.cpp (right):

https://codereview.chromium.org/378253003/diff/1/Source/wtf/SaturatedArithmet...
Source/wtf/SaturatedArithmeticTest.cpp:113: int v0 =
saturatedSetSigned<32-kFractionBits, kFractionBits>(0);
On 2014/07/09 13:59:26, Sami wrote:
> Ditto. Same for the tests below.

Done.

[Sami, 2014-07-09 14:49:26]

On 2014/07/09 14:27:53, picksi wrote:
> It can be fairly easily repro'd locally by setting chromium.gyp_env to:
> 
> { 'GYP_DEFINES': 'OS=android use_goma=1 android_full_debug=1', }"
> 
> and then running:
> 
> ninja -j200 -k10 -C out/Debug' content_shell_apk
> 
> It fails with the delightfully woolly error:
> 
> asm operand 2 probably doesn't match constraints

Maybe we've should be using a more specific constraint here, like 'M'?

"M: Integer in the range 0 to 32"

More here:
https://gcc.gnu.org/onlinedocs/gcc/Machine-Constraints.html#Machine-Constraints

+simonb@, I summon thee to bring us your arcane GCC knowledge.

[picksi, 2014-07-15 09:25:08]

Mostyn has an much easier fix for this, which disables the ASM in a non-opt
build. See here:

https://codereview.chromium.org/396603002/

This looks like a neater way of fixing this.

[eseidel, 2014-07-22 15:40:22]

Looks fine to me.  eae is the right reviewer.

https://codereview.chromium.org/378253003/diff/20001/Source/platform/LayoutUn...
File Source/platform/LayoutUnit.h (right):

https://codereview.chromium.org/378253003/diff/20001/Source/platform/LayoutUn...
Source/platform/LayoutUnit.h:229: 32 - kLayoutUnitFractionalBits,
nit: We don't wrap to 80c in blink.  I'd probably just leave this unwrapped.

[eae, 2014-07-22 15:45:55]

LGTM

[eae, 2014-07-22 15:45:58]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2014-07-22 15:46:54]

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

[commit-bot: I haz the power, 2014-07-22 16:52:41]

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

[commit-bot: I haz the power, 2014-07-22 16:55:34]

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

[commit-bot: I haz the power, 2014-07-22 16:55:34]

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

[bcwhite, 2015-11-13 19:37:39]

This CL never landed.  Should it be deleted?

[eae, 2015-11-13 19:48:03]

Probably

[picksi, 2015-11-16 11:29:17]

This was fixed by https://codereview.chromium.org/396603002/ instead.