Merge SaturatedArithmatic.h into base and replace saturated math in ui/gfx/geometry/safe_integer_conversions.h |
|||||||
Issue descriptionSaturated math was added to base here: https://codereview.chromium.org/2354783004 we have awesome implementations of this in blink in SaturatedArithmatic.h that generate smaller and faster code. Given a sample program compiled with lang++ -std=c++11 -O3: int main(int argc, char *argv[]) { return SafeSubtract(static_cast<int>(*argv[0]), static_cast<int>(*argv[1])); } vs int main(int argc, char *argv[]) { return saturatedSubtraction(static_cast<int>(*argv[0]), static_cast<int>(*argv[1])); } I see saturatedSubtraction: 0000000100000f80 pushq %rbp 0000000100000f81 movq %rsp, %rbp 0000000100000f84 movq (%rsi), %rax 0000000100000f87 movq 0x8(%rsi), %rcx 0000000100000f8b movsbl (%rax), %eax 0000000100000f8e movsbl (%rcx), %ecx 0000000100000f91 movl %eax, %edx 0000000100000f93 subl %ecx, %edx 0000000100000f95 xorl %eax, %ecx 0000000100000f97 movl %edx, %esi 0000000100000f99 xorl %eax, %esi 0000000100000f9b shrl $0x1f, %eax 0000000100000f9e addl $0x7fffffff, %eax ## imm = 0x7FFFFFFF 0000000100000fa3 testl %ecx, %esi 0000000100000fa5 cmovnsl %edx, %eax 0000000100000fa8 popq %rbp 0000000100000fa9 retq vs SafeSubtract: 0000000100000f70 pushq %rbp 0000000100000f71 movq %rsp, %rbp 0000000100000f74 movq (%rsi), %rax 0000000100000f77 movq 0x8(%rsi), %rcx 0000000100000f7b movsbl (%rax), %edx 0000000100000f7e movsbl (%rcx), %ecx 0000000100000f81 testl %ecx, %ecx 0000000100000f83 jns 0x100000f94 0000000100000f85 leal 0x7fffffff(%rcx), %esi 0000000100000f8b movl $0x7fffffff, %eax ## imm = 0x7FFFFFFF 0000000100000f90 cmpl %edx, %esi 0000000100000f92 jl 0x100000fae 0000000100000f94 movl %edx, %esi 0000000100000f96 subl %ecx, %esi 0000000100000f98 movl %ecx, %eax 0000000100000f9a xorl $0x80000000, %eax ## imm = 0x80000000 0000000100000f9f cmpl %edx, %eax 0000000100000fa1 movl $0x80000000, %eax ## imm = 0x80000000 0000000100000fa6 cmovlel %esi, %eax 0000000100000fa9 testb %cl, %cl 0000000100000fab cmovlel %esi, %eax 0000000100000fae popq %rbp 0000000100000faf retq The inline assembly we have for ARM in SaturatedArithmaticARM.h should be much better on phones as well: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/asm/SaturatedArithmeticARM.h
,
Oct 26 2016
safe_integer_conversions.h combines saturated math with floor/ceil/rounding where I think you're talking about. I'm not sure that those need to be in base, and I don't see them in SaturatedArithmatic so maybe that's orthogonal here.
,
Oct 26 2016
safe_integer_conversions.h has a bunch of saturated math added in the linked patch, it's nearly half the file.
,
Nov 3 2016
Yes, these do very different things and aren't analogous.
,
Nov 3 2016
FYI, Justin and I talked about this yesterday and agreed that we should, over time, try to move SaturatedArithmatic.h to base as there are some places in chrome that could benefit from it. Just as there are places in Blink that could benefit from safe_integer_conversions.h instead of the clamp_to macros we use today.
,
Nov 4 2016
I think there's some confusion here. The SafeSubtract and SafeAdd methods in safe_integer_conversions.h do the same thing as SaturatedArithmetic.h, but are slower because they contain multiple branches and don't have the inline assembly version for ARM. safe_integer_conversions.h's SafeSubtract should be replaced with WTF's saturatedSubtraction. safe_integer_conversions.h's SafeAdd should be replaced with WTF's saturatedAddition. The ToRounded* methods in the file are unrelated.
,
Nov 4 2016
Sorry, I didn't realize the proposal was that narrow. Yes, SafeSubtract and SafeAdd look like they could be trivially replaced with saturatedSubtraction and saturatedAddition. The "safe" versions definitely have a very non-optimal implementation, whereas the "saturated" ones look about as tight as you can get on ARM and Intel.
,
Nov 4 2016
Why is this WontFix?
,
Nov 4 2016
Because I'm getting old and easily confused when triaging multiple bugs.
,
Nov 12 2016
esprehn: what do you want me to do about this?
** Presubmit ERRORS **
You added one or more #includes that violate checkdeps rules.
third_party/WebKit/Source/core/style/ComputedStyle.cpp
Illegal include: "base/numerics/saturated_arithmetic.h"
Because of "-base" from third_party's include_rules. \
third_party/WebKit/Source/platform/LayoutUnit.h
Illegal include: "base/numerics/saturated_arithmetic.h"
Because of "-base" from third_party's include_rules. \
third_party/WebKit/Source/platform/geometry/IntPoint.h
Illegal include: "base/numerics/saturated_arithmetic.h"
Because of "-base" from third_party's include_rules.
,
Nov 12 2016
Use a proxy header like this: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/CheckedNumeric.h
,
Nov 21 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/42cb2e5d1ee0e5a9e1073307b93df913d2df913f commit 42cb2e5d1ee0e5a9e1073307b93df913d2df913f Author: enne <enne@chromium.org> Date: Mon Nov 21 04:30:56 2016 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 Review-Url: https://codereview.chromium.org/2499783002 Cr-Commit-Position: refs/heads/master@{#433474} [modify] https://crrev.com/42cb2e5d1ee0e5a9e1073307b93df913d2df913f/base/BUILD.gn [add] https://crrev.com/42cb2e5d1ee0e5a9e1073307b93df913d2df913f/base/numerics/saturated_arithmetic.h [rename] https://crrev.com/42cb2e5d1ee0e5a9e1073307b93df913d2df913f/base/numerics/saturated_arithmetic_arm.h [add] https://crrev.com/42cb2e5d1ee0e5a9e1073307b93df913d2df913f/base/numerics/saturated_arithmetic_unittest.cc [modify] https://crrev.com/42cb2e5d1ee0e5a9e1073307b93df913d2df913f/third_party/WebKit/Source/core/style/ComputedStyle.cpp [modify] https://crrev.com/42cb2e5d1ee0e5a9e1073307b93df913d2df913f/third_party/WebKit/Source/platform/LayoutUnit.h [modify] https://crrev.com/42cb2e5d1ee0e5a9e1073307b93df913d2df913f/third_party/WebKit/Source/platform/geometry/IntPoint.h [modify] https://crrev.com/42cb2e5d1ee0e5a9e1073307b93df913d2df913f/third_party/WebKit/Source/wtf/BUILD.gn [modify] https://crrev.com/42cb2e5d1ee0e5a9e1073307b93df913d2df913f/third_party/WebKit/Source/wtf/SaturatedArithmetic.h [delete] https://crrev.com/9806cd6a5d72359007cc569abc843f48a53d94e3/third_party/WebKit/Source/wtf/SaturatedArithmeticTest.cpp [modify] https://crrev.com/42cb2e5d1ee0e5a9e1073307b93df913d2df913f/tools/ubsan/blacklist.txt [modify] https://crrev.com/42cb2e5d1ee0e5a9e1073307b93df913d2df913f/tools/ubsan/security_blacklist.txt [modify] https://crrev.com/42cb2e5d1ee0e5a9e1073307b93df913d2df913f/ui/gfx/geometry/point.h [modify] https://crrev.com/42cb2e5d1ee0e5a9e1073307b93df913d2df913f/ui/gfx/geometry/rect.cc [modify] https://crrev.com/42cb2e5d1ee0e5a9e1073307b93df913d2df913f/ui/gfx/geometry/rect.h [modify] https://crrev.com/42cb2e5d1ee0e5a9e1073307b93df913d2df913f/ui/gfx/geometry/safe_integer_conversions.h [modify] https://crrev.com/42cb2e5d1ee0e5a9e1073307b93df913d2df913f/ui/gfx/geometry/safe_integer_conversions_unittest.cc [modify] https://crrev.com/42cb2e5d1ee0e5a9e1073307b93df913d2df913f/ui/gfx/geometry/size.cc [modify] https://crrev.com/42cb2e5d1ee0e5a9e1073307b93df913d2df913f/ui/gfx/geometry/vector2d.cc
,
Jul 26 2017
I implemented the ClampedNumeric<> templates in base/numerics and switched WTF over to those. |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by danakj@chromium.org
, Oct 26 2016