I like this patch a lot! But I do have some comments.
https://codereview.chromium.org/19528003/diff/5001/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):
https://codereview.chromium.org/19528003/diff/5001/src/arm/lithium-codegen-arm.cc#newcode5632
src/arm/lithium-codegen-arm.cc:5632: if
(instr->hydrogen_value()->IsDeoptimize()) {
Uhm, what? What else would the hydrogen_value be? I think you meant to
check for hydrogen()->type() here (but see my comments on the ia32
version).
https://codereview.chromium.org/19528003/diff/5001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):
https://codereview.chromium.org/19528003/diff/5001/src/code-stubs-hydrogen.cc#newcode420
src/code-stubs-hydrogen.cc:420: environment()->Push(object);
I don't understand why this Push/Pop sequence is necessary. Why was the
old code not safe?
https://codereview.chromium.org/19528003/diff/5001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):
https://codereview.chromium.org/19528003/diff/5001/src/hydrogen-instructions.h#newcode165
src/hydrogen-instructions.h:165: V(Deoptimize)
\
alphabetic order please
https://codereview.chromium.org/19528003/diff/5001/src/hydrogen-instructions.h#newcode1496
src/hydrogen-instructions.h:1496: // We insert soft-deoptimize when we
hit code with unknown typefeedback,
comment is outdated
https://codereview.chromium.org/19528003/diff/5001/src/hydrogen.cc
File src/hydrogen.cc (right):
https://codereview.chromium.org/19528003/diff/5001/src/hydrogen.cc#newcode717
src/hydrogen.cc:717: merge_block_(NULL) {
nit: unnecessary whitespace change
https://codereview.chromium.org/19528003/diff/5001/src/hydrogen.cc#newcode855
src/hydrogen.cc:855: // Deopt on true. Nothing to do, just continue the
false block.
s/Deopt/Return/ ?
https://codereview.chromium.org/19528003/diff/5001/src/hydrogen.cc#newcode868
src/hydrogen.cc:868:
builder_->PadEnvironmentForContinuation(last_true_block_,
IIUC, this call never does anything, because merge_block_'s environment
is guaranteed to be a copy of last_true_block_'s environment, so in
particular it must have the same length.
Continuing this train of thought, I think the manual environment copying
is not necessary. The line
"last_true_block_->GotoNoSimulate(merge_block_);" which is
unconditionally executed below should handle that for you.
Or maybe this is a bug currently, and to handle the deopt_then_ case
correctly you actually need to do this instead above:
HBasicBlock* last_false_block = builder_->current_block();
HEnvironment* merge_env = deopt_then_
? last_false_block->last_environment()->Copy()
: last_true_block_->last_environment()->Copy()
https://codereview.chromium.org/19528003/diff/5001/src/hydrogen.cc#newcode1025
src/hydrogen.cc:1025: // environment by pushing undefined so that the
environment match during the
nit: "the environments match"
https://codereview.chromium.org/19528003/diff/5001/src/hydrogen.cc#newcode1040
src/hydrogen.cc:1040: Handle<Map> map) {
nit: fits on last line?
https://codereview.chromium.org/19528003/diff/5001/src/hydrogen.cc#newcode1725
src/hydrogen.cc:1725: Add<HDeoptimize>();
I'm not a fan of this parameterless convenience method. There is no
intuitive default deopt type, I think every call site should make an
explicit decision about the type.
https://codereview.chromium.org/19528003/diff/5001/src/hydrogen.h
File src/hydrogen.h (right):
https://codereview.chromium.org/19528003/diff/5001/src/hydrogen.h#newcode1428
src/hydrogen.h:1428:
isolate()->counters()->soft_deopts_requested()->Increment();
This sounds pretty SOFT-deopt specific. I think the method body you want
is:
if (type == Deoptimizer::SOFT) {
isolate()->counters()->soft_deopts_requested()->Increment();
if (FLAG_always_opt) return NULL;
}
if (current_block()->IsDeoptimizing()) return NULL;
HDeoptimize* instr = new(zone()) HDeoptimize(type);
AddInstruction(instr);
if (type == Deoptimizer::SOFT) {
isolate()->counters()->soft_deopts_inserted()->Increment();
graph()->set_has_soft_deoptimize(true);
}
current_block()->MarkAsDeoptimizing();
return instr;
https://codereview.chromium.org/19528003/diff/5001/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):
https://codereview.chromium.org/19528003/diff/5001/src/ia32/lithium-codegen-ia32.cc#newcode6322
src/ia32/lithium-codegen-ia32.cc:6322:
ASSERT(instr->hydrogen_value()->IsDeoptimize());
This explicit ASSERT is unnecessary, as "instr->hydrogen()" below
implicitly checks for that too.
https://codereview.chromium.org/19528003/diff/5001/src/ia32/lithium-codegen-ia32.cc#newcode6324
src/ia32/lithium-codegen-ia32.cc:6324: Deoptimize(instr->environment());
I think I'd prefer to extend Deoptimize()'s signature to take a
Deoptimizer::BailoutType as an additional parameter. Then you could just
always call:
Deoptimize(instr->environment, instr->hydrogen()->type());
here.
https://codereview.chromium.org/19528003/diff/5001/src/mips/lithium-codegen-mips.cc
File src/mips/lithium-codegen-mips.cc (right):
https://codereview.chromium.org/19528003/diff/5001/src/mips/lithium-codegen-mips.cc#newcode5624
src/mips/lithium-codegen-mips.cc:5624: if
(instr->hydrogen_value()->IsDeoptimize()) {
see ARM comment
https://codereview.chromium.org/19528003/diff/5001/src/x64/lithium-codegen-x64.cc
File src/x64/lithium-codegen-x64.cc (right):
https://codereview.chromium.org/19528003/diff/5001/src/x64/lithium-codegen-x64.cc#newcode5356
src/x64/lithium-codegen-x64.cc:5356: Deoptimize(instr->environment());
see ia32 comment
https://codereview.chromium.org/19528003/
--
--
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/groups/opt_out.
