Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1583783003:
[Interpreter] Adds support for variable/function declarations in lookup slots. (Closed)
Description[Interpreter] Adds support for variable/function declarations in lookup slots.
Adds support for variable and function declarations in lookup slots to the
interpreter.
BUG=v8:4280
LOG=N
Committed: https://crrev.com/dac46ef7171ed3003ecbc526e21f3c7f68c2ef1a
Cr-Commit-Position: refs/heads/master@{#33355}
Patch Set 1 #
Total comments: 20
Patch Set 2 : Addresses comments from Ross. #Patch Set 3 : Rebased the patch and fixed a typo in a comment. #
Total comments: 4
Patch Set 4 : Fixes comments #
Messages
Total messages: 16 (6 generated)
mythria@chromium.org changed reviewers: + rmcilroy@chromium.org
PTAL. Thanks, Mythri https://codereview.chromium.org/1583783003/diff/1/src/interpreter/bytecode-ge... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/1583783003/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:1443: // initializations of const declarations. This is not related to this cl. I should have added this earlier. I can move this to a different cl but that would be just a TODO. I was not sure which is better.
https://codereview.chromium.org/1583783003/diff/1/src/interpreter/bytecode-ge... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/1583783003/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:567: TemporaryRegisterScope temporary_register_scope(builder()); Please don't use TemporaryRegisterScope (it is going away anyway) - you should just need to use execution_result()->NewRegister() since the execution result has it's own register allocation scope for the expression visitor. https://codereview.chromium.org/1583783003/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:569: DCHECK(IsDeclaredVariableMode(mode)); nit - move to the top of the case https://codereview.chromium.org/1583783003/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:573: builder()->LoadLiteral(variable->name()).StoreAccumulatorInRegister(name); nit - newline abovem, and newline between LoadLiteral and StoreAccumulator... https://codereview.chromium.org/1583783003/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:575: builder()->LoadTheHole().StoreAccumulatorInRegister(init_value); ditto https://codereview.chromium.org/1583783003/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:578: ->LoadLiteral(Smi::FromInt(0)) Add comment that this indicates no initial value and why this should not be undefined (like fullcodegen). https://codereview.chromium.org/1583783003/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:584: builder()->CallRuntime(Runtime::kDeclareLookupSlot, name, 3); nit - no need for builder() here https://codereview.chromium.org/1583783003/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:623: builder()->LoadLiteral(variable->name()).StoreAccumulatorInRegister(name); newline https://codereview.chromium.org/1583783003/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:629: builder()->CallRuntime(Runtime::kDeclareLookupSlot, name, 3); only one builder() for these 5 lines https://codereview.chromium.org/1583783003/diff/1/test/cctest/interpreter/tes... File test/cctest/interpreter/test-bytecode-generator.cc (right): https://codereview.chromium.org/1583783003/diff/1/test/cctest/interpreter/tes... test/cctest/interpreter/test-bytecode-generator.cc:6335: BytecodeGeneratorHelper helper; no need for these variables (I thought the compiler would complain about these unused variables). In fact, there is no need for the empty test at all - just add a TODO without an empty test function. https://codereview.chromium.org/1583783003/diff/1/test/cctest/interpreter/tes... File test/cctest/interpreter/test-interpreter.cc (right): https://codereview.chromium.org/1583783003/diff/1/test/cctest/interpreter/tes... test/cctest/interpreter/test-interpreter.cc:3615: " return get_x() + x;\n" These are testing variable declarations (since you are getting the function via the variable get_x or get_eval_x). To test function declaration you will need to call func() itself from the outer function.
Thanks for the review. I fixed them. PTAL, Thanks, Mythri https://codereview.chromium.org/1583783003/diff/1/src/interpreter/bytecode-ge... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/1583783003/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:567: TemporaryRegisterScope temporary_register_scope(builder()); On 2016/01/14 11:21:12, rmcilroy wrote: > Please don't use TemporaryRegisterScope (it is going away anyway) - you should > just need to use execution_result()->NewRegister() since the execution result > has it's own register allocation scope for the expression visitor. Done. https://codereview.chromium.org/1583783003/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:569: DCHECK(IsDeclaredVariableMode(mode)); On 2016/01/14 11:21:12, rmcilroy wrote: > nit - move to the top of the case Done. https://codereview.chromium.org/1583783003/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:573: builder()->LoadLiteral(variable->name()).StoreAccumulatorInRegister(name); On 2016/01/14 11:21:12, rmcilroy wrote: > nit - newline abovem, and newline between LoadLiteral and StoreAccumulator... I am not sure, I understood this correctly. cl format puts LoadLiteral and StoreAccumulator in the same line. https://codereview.chromium.org/1583783003/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:578: ->LoadLiteral(Smi::FromInt(0)) On 2016/01/14 11:21:12, rmcilroy wrote: > Add comment that this indicates no initial value and why this should not be > undefined (like fullcodegen). Done. https://codereview.chromium.org/1583783003/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:584: builder()->CallRuntime(Runtime::kDeclareLookupSlot, name, 3); On 2016/01/14 11:21:12, rmcilroy wrote: > nit - no need for builder() here Done. https://codereview.chromium.org/1583783003/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:623: builder()->LoadLiteral(variable->name()).StoreAccumulatorInRegister(name); On 2016/01/14 11:21:12, rmcilroy wrote: > newline Done. https://codereview.chromium.org/1583783003/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:629: builder()->CallRuntime(Runtime::kDeclareLookupSlot, name, 3); On 2016/01/14 11:21:12, rmcilroy wrote: > only one builder() for these 5 lines Done. https://codereview.chromium.org/1583783003/diff/1/test/cctest/interpreter/tes... File test/cctest/interpreter/test-bytecode-generator.cc (right): https://codereview.chromium.org/1583783003/diff/1/test/cctest/interpreter/tes... test/cctest/interpreter/test-bytecode-generator.cc:6335: BytecodeGeneratorHelper helper; On 2016/01/14 11:21:12, rmcilroy wrote: > no need for these variables (I thought the compiler would complain about these > unused variables). In fact, there is no need for the empty test at all - just > add a TODO without an empty test function. Done. https://codereview.chromium.org/1583783003/diff/1/test/cctest/interpreter/tes... File test/cctest/interpreter/test-interpreter.cc (right): https://codereview.chromium.org/1583783003/diff/1/test/cctest/interpreter/tes... test/cctest/interpreter/test-interpreter.cc:3615: " return get_x() + x;\n" On 2016/01/14 11:21:12, rmcilroy wrote: > These are testing variable declarations (since you are getting the function via > the variable get_x or get_eval_x). To test function declaration you will need to > call func() itself from the outer function. In strict mode, we cannot function declarations eval will be in its local scope. So moved the strict mode test to variable declaration. Done.
LGTM with a request, thanks. https://codereview.chromium.org/1583783003/diff/40001/src/interpreter/bytecod... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/1583783003/diff/40001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:448: RegisterAllocationScope register_scope(this); Instead of adding these, could you instead modify VisitDeclarations such that you replace the line: AstVisitor::VisitDeclarations() with the following: for (int i = 0; i < declarations->length(); i++) { RegisterAllocationScope register_scope(this); Visit(declarations->at(i)); } Thanks. https://codereview.chromium.org/1583783003/diff/40001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:515: RegisterAllocationScope register_scope(this); Also remove this once the above comment is done.
Thanks, https://codereview.chromium.org/1583783003/diff/40001/src/interpreter/bytecod... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/1583783003/diff/40001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:448: RegisterAllocationScope register_scope(this); On 2016/01/15 14:17:20, rmcilroy wrote: > Instead of adding these, could you instead modify VisitDeclarations such that > you replace the line: AstVisitor::VisitDeclarations() with the following: > > for (int i = 0; i < declarations->length(); i++) { > RegisterAllocationScope register_scope(this); > Visit(declarations->at(i)); > } > > Thanks. Done. https://codereview.chromium.org/1583783003/diff/40001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:515: RegisterAllocationScope register_scope(this); On 2016/01/15 14:17:20, rmcilroy wrote: > Also remove this once the above comment is done. Done.
The CQ bit was checked by mythria@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1583783003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1583783003/60001
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 mythria@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rmcilroy@chromium.org Link to the patchset: https://codereview.chromium.org/1583783003/#ps60001 (title: "Fixes comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1583783003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1583783003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Adds support for variable/function declarations in lookup slots. Adds support for variable and function declarations in lookup slots to the interpreter. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Adds support for variable/function declarations in lookup slots. Adds support for variable and function declarations in lookup slots to the interpreter. BUG=v8:4280 LOG=N Committed: https://crrev.com/dac46ef7171ed3003ecbc526e21f3c7f68c2ef1a Cr-Commit-Position: refs/heads/master@{#33355} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/dac46ef7171ed3003ecbc526e21f3c7f68c2ef1a Cr-Commit-Position: refs/heads/master@{#33355} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
