Hi guys,
Addressed Michael's comments. Best,
--Michael
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(),
On 2015/06/12 07:59:06, Michael Starzinger wrote:
nit: Let's put this into a local variable for consistency with the
rest of the
code.
Done.
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());
On 2015/06/12 07:59:06, Michael Starzinger wrote:
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.
Well said, done.
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
On 2015/06/12 07:59:06, Michael Starzinger wrote:
Let's discuss this TODO in person, I would like to understand this
further.
Yeah, the deal is to use a vector-based IC you need a slot. So if
optimized code wants to go off and use one, the slot needs to be
allocated in the AST. And that would be a major shame if full code
doesn't use it. Benedikt knows something about this line too, he
suggested the runtime call.
Alternatively, we beef up full code to use an IC here, and then I'm
happy in a different way. :p What do you think?
https://codereview.chromium.org/1178363002/diff/1/src/compiler/ast-graph-builder.cc#newcode2693
src/compiler/ast-graph-builder.cc:2693:
ResolveFeedbackSlot(expr->CountSlot()),
On 2015/06/12 07:59:06, Michael Starzinger wrote:
nit: Let's put this into a local variable for consistency with the
rest of the
code.
Done.
https://codereview.chromium.org/1178363002/diff/1/src/compiler/ast-graph-builder.cc#newcode2702
src/compiler/ast-graph-builder.cc:2702:
ResolveFeedbackSlot(expr->CountSlot()),
On 2015/06/12 07:59:06, Michael Starzinger wrote:
nit: Likewise.
Done.
https://codereview.chromium.org/1178363002/diff/1/src/compiler/ast-graph-builder.cc#newcode2714
src/compiler/ast-graph-builder.cc:2714:
ResolveFeedbackSlot(expr->CountSlot()),
On 2015/06/12 07:59:05, Michael Starzinger wrote:
nit: Likewise.
Done.
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);
On 2015/06/12 07:59:06, Michael Starzinger wrote:
nit: This DCHECK is subsumed by the one before, please drop it.
Will do. The reason for the paranoia is that this is the only place in
the code where I'm explicitly passing an invalid ic slot, because I know
that BuildVariableAssignment won't need 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);
On 2015/06/12 07:59:06, Michael Starzinger wrote:
nit: This DCHECK is subsumed by the one before, please drop it.
Done.
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);
On 2015/06/12 07:59:06, Michael Starzinger wrote:
nit: Let's drop this DCHECK.
Done.
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.
