|
|
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. |
DescriptionMIPS: 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. #
Created: 4 years, 10 months ago
Messages
Total messages: 15 (7 generated)
Description was changed from ========== MIPS: Refine '[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= ========== to ========== MIPS: Refine '[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= ==========
Hi Benedikt, we introduced Lsa/Dlsa macros for MIPSr6 support in the commits listed bellow. Please use them instead of sll(); addu(); or dsll(); daddu() sequences. Also when you have time, please inform me where we could announce the new MIPS instruction to the whole v8 team. Also please use a4 register on MIPS64 instead of MIPS32’s t0 and it was interesting for me also that FmoveHigh() copies and sign-extends the upper half of a double register to the lower 32 bit of the destination register so after FmoveHigh(a4, -0.0) a4 will contain 0xffffffff80000000. MIPS: Add lsa and dlsa r6 instructions. 8d6899c827711203432694e384f0f534146f1a42 [8d6899c] MIPS: Use the Lsa() macro/r6 instruction in existing code. d9af984e701599ae3a3794142847605727befe1f [d9af984] MIPS64: Use the Lsa() and Dlsa() macros/r6 instructions in existing code. +b3517e2b4b2d5d9425a99451ccdfd6cd39c98867 [b3517e2]
On 2016/01/28 19:48:29, balazs.kilvady wrote: > Hi Benedikt, we introduced Lsa/Dlsa macros for MIPSr6 support in the commits > listed bellow. Please use them instead of sll(); addu(); or dsll(); daddu() > sequences. Also when you have time, please inform me where we could announce the > new MIPS instruction to the whole v8 team. > > Also please use a4 register on MIPS64 instead of MIPS32’s t0 and > it was interesting for me also that FmoveHigh() copies and sign-extends the > upper half of a double register to the lower 32 bit of the destination register > so after FmoveHigh(a4, -0.0) a4 will contain 0xffffffff80000000. > > MIPS: Add lsa and dlsa r6 instructions. > 8d6899c827711203432694e384f0f534146f1a42 [8d6899c] > > MIPS: Use the Lsa() macro/r6 instruction in existing code. > d9af984e701599ae3a3794142847605727befe1f [d9af984] > > MIPS64: Use the Lsa() and Dlsa() macros/r6 instructions in existing code. > +b3517e2b4b2d5d9425a99451ccdfd6cd39c98867 [b3517e2] Maybe you can compile a best practices/style guide file for mips and mips64 folders, then one can consult these if in doubt.
Description was changed from ========== MIPS: Refine '[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= ========== to ========== 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= ==========
On 2016/01/28 19:57:29, Benedikt Meurer (Google) wrote: > On 2016/01/28 19:48:29, balazs.kilvady wrote: > > Hi Benedikt, we introduced Lsa/Dlsa macros for MIPSr6 support in the commits > > listed bellow. Please use them instead of sll(); addu(); or dsll(); daddu() > > sequences. Also when you have time, please inform me where we could announce > the > > new MIPS instruction to the whole v8 team. > > > > Also please use a4 register on MIPS64 instead of MIPS32’s t0 and > > it was interesting for me also that FmoveHigh() copies and sign-extends the > > upper half of a double register to the lower 32 bit of the destination > register > > so after FmoveHigh(a4, -0.0) a4 will contain 0xffffffff80000000. > > > > MIPS: Add lsa and dlsa r6 instructions. > > 8d6899c827711203432694e384f0f534146f1a42 [8d6899c] > > > > MIPS: Use the Lsa() macro/r6 instruction in existing code. > > d9af984e701599ae3a3794142847605727befe1f [d9af984] > > > > MIPS64: Use the Lsa() and Dlsa() macros/r6 instructions in existing code. > > +b3517e2b4b2d5d9425a99451ccdfd6cd39c98867 [b3517e2] > > Maybe you can compile a best practices/style guide file for mips and mips64 > folders, then one can consult these if in doubt. Thanks for the info.
LGTM with comment. Thanks for the refinement. https://codereview.chromium.org/1643973002/diff/20001/src/mips64/builtins-mip... File src/mips64/builtins-mips64.cc (right): https://codereview.chromium.org/1643973002/diff/20001/src/mips64/builtins-mip... src/mips64/builtins-mips64.cc:222: __ FmoveHigh(a4, reg); In this case it is sufficient to know that the value of reg interpreted as int64 is negative. So you could also load the full value into a4 and then check whether a4 is negative I guess?
Thanks for the review. https://codereview.chromium.org/1643973002/diff/20001/src/mips64/builtins-mip... File src/mips64/builtins-mips64.cc (right): https://codereview.chromium.org/1643973002/diff/20001/src/mips64/builtins-mip... src/mips64/builtins-mips64.cc:222: __ FmoveHigh(a4, reg); On 2016/01/29 05:14:22, Benedikt Meurer wrote: > In this case it is sufficient to know that the value of reg interpreted as int64 > is negative. So you could also load the full value into a4 and then check > whether a4 is negative I guess? I have tryed that solution and that was fine for min-max tests but other test failed, for example: === mjsunit/constant-folding-2 === /home/balazs_kilvady/Work/v8/test/mjsunit/mjsunit.js:210: Failure: expected <"-Infinity"> found <"Infinity">
The CQ bit was checked by balazs.kilvady@imgtec.com
The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org Link to the patchset: https://codereview.chromium.org/1643973002/#ps40001 (title: "Fix comments.")
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
Message was sent while issue was closed.
Description was changed from ========== 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= ========== to ========== 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= ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== 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= ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/b35a7aaf26dde9a235767a26a3543d9725d508f3 Cr-Commit-Position: refs/heads/master@{#33611} |