Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2271153003:
[deoptimizer] Fix for non-topmost interpreted frame. (Closed)
|
Created:
4 years, 3 months ago by Michael Starzinger Modified:
4 years, 3 months ago 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[deoptimizer] Fix for non-topmost interpreted frame.
The accumulator is always part of the translation for every interpreted
frame. The assumption is that all frames are in {TOS_REGISTER} state.
This however is not supported for non-topmost frames and we need to
avoid pushing the accumulator onto the machine stack.
R=jarin@chromium.org
Committed: https://crrev.com/9c8f4775bd7ce3063db5b4b50d2bb8323a23c650
Cr-Commit-Position: refs/heads/master@{#38945}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Addressed comments. #
Total comments: 4
Patch Set 3 : Addressed comments. #Messages
Total messages: 24 (16 generated)
The CQ bit was checked by mstarzinger@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...
mstarzinger@chromium.org changed reviewers: + rmcilroy@chromium.org
Jaro: PTAL. Ross: FYI.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2271153003/diff/1/src/deoptimizer.cc File src/deoptimizer.cc (right): https://codereview.chromium.org/2271153003/diff/1/src/deoptimizer.cc#newcode1111 src/deoptimizer.cc:1111: if (!is_topmost) height_in_bytes -= kPointerSize; Should we do this before printing height for the trace_scope_ != NULL case above?
Comments addressed. https://codereview.chromium.org/2271153003/diff/1/src/deoptimizer.cc File src/deoptimizer.cc (right): https://codereview.chromium.org/2271153003/diff/1/src/deoptimizer.cc#newcode1111 src/deoptimizer.cc:1111: if (!is_topmost) height_in_bytes -= kPointerSize; On 2016/08/24 11:18:30, rmcilroy wrote: > Should we do this before printing height for the trace_scope_ != NULL case > above? Done. No strong opinion either way.
The CQ bit was checked by mstarzinger@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: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2271153003/diff/20001/src/deoptimizer.cc File src/deoptimizer.cc (right): https://codereview.chromium.org/2271153003/diff/20001/src/deoptimizer.cc#newc... src/deoptimizer.cc:1267: // builtin will pop it off the topmost frame (possibly after materialization). Would not it be better to put the comment inside the then branch? https://codereview.chromium.org/2271153003/diff/20001/src/deoptimizer.cc#newc... src/deoptimizer.cc:1282: } else { Perhaps you could say here something like // For non-topmost frames, skip the accumulator translation. For those // frames, the return value from the callee will become the accumulator.
Thanks! Addressed comments. Landing. https://codereview.chromium.org/2271153003/diff/20001/src/deoptimizer.cc File src/deoptimizer.cc (right): https://codereview.chromium.org/2271153003/diff/20001/src/deoptimizer.cc#newc... src/deoptimizer.cc:1267: // builtin will pop it off the topmost frame (possibly after materialization). On 2016/08/26 12:31:43, Jarin wrote: > Would not it be better to put the comment inside the then branch? Done. https://codereview.chromium.org/2271153003/diff/20001/src/deoptimizer.cc#newc... src/deoptimizer.cc:1282: } else { On 2016/08/26 12:31:43, Jarin wrote: > Perhaps you could say here something like > > // For non-topmost frames, skip the accumulator translation. For those > // frames, the return value from the callee will become the accumulator. Done.
The CQ bit was checked by mstarzinger@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: This issue passed the CQ dry run.
The CQ bit was checked by mstarzinger@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rmcilroy@chromium.org, jarin@chromium.org Link to the patchset: https://codereview.chromium.org/2271153003/#ps40001 (title: "Addressed comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from
==========
[deoptimizer] Fix for non-topmost interpreted frame.
The accumulator is always part of the translation for every interpreted
frame. The assumption is that all frames are in {TOS_REGISTER} state.
This however is not supported for non-topmost frames and we need to
avoid pushing the accumulator onto the machine stack.
R=jarin@chromium.org
==========
to
==========
[deoptimizer] Fix for non-topmost interpreted frame.
The accumulator is always part of the translation for every interpreted
frame. The assumption is that all frames are in {TOS_REGISTER} state.
This however is not supported for non-topmost frames and we need to
avoid pushing the accumulator onto the machine stack.
R=jarin@chromium.org
Committed: https://crrev.com/9c8f4775bd7ce3063db5b4b50d2bb8323a23c650
Cr-Commit-Position: refs/heads/master@{#38945}
==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/9c8f4775bd7ce3063db5b4b50d2bb8323a23c650 Cr-Commit-Position: refs/heads/master@{#38945} |
