PTAL. Sorry, my rebase get screwed up, so I don't have a rebased version of
the
old patch to compare against.
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()) {
On 2013/07/22 16:53:12, Jakob wrote:
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).
Done.
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);
On 2013/07/22 16:53:12, Jakob wrote:
I don't understand why this Push/Pop sequence is necessary. Why was
the old code
not safe?
Sort of. The problem is that there was no real merge of paths before, so
no phi node was needed at all for the result. Now that the deoptimize
path actually merges into the join at the end of the if, the return
value from the "true" path must be the input to a phi that the "else
deopt" path contributes a undefined or some other dummy value to.
Otherwise, the register allocator complains that there is a use before a
definition along the else path. It doesn't know that the the deoptimize
instruction makes the else value of the phi irrelevant.
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)
\
On 2013/07/22 16:53:12, Jakob wrote:
alphabetic order please
Done.
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,
On 2013/07/22 16:53:12, Jakob wrote:
comment is outdated
Done.
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) {
On 2013/07/22 16:53:12, Jakob wrote:
nit: unnecessary whitespace change
Done.
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.
On 2013/07/22 16:53:12, Jakob wrote:
s/Deopt/Return/ ?
Done.
https://codereview.chromium.org/19528003/diff/5001/src/hydrogen.cc#newcode868
src/hydrogen.cc:868:
builder_->PadEnvironmentForContinuation(last_true_block_,
On 2013/07/22 16:53:12, Jakob wrote:
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()
Done.
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
On 2013/07/22 16:53:12, Jakob wrote:
nit: "the environments match"
Done.
https://codereview.chromium.org/19528003/diff/5001/src/hydrogen.cc#newcode1040
src/hydrogen.cc:1040: Handle<Map> map) {
On 2013/07/22 16:53:12, Jakob wrote:
nit: fits on last line?
Done.
https://codereview.chromium.org/19528003/diff/5001/src/hydrogen.cc#newcode1725
src/hydrogen.cc:1725: Add<HDeoptimize>();
On 2013/07/22 16:53:12, Jakob wrote:
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.
Done.
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();
Good catch. Done.
On 2013/07/22 16:53:12, Jakob wrote:
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());
On 2013/07/22 16:53:12, Jakob wrote:
This explicit ASSERT is unnecessary, as "instr->hydrogen()" below
implicitly
checks for that too.
Done.
https://codereview.chromium.org/19528003/diff/5001/src/ia32/lithium-codegen-ia32.cc#newcode6324
src/ia32/lithium-codegen-ia32.cc:6324: Deoptimize(instr->environment());
On 2013/07/22 16:53:12, Jakob wrote:
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.
Done
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()) {
On 2013/07/22 16:53:12, Jakob wrote:
see ARM comment
Done.
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());
On 2013/07/22 16:53:12, Jakob wrote:
see ia32 comment
Done.
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.
