Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1551363002:
[Interpreter] StateValuesRequireUpdate handles cases when deoptimization is disabled. (Closed)
Description[Interpreter] StateValuesRequireUpdate handles cases when deoptimization is disabled.
Fixes StateValuesRequireUpdate function to return false if deoptimization is not enabled.
When deoptimization is not enabled there is no need to create nodes for state values.
BUG=v8:4280
LOG=N
Committed: https://crrev.com/08419a0989a5fc74ef43b14390ca0365dea906f4
Cr-Commit-Position: refs/heads/master@{#33116}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Addresses comments from Ross #Patch Set 3 : Fixed a bug in earlier patch. #Patch Set 4 : Fixes the comments correctly this time. Misread the comment earlier. #
Messages
Total messages: 22 (11 generated)
Description was changed from ========== [Interpreter] Fixes DCHECK in bytecode-graph-builder. Fixes DCHECK to ignore the check if environment values are up to date only when deoptimization is enabled. When deoptimization is not enabled no nodes are created for state values and hence the check should not be performed. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Fixes DCHECK in bytecode-graph-builder. Fixes DCHECK to ignore the check for up to date environment values when deoptimization is disabled. When deoptimization is not enabled no nodes are created for state values and hence the check should not be performed. BUG=v8:4280 LOG=N ==========
mythria@chromium.org changed reviewers: + rmcilroy@chromium.org
PTAL, Thanks, Mythri
https://codereview.chromium.org/1551363002/diff/1/src/compiler/bytecode-graph... File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/1551363002/diff/1/src/compiler/bytecode-graph... src/compiler/bytecode-graph-builder.cc:29: DCHECK(!builder_->info()->is_deoptimization_enabled() || Could you move this up into StateValuesRequireUpdate (returning false if deoptimization is disable). https://codereview.chromium.org/1551363002/diff/1/test/mjsunit/mjsunit.status File test/mjsunit/mjsunit.status (left): https://codereview.chromium.org/1551363002/diff/1/test/mjsunit/mjsunit.status... test/mjsunit/mjsunit.status:997: 'regress/regress-crbug-527364': [SKIP], Looks like this still fails for other reasons on the bots.
Description was changed from ========== [Interpreter] Fixes DCHECK in bytecode-graph-builder. Fixes DCHECK to ignore the check for up to date environment values when deoptimization is disabled. When deoptimization is not enabled no nodes are created for state values and hence the check should not be performed. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Fixes StateValuesAreUpToDate in bytecode-graph-builder. Fixes StateValuesAreUpToDate function to return true if deoptimization is not enabled. BUG=v8:4280 LOG=N ==========
Description was changed from ========== [Interpreter] Fixes StateValuesAreUpToDate in bytecode-graph-builder. Fixes StateValuesAreUpToDate function to return true if deoptimization is not enabled. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] StateValuesRequireUpdate handles cases when deoptimization is disabled. Fixes StateValuesRequireUpdate function to return false if deoptimization is not enabled. When deoptimization is not enabled there is no need to create nodes for state values. BUG=v8:4280 LOG=N ==========
Description was changed from ========== [Interpreter] StateValuesRequireUpdate handles cases when deoptimization is disabled. Fixes StateValuesRequireUpdate function to return false if deoptimization is not enabled. When deoptimization is not enabled there is no need to create nodes for state values. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] StateValuesRequireUpdate handles cases when deoptimization is disabled. Fixes StateValuesRequireUpdate function to return false if deoptimization is not enabled. When deoptimization is not enabled there is no need to create nodes for state values. BUG=v8:4280 LOG=N ==========
Could you please look at the changes. Thanks, Mythri https://codereview.chromium.org/1551363002/diff/1/src/compiler/bytecode-graph... File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/1551363002/diff/1/src/compiler/bytecode-graph... src/compiler/bytecode-graph-builder.cc:29: DCHECK(!builder_->info()->is_deoptimization_enabled() || On 2016/01/04 15:40:24, rmcilroy wrote: > Could you move this up into StateValuesRequireUpdate (returning false if > deoptimization is disable). Done.
lgtm
The CQ bit was checked by mythria@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1551363002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1551363002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_presubmit on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/9442)
mythria@chromium.org changed reviewers: + bmeurer@chromium.org
I did a minor fix because DCHECK in ~FrameStateBeforeAndAfter() was failing when deoptimization is not enabled. Could you please take a look. Thanks, Mythri
LGTM.
The CQ bit was checked by mythria@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1551363002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1551363002/60001
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] StateValuesRequireUpdate handles cases when deoptimization is disabled. Fixes StateValuesRequireUpdate function to return false if deoptimization is not enabled. When deoptimization is not enabled there is no need to create nodes for state values. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] StateValuesRequireUpdate handles cases when deoptimization is disabled. Fixes StateValuesRequireUpdate function to return false if deoptimization is not enabled. When deoptimization is not enabled there is no need to create nodes for state values. BUG=v8:4280 LOG=N ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] StateValuesRequireUpdate handles cases when deoptimization is disabled. Fixes StateValuesRequireUpdate function to return false if deoptimization is not enabled. When deoptimization is not enabled there is no need to create nodes for state values. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] StateValuesRequireUpdate handles cases when deoptimization is disabled. Fixes StateValuesRequireUpdate function to return false if deoptimization is not enabled. When deoptimization is not enabled there is no need to create nodes for state values. BUG=v8:4280 LOG=N Committed: https://crrev.com/08419a0989a5fc74ef43b14390ca0365dea906f4 Cr-Commit-Position: refs/heads/master@{#33116} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/08419a0989a5fc74ef43b14390ca0365dea906f4 Cr-Commit-Position: refs/heads/master@{#33116} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
