Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2890363002:
Fix deoptmization of inlined TF InstanceOf to call ToBoolean (Closed)
|
Created:
3 years, 7 months ago by danno Modified:
3 years, 6 months ago Reviewers:
Michael Starzinger CC:
v8-reviews_googlegroups.com, Benedikt Meurer Target Ref:
refs/heads/master Project:
v8 Visibility:
Public. |
DescriptionFix deoptmization of inlined TF instanceOf to call ToBoolean
This CL leverages and extends the deopt-to-stub mechanisms previously
introduced to support deopting from CSA-built builtins (e.g. Array.prototype.forEach).
BUG=v8:6373
LOG=N
Review-Url: https://codereview.chromium.org/2890363002
Cr-Commit-Position: refs/heads/master@{#46144}
Committed: https://chromium.googlesource.com/v8/v8/+/e2544f6c03cb6725328036192a64fd80fad0ff1d
Patch Set 1 #Patch Set 2 : Rebased #Patch Set 3 : Add descriptors #Patch Set 4 : Fix nullptr deref #Patch Set 5 : Make it work a gain #Patch Set 6 : Add test flag #
Total comments: 12
Patch Set 7 : Review feedback #
Messages
Total messages: 47 (41 generated)
The CQ bit was checked by danno@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...) v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/22839)
The CQ bit was checked by danno@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...) v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/26191) v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/27874)
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:60001) has been deleted
The CQ bit was checked by danno@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...) v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...) v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...) v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...)
The CQ bit was checked by danno@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)
The CQ bit was checked by danno@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...) v8_linux64_verify_csa_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...)
The CQ bit was checked by danno@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)
The CQ bit was checked by danno@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Fix deoptmization of inlined TF InstanceOf to call ToBoolean BUG=v8:6373 LOG=N ========== to ========== Fix deoptmization of inlined TF InstanceOf to call ToBoolean This CL leverages and extends the deopt-to-stub mechanisms previously introduced to support deopting from CSA-built builtins (e.g. Array.prototype.forEach). BUG=v8:6373 LOG=N ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
danno@chromium.org changed reviewers: + mstarzinger@chromium.org
PTAL
Description was changed from ========== Fix deoptmization of inlined TF InstanceOf to call ToBoolean This CL leverages and extends the deopt-to-stub mechanisms previously introduced to support deopting from CSA-built builtins (e.g. Array.prototype.forEach). BUG=v8:6373 LOG=N ========== to ========== Fix deoptmization of inlined TF instanceOf to call ToBoolean This CL leverages and extends the deopt-to-stub mechanisms previously introduced to support deopting from CSA-built builtins (e.g. Array.prototype.forEach). BUG=v8:6373 LOG=N ==========
Looking good. Just minor comments. https://codereview.chromium.org/2890363002/diff/160001/src/builtins/builtins-... File src/builtins/builtins-conversion-gen.cc (right): https://codereview.chromium.org/2890363002/diff/160001/src/builtins/builtins-... src/builtins/builtins-conversion-gen.cc:260: // Requires parameter on stack to that it can be used as a continuation from a nit: s/to/so/ https://codereview.chromium.org/2890363002/diff/160001/src/compiler/frame-sta... File src/compiler/frame-states.cc (right): https://codereview.chromium.org/2890363002/diff/160001/src/compiler/frame-sta... src/compiler/frame-states.cc:162: js_graph, name, context, &actual_parameters[0], The "&actual_parameters[0]" is not safe for empty vectors. This is crashing on me when I compile this with C++ stdlib assert enabled. With C++11 we can use "actual_parameters.data()" here instead. ~/Development/v8.git/build/linux/debian_jessie_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.8/346: error: attempt to subscript container with out-of-bounds index 0, but container only holds 0 elements. Objects involved in the operation: sequence "this" @ 0x0x7ffd01d08958 { } https://codereview.chromium.org/2890363002/diff/160001/src/compiler/js-native... File src/compiler/js-native-context-specialization.cc (right): https://codereview.chromium.org/2890363002/diff/160001/src/compiler/js-native... src/compiler/js-native-context-specialization.cc:178: Node* outer_frame_state = NodeProperties::GetFrameStateInput(node); nit: s/outer_frame_state/frame_state/ https://codereview.chromium.org/2890363002/diff/160001/src/compiler/js-native... src/compiler/js-native-context-specialization.cc:248: Node* frame_state = CreateStubBuiltinContinuationFrameState( nit: Please add a comment explaining the reasoning behind this frame-state and what it is needed for. https://codereview.chromium.org/2890363002/diff/160001/src/compiler/js-native... src/compiler/js-native-context-specialization.cc:248: Node* frame_state = CreateStubBuiltinContinuationFrameState( nit: s/frame_state/continuation_frame_state/ https://codereview.chromium.org/2890363002/diff/160001/src/interface-descript... File src/interface-descriptors.h (right): https://codereview.chromium.org/2890363002/diff/160001/src/interface-descript... src/interface-descriptors.h:518: void InitializePlatformIndependent( nit: Can we use DECLARE_DESCRIPTOR_WITH_CUSTOM_FUNCTION_TYPE to include this declaration?
The CQ bit was checked by danno@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Feedback addressed, please take another look. https://codereview.chromium.org/2890363002/diff/160001/src/builtins/builtins-... File src/builtins/builtins-conversion-gen.cc (right): https://codereview.chromium.org/2890363002/diff/160001/src/builtins/builtins-... src/builtins/builtins-conversion-gen.cc:260: // Requires parameter on stack to that it can be used as a continuation from a On 2017/06/22 14:16:11, Michael Starzinger wrote: > nit: s/to/so/ Done. https://codereview.chromium.org/2890363002/diff/160001/src/compiler/frame-sta... File src/compiler/frame-states.cc (right): https://codereview.chromium.org/2890363002/diff/160001/src/compiler/frame-sta... src/compiler/frame-states.cc:162: js_graph, name, context, &actual_parameters[0], On 2017/06/22 14:16:11, Michael Starzinger wrote: > The "&actual_parameters[0]" is not safe for empty vectors. This is crashing on > me when I compile this with C++ stdlib assert enabled. With C++11 we can use > "actual_parameters.data()" here instead. > > ~/Development/v8.git/build/linux/debian_jessie_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.8/346: > error: attempt to subscript container with out-of-bounds index 0, but > container only holds 0 elements. > > Objects involved in the operation: > sequence "this" @ 0x0x7ffd01d08958 { > } Done. https://codereview.chromium.org/2890363002/diff/160001/src/compiler/js-native... File src/compiler/js-native-context-specialization.cc (right): https://codereview.chromium.org/2890363002/diff/160001/src/compiler/js-native... src/compiler/js-native-context-specialization.cc:178: Node* outer_frame_state = NodeProperties::GetFrameStateInput(node); On 2017/06/22 14:16:11, Michael Starzinger wrote: > nit: s/outer_frame_state/frame_state/ Done. https://codereview.chromium.org/2890363002/diff/160001/src/compiler/js-native... src/compiler/js-native-context-specialization.cc:248: Node* frame_state = CreateStubBuiltinContinuationFrameState( On 2017/06/22 14:16:11, Michael Starzinger wrote: > nit: s/frame_state/continuation_frame_state/ Done. https://codereview.chromium.org/2890363002/diff/160001/src/compiler/js-native... src/compiler/js-native-context-specialization.cc:248: Node* frame_state = CreateStubBuiltinContinuationFrameState( On 2017/06/22 14:16:11, Michael Starzinger wrote: > nit: s/frame_state/continuation_frame_state/ Done. https://codereview.chromium.org/2890363002/diff/160001/src/interface-descript... File src/interface-descriptors.h (right): https://codereview.chromium.org/2890363002/diff/160001/src/interface-descript... src/interface-descriptors.h:518: void InitializePlatformIndependent( On 2017/06/22 14:16:12, Michael Starzinger wrote: > nit: Can we use DECLARE_DESCRIPTOR_WITH_CUSTOM_FUNCTION_TYPE to include this > declaration? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM. Cool stuff. Thanks!
The CQ bit was checked by danno@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1498146122226790,
"parent_rev": "731d1b73af90f1e5507adfc53a00d4418e509158", "commit_rev":
"e2544f6c03cb6725328036192a64fd80fad0ff1d"}
Message was sent while issue was closed.
Description was changed from ========== Fix deoptmization of inlined TF instanceOf to call ToBoolean This CL leverages and extends the deopt-to-stub mechanisms previously introduced to support deopting from CSA-built builtins (e.g. Array.prototype.forEach). BUG=v8:6373 LOG=N ========== to ========== Fix deoptmization of inlined TF instanceOf to call ToBoolean This CL leverages and extends the deopt-to-stub mechanisms previously introduced to support deopting from CSA-built builtins (e.g. Array.prototype.forEach). BUG=v8:6373 LOG=N Review-Url: https://codereview.chromium.org/2890363002 Cr-Commit-Position: refs/heads/master@{#46144} Committed: https://chromium.googlesource.com/v8/v8/+/e2544f6c03cb6725328036192a64fd80fad... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:180001) as https://chromium.googlesource.com/v8/v8/+/e2544f6c03cb6725328036192a64fd80fad... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
