Chromium Code Reviews

Issue 1643973002: MIPS: Refine '[builtins] Make Math.max and Math.min fast by default.' (Closed)

Created:
4 years, 10 months ago by balazs.kilvady
Modified:
4 years, 10 months ago
Reviewers:
Benedikt Meurer, ivica.bogosavljevic, gergely.kis.imgtec, akos.palfi.imgtec
CC:
v8-reviews_googlegroups.com
Base URL:
https://chromium.googlesource.com/v8/v8.git@master
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

MIPS: Fix '[builtins] Make Math.max and Math.min fast by default.' Port cb9b801069c432e745f0651cf00d65ef88cd7f06 Original commit message: The previous versions of Math.max and Math.min made it difficult to optimize those (that's why we already have custom code in Crankshaft), and due to lack of ideas what to do about the variable number of arguments, we will probably need to stick in special code in TurboFan as well; so inlining those builtins is off the table, hence there's no real advantage in having them around as "not quite JS" with extra work necessary in the optimizing compilers to still make those builtins somewhat fast in cases where we cannot inline them (also there's a tricky deopt loop in Crankshaft related to Math.min and Math.max, but that will be dealt with later). So to sum up: Instead of trying to make Math.max and Math.min semi-fast in the optimizing compilers with weird work-arounds support %_Arguments %_ArgumentsLength, we do provide the optimal code as native builtins instead and call it a day (which gives a nice performance boost on some benchmarks). BUG= Committed: https://crrev.com/b35a7aaf26dde9a235767a26a3543d9725d508f3 Cr-Commit-Position: refs/heads/master@{#33611}

Patch Set 1 #

Patch Set 2 : Fix 64-bit port. #

Total comments: 2

Patch Set 3 : Fix comments. #

Unified diffs Side-by-side diffs Stats (+16 lines, -18 lines)
M src/mips/builtins-mips.cc View 3 chunks +4 lines, -6 lines 0 comments
M src/mips64/builtins-mips64.cc View 6 chunks +12 lines, -12 lines 0 comments

Messages

Total messages: 15 (7 generated)
balazs.kilvady
Hi Benedikt, we introduced Lsa/Dlsa macros for MIPSr6 support in the commits listed bellow. Please ...
4 years, 10 months ago (2016-01-28 19:48:29 UTC) #3
Benedikt Meurer (Google)
On 2016/01/28 19:48:29, balazs.kilvady wrote: > Hi Benedikt, we introduced Lsa/Dlsa macros for MIPSr6 support ...
4 years, 10 months ago (2016-01-28 19:57:29 UTC) #4
balazs.kilvady
On 2016/01/28 19:57:29, Benedikt Meurer (Google) wrote: > On 2016/01/28 19:48:29, balazs.kilvady wrote: > > ...
4 years, 10 months ago (2016-01-28 20:02:27 UTC) #6
Benedikt Meurer
LGTM with comment. Thanks for the refinement. https://codereview.chromium.org/1643973002/diff/20001/src/mips64/builtins-mips64.cc File src/mips64/builtins-mips64.cc (right): https://codereview.chromium.org/1643973002/diff/20001/src/mips64/builtins-mips64.cc#newcode222 src/mips64/builtins-mips64.cc:222: __ FmoveHigh(a4, ...
4 years, 10 months ago (2016-01-29 05:14:22 UTC) #7
balazs.kilvady
Thanks for the review. https://codereview.chromium.org/1643973002/diff/20001/src/mips64/builtins-mips64.cc File src/mips64/builtins-mips64.cc (right): https://codereview.chromium.org/1643973002/diff/20001/src/mips64/builtins-mips64.cc#newcode222 src/mips64/builtins-mips64.cc:222: __ FmoveHigh(a4, reg); On 2016/01/29 ...
4 years, 10 months ago (2016-01-29 11:09:21 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1643973002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1643973002/40001
4 years, 10 months ago (2016-01-29 11:16:34 UTC) #11
commit-bot: I haz the power
Committed patchset #3 (id:40001)
4 years, 10 months ago (2016-01-29 11:40:30 UTC) #13
commit-bot: I haz the power
4 years, 10 months ago (2016-01-29 11:41:00 UTC) #15
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/b35a7aaf26dde9a235767a26a3543d9725d508f3
Cr-Commit-Position: refs/heads/master@{#33611}

Powered by Google App Engine