The TurboFan part is looking good. Mostly nits.
https://codereview.chromium.org/1178363002/diff/1/src/compiler/ast-graph-builder.cc
File src/compiler/ast-graph-builder.cc (right):
https://codereview.chromium.org/1178363002/diff/1/src/compiler/ast-graph-builder.cc#newcode1645
src/compiler/ast-graph-builder.cc:1645: ResolveFeedbackSlot(slot),
BailoutId::None(),
nit: Let's put this into a local variable for consistency with the rest
of the code.
https://codereview.chromium.org/1178363002/diff/1/src/compiler/ast-graph-builder.cc#newcode1914
src/compiler/ast-graph-builder.cc:1914: DCHECK(!FLAG_vector_stores ||
store_slot_index == expr->slot_count());
nit: Please move this to before the call to ProdcueValue(), every
expression visitor has a call to ProduceValue() right at the bottom,
otherwise the contract is broken. I would like to visually keep it that
way.
https://codereview.chromium.org/1178363002/diff/1/src/compiler/ast-graph-builder.cc#newcode1951
src/compiler/ast-graph-builder.cc:1951: // TODO(mvstanton): this should
be a runtime call. We can't use an
Let's discuss this TODO in person, I would like to understand this
further.
https://codereview.chromium.org/1178363002/diff/1/src/compiler/ast-graph-builder.cc#newcode2693
src/compiler/ast-graph-builder.cc:2693:
ResolveFeedbackSlot(expr->CountSlot()),
nit: Let's put this into a local variable for consistency with the rest
of the code.
https://codereview.chromium.org/1178363002/diff/1/src/compiler/ast-graph-builder.cc#newcode2702
src/compiler/ast-graph-builder.cc:2702:
ResolveFeedbackSlot(expr->CountSlot()),
nit: Likewise.
https://codereview.chromium.org/1178363002/diff/1/src/compiler/ast-graph-builder.cc#newcode2714
src/compiler/ast-graph-builder.cc:2714:
ResolveFeedbackSlot(expr->CountSlot()),
nit: Likewise.
https://codereview.chromium.org/1178363002/diff/1/src/compiler/ast-graph-builder.cc#newcode3145
src/compiler/ast-graph-builder.cc:3145: DCHECK(!FLAG_vector_stores ||
arguments->location() != Variable::UNALLOCATED);
nit: This DCHECK is subsumed by the one before, please drop it.
https://codereview.chromium.org/1178363002/diff/1/src/compiler/ast-graph-builder.cc#newcode3166
src/compiler/ast-graph-builder.cc:3166: DCHECK(!FLAG_vector_stores ||
rest->location() != Variable::UNALLOCATED);
nit: This DCHECK is subsumed by the one before, please drop it.
https://codereview.chromium.org/1178363002/diff/1/src/compiler/ast-graph-builder.cc#newcode3181
src/compiler/ast-graph-builder.cc:3181: this_function_var->location() !=
Variable::UNALLOCATED);
nit: Let's drop this DCHECK.
https://codereview.chromium.org/1178363002/
--
--
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.
