Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2538823002:
[Turbofan] Disable JSFrameSpecialization for interpreted frames. (Closed)
Description[Turbofan] Disable JSFrameSpecialization for interpreted frames.
JSFrameSpecialization depends on the layout of the frame and doesn't work
with interpreted frames. Disable it since it is only used for OSR from asmjs code, which shouldn't go through the bytecode graph builder in many cases.
BUG=669517
Committed: https://crrev.com/6d90507a7ca6b3d77fc5dd5a355b382b1fed4286
Cr-Commit-Position: refs/heads/master@{#41387}
Patch Set 1 #Patch Set 2 : Change to disable frame specialization #
Messages
Total messages: 26 (15 generated)
The CQ bit was checked by rmcilroy@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...
rmcilroy@chromium.org changed reviewers: + mstarzinger@chromium.org
Michi: this fixes the immediate issue, but we might want to instead change linkage()->GetOsrValueLocation() instead so that we can use the same index in BytecodeGraphBuilder as AstGraphBuilder and avoid further issues like this, WDYT? Also, I'm still not sure about what happens if try and visit an OSRValue with kOsrAccumulatorRegisterIndex? Presumably we should never see this as an input on a JumpLoop backedge - if so maybe we should DCHECK we don't see it in instruction-selector?
bmeurer@chromium.org changed reviewers: + bmeurer@chromium.org, tebbi@chromium.org
Tobias noticed this issue before (independently while looking into the OSR typer), and I think he had figured the proper indices, so adding him for review.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for the fix, I do think the fix is sound and would address the issue.
However running JSFrameSpecialization together with the BytecodeGraphBuilder has
really meager test coverage and I am really scared about more hidden bugs. How
would you feel about only running frame specialization when we are not compiling
from bytecode? This could be done in {PipelineCompilationJob::PrepareJobImpl}
where we call {MarkAsFrameSpecializing}. Then we could just leave a [D]CHECK in
JSFrameSpecialization that makes sure we never apply it on an InterpretedFrame.
WDYT?
On 2016/11/29 19:16:58, Benedikt Meurer wrote: > Tobias noticed this issue before (independently while looking into the OSR > typer), and I think he had figured the proper indices, so adding him for review. Yes, this CL is a strict improvement compared to my partially wrong guesses.
I'd also vote for not running it.
Description was changed from ========== [Turbofan] Fix JSFrameSpecialization::ReduceOsrValue for interpreted frames. BUG=669517 ========== to ========== [Turbofan] Disable JSFrameSpecialization for interpreted frames. JSFrameSpecialization depends on the layout of the frame and doesn't work with interpreted frames. Disable it since it is only used for OSR from asmjs code, which shouldn't go through the bytecode graph builder in many cases. BUG=669517 ==========
On 2016/11/30 09:36:39, Michael Starzinger wrote:
> Thanks for the fix, I do think the fix is sound and would address the issue.
> However running JSFrameSpecialization together with the BytecodeGraphBuilder
has
> really meager test coverage and I am really scared about more hidden bugs. How
> would you feel about only running frame specialization when we are not
compiling
> from bytecode? This could be done in {PipelineCompilationJob::PrepareJobImpl}
> where we call {MarkAsFrameSpecializing}. Then we could just leave a [D]CHECK
in
> JSFrameSpecialization that makes sure we never apply it on an
InterpretedFrame.
> WDYT?
Sounds good, done. PTAL.
LGTM. Thanks!
lgtm
The CQ bit was checked by rmcilroy@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 rmcilroy@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": 20001, "attempt_start_ts": 1480514498770660,
"parent_rev": "770abeb4dcd042eecc8b9471e20ee331141d0435", "commit_rev":
"f5621952e464a0efee1118dd78f6cd7886ac62ad"}
Message was sent while issue was closed.
Description was changed from ========== [Turbofan] Disable JSFrameSpecialization for interpreted frames. JSFrameSpecialization depends on the layout of the frame and doesn't work with interpreted frames. Disable it since it is only used for OSR from asmjs code, which shouldn't go through the bytecode graph builder in many cases. BUG=669517 ========== to ========== [Turbofan] Disable JSFrameSpecialization for interpreted frames. JSFrameSpecialization depends on the layout of the frame and doesn't work with interpreted frames. Disable it since it is only used for OSR from asmjs code, which shouldn't go through the bytecode graph builder in many cases. BUG=669517 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [Turbofan] Disable JSFrameSpecialization for interpreted frames. JSFrameSpecialization depends on the layout of the frame and doesn't work with interpreted frames. Disable it since it is only used for OSR from asmjs code, which shouldn't go through the bytecode graph builder in many cases. BUG=669517 ========== to ========== [Turbofan] Disable JSFrameSpecialization for interpreted frames. JSFrameSpecialization depends on the layout of the frame and doesn't work with interpreted frames. Disable it since it is only used for OSR from asmjs code, which shouldn't go through the bytecode graph builder in many cases. BUG=669517 Committed: https://crrev.com/6d90507a7ca6b3d77fc5dd5a355b382b1fed4286 Cr-Commit-Position: refs/heads/master@{#41387} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/6d90507a7ca6b3d77fc5dd5a355b382b1fed4286 Cr-Commit-Position: refs/heads/master@{#41387} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
