1356936 - Use __builtin_add/sub/mul_overflow() where available to optimize CheckedInt

Status: RESOLVED FIXED

(Core :: MFBT, enhancement)

Description

[Mats Palmgren (inactive)]

Clang 3.4 and GCC 5 have builtin functions for overflow checks.
I suspect they are faster than our current CheckedInt overflow checks
(but I haven't measured).

https://clang.llvm.org/docs/LanguageExtensions.html#checked-arithmetic-builtins
https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html

Comment 1

[Mats Palmgren (inactive)]

Attachment: Use __builtin_add/sub/mul_overflow() where available to optimize CheckedInt.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=162e2e5d876f4aafb80d8f3db1447d0ad95d3dfa

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 8858691 [details] [diff] [review]
Use __builtin_add/sub/mul_overflow() where available to optimize CheckedInt.

Review of attachment 8858691 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/CheckedInt.h
@@ +15,5 @@
>  #include "mozilla/IntegerTypeTraits.h"
>  
> +#if (defined(__clang__) &&                                             \
> +     ((__clang_major__ > 3) ||                                         \
> +      (__clang_major__ == 3 && __clang_minor__ >= 4)))                 \

This might seem like the right way to do it, but it's not.  Per http://clang.llvm.org/docs/LanguageExtensions.html#builtin-macros these are *marketing* version numbers, and it's not the case that clang version numbers exposed here reflect capabilities in any way.  You should be using feature-testing macros as described at http://clang.llvm.org/docs/LanguageExtensions.html#langext-feature-check and http://clang.llvm.org/docs/LanguageExtensions.html#checked-arithmetic-builtins instead.

@@ +724,5 @@
> +MOZ_CHECKEDINT_BASIC_BINARY_OPERATOR2(Add, +, __builtin_add_overflow)
> +MOZ_CHECKEDINT_BASIC_BINARY_OPERATOR2(Sub, -, __builtin_sub_overflow)
> +MOZ_CHECKEDINT_BASIC_BINARY_OPERATOR2(Mul, *, __builtin_mul_overflow)
> +MOZ_CHECKEDINT_BASIC_BINARY_OPERATOR(Div, /)
> +MOZ_CHECKEDINT_BASIC_BINARY_OPERATOR(Mod, %)

How about moving div/mod outside the #ifdef so they're more obviously shared between both cases?

Comment 3

[(no longer active)]

Mats, are you planning to pick this up again?

Comment 4

[Mats Palmgren (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=50ed82d2071bae2af5485dc4a104ea9953dc1d10

Comment 5

[Mats Palmgren (inactive)]

Attachment: bug.cpp

It seems GCC has a bug in __builtin_mul_overflow() when compiling
with the -m32 flag, as we do for our Linux/Android 32-bit builds.
The result from this test should be:
# g++ -std=c++11 bug.cpp  && ./a.out
SUCCESS: overflow detected
Result is 0

but when compiled with -m32 it fails to detect the overflow:
# g++ -m32 -std=c++11 bug.cpp  && ./a.out
ERROR: failed to detect overflow!
Result is 0

(g++ (Ubuntu 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609)

Comment 6

[Mats Palmgren (inactive)]

Attachment: Use __builtin_add/sub/mul_overflow() where available to optimize CheckedInt.

I excluded 32-bit builds for now since it appears the GCC compiler
we use has not implemented these built-ins correctly. (see above)

Comment 7

[Nathan Froyd [:froydnj]]

Looks like it only fails to detect overflow when not optimizing.  Peculiar.  Can you file a GCC bug?

Comment 8

[Mats Palmgren (inactive)]

Will do.

Comment 9

[Mats Palmgren (inactive)]

Filed GCC bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82274

Comment 10

[(no longer active)]

Comment on attachment 8910473 [details] [diff] [review]
Use __builtin_add/sub/mul_overflow() where available to optimize CheckedInt.

Review of attachment 8910473 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks a lot!

::: mfbt/CheckedInt.h
@@ +18,5 @@
> +// since "gcc -m32" claims to support these but its implementation is buggy.
> +#if defined(HAVE_64BIT_BUILD)
> +#if defined(__has_builtin)
> +#define MOZ_HAS_BUILTIN_OP_OVERFLOW (__has_builtin(__builtin_add_overflow))
> +#elif defined(__GNUC__)

clang defines __GNUC__ but it currently defines it to 4.  It of course does define __has_builtin, so this branch should never be taken by clang, but nevertheless, do you mind adding a comment to that affect here, or a !defined(__clang__), whichever you prefer please?

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/55541ed45819
Use __builtin_add/sub/mul_overflow() where available to optimize CheckedInt.  r=waldo,ehsan

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/55541ed45819