Unify SoftDeoptimize and Deoptimize hydrogen instructions

Unify SoftDeoptimize and Deoptimize hydrogen instructions

BUG=chromium:258519
R=jkummerow@chromium.org

Committed: http://code.google.com/p/v8/source/detail?r=15827

[danno, 2013-07-18 14:11:24]

Unify SoftDeoptimize and Deoptimize hydrogen instructions

Also add convenience utilities for HReturn and HSimulate that can be used in
code related to the deoptimize changes.

[Jakob Kummerow, 2013-07-22 16:53:12]

I like this patch a lot! But I do have some comments.

https://codereview.chromium.org/19528003/diff/5001/src/arm/lithium-codegen-ar...
File src/arm/lithium-codegen-arm.cc (right):

https://codereview.chromium.org/19528003/diff/5001/src/arm/lithium-codegen-ar...
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...
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....
src/hydrogen-instructions.h:165: V(Deoptimize)                                \
alphabetic order please

https://codereview.chromium.org/19528003/diff/5001/src/hydrogen-instructions....
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-i...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/19528003/diff/5001/src/ia32/lithium-codegen-i...
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-i...
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-m...
File src/mips/lithium-codegen-mips.cc (right):

https://codereview.chromium.org/19528003/diff/5001/src/mips/lithium-codegen-m...
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-x6...
File src/x64/lithium-codegen-x64.cc (right):

https://codereview.chromium.org/19528003/diff/5001/src/x64/lithium-codegen-x6...
src/x64/lithium-codegen-x64.cc:5356: Deoptimize(instr->environment());
see ia32 comment

[danno, 2013-07-23 12:18:14]

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-ar...
File src/arm/lithium-codegen-arm.cc (right):

https://codereview.chromium.org/19528003/diff/5001/src/arm/lithium-codegen-ar...
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...
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....
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....
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-i...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/19528003/diff/5001/src/ia32/lithium-codegen-i...
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-i...
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-m...
File src/mips/lithium-codegen-mips.cc (right):

https://codereview.chromium.org/19528003/diff/5001/src/mips/lithium-codegen-m...
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-x6...
File src/x64/lithium-codegen-x64.cc (right):

https://codereview.chromium.org/19528003/diff/5001/src/x64/lithium-codegen-x6...
src/x64/lithium-codegen-x64.cc:5356: Deoptimize(instr->environment());
On 2013/07/22 16:53:12, Jakob wrote:
> see ia32 comment

Done.

[Jakob Kummerow, 2013-07-23 13:12:01]

LGTM!

[danno, 2013-07-23 13:35:25]

Committed patchset #5 manually as r15827 (presubmit successful).