Nice. Just minor stuff...
https://codereview.chromium.org/517323002/diff/20001/src/compiler/ast-graph-builder.h
File src/compiler/ast-graph-builder.h (right):
https://codereview.chromium.org/517323002/diff/20001/src/compiler/ast-graph-builder.h#newcode268
src/compiler/ast-graph-builder.h:268: Node* Checkpoint(BailoutId ast_id,
Node* parent);
I believe we will always initially create framestates without parents,
so we should not add the argument here. Instead, the Checkpoint method
should just stick in the 'undefined' as the last input.
https://codereview.chromium.org/517323002/diff/20001/src/compiler/graph-builder.cc
File src/compiler/graph-builder.cc (right):
https://codereview.chromium.org/517323002/diff/20001/src/compiler/graph-builder.cc#newcode44
src/compiler/graph-builder.cc:44: if (!has_context && !has_framestate &&
!has_control && !has_effect) {
Good catch! Thanks for fixing.
https://codereview.chromium.org/517323002/diff/20001/src/compiler/instruction-selector.cc
File src/compiler/instruction-selector.cc (right):
https://codereview.chromium.org/517323002/diff/20001/src/compiler/instruction-selector.cc#newcode1083
src/compiler/instruction-selector.cc:1083: if (descriptor->parent() !=
NULL) {
Could you recurse first? (So that the outer guys are first in the
array.)
https://codereview.chromium.org/517323002/diff/20001/src/compiler/instruction.h
File src/compiler/instruction.h (right):
https://codereview.chromium.org/517323002/diff/20001/src/compiler/instruction.h#newcode706
src/compiler/instruction.h:706: FrameStateDescriptor* parent = NULL)
Perhaps rename 'parent -> 'outer_state' (or 'outer'), so that it is
clear which way the nesting goes.
https://codereview.chromium.org/517323002/diff/20001/test/compiler-unittests/instruction-selector-unittest.cc
File test/compiler-unittests/instruction-selector-unittest.cc (right):
https://codereview.chromium.org/517323002/diff/20001/test/compiler-unittests/instruction-selector-unittest.cc#newcode561
test/compiler-unittests/instruction-selector-unittest.cc:561:
EXPECT_EQ(43, s.ToInt32(call_instr->InputAt(2)));
Hmm, could we put the parent values first here?
https://codereview.chromium.org/517323002/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
