Inline Array.prototype.forEach in TurboFan

Inline Array.prototype.forEach in TurboFan

This CL contains a few pieces:

- A new mechanism to create "BuiltinContinuation" checkpoints in TurboFan 
  graphs, which--when triggered--swizzle the values in the the FrameState to be 
  parameters to a typically TF-generated builtin that resumes execution to finish
  the slow-case functionality.
- Continuation builtins that have special handling in the deoptimizer and their own 
  new frame type to ensure that the values they need to begin executing can be stashed
  away and restored immediately before the builtin is called via a trampoline that runs
  when the continuation builtin's frame execution resumes.
- An implementation of Array.prototype.forEach in TurboFan that can be used to 
  inline it. The inlined forEach implementation uses the checkpoints mechanism
  described above to deopt in the middle of the forEach in the cases that optimization
  invariants are violated. There is a slightly different continuation stub for each
  deopt point in the forEach implementation to ensure the correct side-effects, i.e.
  that the deopt of the builtin isn't programmatically observable.

Review-Url: https://codereview.chromium.org/2803853005
Cr-Commit-Position: refs/heads/master@{#45764}
Committed: https://chromium.googlesource.com/v8/v8/+/90c3a2d54b956d06ace2c97c6bfdb10ea9cb8e58

[danno, 2017-05-15 15:00:22]

Description was changed from

==========
Checkpoint for prototype of inlined forEach
==========

to

==========
Inline Array.prototype.forEach in TurboFan

This CL contains a few pieces:

- A new mechanism to create "DeoptToJavaScriptBuiltin" checkpoints in TurboFan 
  graphs, which--when triggered--swizzle the values in the the frame state to be

  parameters to a typically TF-generated builtin that resumes execution to
finish
  the slow-case functionality.
- An implementation of Array.prototype.forEach in TurboFan that can be used to 
  inline the function. The inlined forEach uses the mechanism uses the
checkpoints
  described above to deopt during the forEach in the cases that invariants are 
  violated. There is a slightly different of the continuation stub for each
deopt 
  point in the forEach implementation to ensure the right side-effects, i.e.
that 
  the deopt of the builtin isn't programmatically observable.
==========

[danno, 2017-05-15 15:00:22]

danno@chromium.org changed reviewers:
+ bmeurer@chromium.org, jarin@chromium.org, mstarzinger@chromium.org

[danno, 2017-05-15 15:01:46]

Description was changed from

==========
Inline Array.prototype.forEach in TurboFan

This CL contains a few pieces:

- A new mechanism to create "DeoptToJavaScriptBuiltin" checkpoints in TurboFan 
  graphs, which--when triggered--swizzle the values in the the frame state to be

  parameters to a typically TF-generated builtin that resumes execution to
finish
  the slow-case functionality.
- An implementation of Array.prototype.forEach in TurboFan that can be used to 
  inline the function. The inlined forEach uses the mechanism uses the
checkpoints
  described above to deopt during the forEach in the cases that invariants are 
  violated. There is a slightly different of the continuation stub for each
deopt 
  point in the forEach implementation to ensure the right side-effects, i.e.
that 
  the deopt of the builtin isn't programmatically observable.
==========

to

==========
Inline Array.prototype.forEach in TurboFan

This CL contains a few pieces:

- A new mechanism to create "DeoptToJavaScriptBuiltin" checkpoints in TurboFan 
  graphs, which--when triggered--swizzle the values in the the FrameState to be 
  parameters to a typically TF-generated builtin that resumes execution to
finish
  the slow-case functionality.
- An implementation of Array.prototype.forEach in TurboFan that can be used to 
  inline it. The inlined forEach implementation uses the checkpoints mechanism
  described above to deopt in the middle of the forEach in the cases that
invariants
  are violated. There is a slightly different continuation stub for each deopt 
  point in the forEach implementation to ensure the correct side-effects, i.e.
that 
  the deopt of the builtin isn't programmatically observable.
==========

[danno, 2017-05-15 15:02:39]

Description was changed from

==========
Inline Array.prototype.forEach in TurboFan

This CL contains a few pieces:

- A new mechanism to create "DeoptToJavaScriptBuiltin" checkpoints in TurboFan 
  graphs, which--when triggered--swizzle the values in the the FrameState to be 
  parameters to a typically TF-generated builtin that resumes execution to
finish
  the slow-case functionality.
- An implementation of Array.prototype.forEach in TurboFan that can be used to 
  inline it. The inlined forEach implementation uses the checkpoints mechanism
  described above to deopt in the middle of the forEach in the cases that
invariants
  are violated. There is a slightly different continuation stub for each deopt 
  point in the forEach implementation to ensure the correct side-effects, i.e.
that 
  the deopt of the builtin isn't programmatically observable.
==========

to

==========
Inline Array.prototype.forEach in TurboFan

This CL contains a few pieces:

- A new mechanism to create "JavaScriptBuiltinContinuation" checkpoints in
TurboFan 
  graphs, which--when triggered--swizzle the values in the the FrameState to be 
  parameters to a typically TF-generated builtin that resumes execution to
finish
  the slow-case functionality.
- An implementation of Array.prototype.forEach in TurboFan that can be used to 
  inline it. The inlined forEach implementation uses the checkpoints mechanism
  described above to deopt in the middle of the forEach in the cases that
invariants
  are violated. There is a slightly different continuation stub for each deopt 
  point in the forEach implementation to ensure the correct side-effects, i.e.
that 
  the deopt of the builtin isn't programmatically observable.
==========

[danno, 2017-05-15 15:06:28]

Description was changed from

==========
Inline Array.prototype.forEach in TurboFan

This CL contains a few pieces:

- A new mechanism to create "JavaScriptBuiltinContinuation" checkpoints in
TurboFan 
  graphs, which--when triggered--swizzle the values in the the FrameState to be 
  parameters to a typically TF-generated builtin that resumes execution to
finish
  the slow-case functionality.
- An implementation of Array.prototype.forEach in TurboFan that can be used to 
  inline it. The inlined forEach implementation uses the checkpoints mechanism
  described above to deopt in the middle of the forEach in the cases that
invariants
  are violated. There is a slightly different continuation stub for each deopt 
  point in the forEach implementation to ensure the correct side-effects, i.e.
that 
  the deopt of the builtin isn't programmatically observable.
==========

to

==========
Inline Array.prototype.forEach in TurboFan

This CL contains a few pieces:

- A new mechanism to create "JavaScriptBuiltinContinuation" checkpoints in
TurboFan 
  graphs, which--when triggered--swizzle the values in the the FrameState to be 
  parameters to a typically TF-generated builtin that resumes execution to
finish
  the slow-case functionality.
- Continuation builtins have special handling in the deoptimizer and their own 
  new frame type to ensure that the values they need to begin executing can be
stashed
  away and restored immediately before the builtin is called via a trampoline
that runs
  when the continuation builtin's frame execution resumes.
- An implementation of Array.prototype.forEach in TurboFan that can be used to 
  inline it. The inlined forEach implementation uses the checkpoints mechanism
  described above to deopt in the middle of the forEach in the cases that
invariants
  are violated. There is a slightly different continuation stub for each deopt 
  point in the forEach implementation to ensure the correct side-effects, i.e.
that 
  the deopt of the builtin isn't programmatically observable.
==========

[danno, 2017-05-15 15:07:23]

[WIP]

Please take a look at the overall design. Ports are missing, but x64 and ia32
work, and I'd like to get your feedback before completing all of the platforms.

[danno, 2017-05-16 07:27:45]

Description was changed from

==========
Inline Array.prototype.forEach in TurboFan

This CL contains a few pieces:

- A new mechanism to create "JavaScriptBuiltinContinuation" checkpoints in
TurboFan 
  graphs, which--when triggered--swizzle the values in the the FrameState to be 
  parameters to a typically TF-generated builtin that resumes execution to
finish
  the slow-case functionality.
- Continuation builtins have special handling in the deoptimizer and their own 
  new frame type to ensure that the values they need to begin executing can be
stashed
  away and restored immediately before the builtin is called via a trampoline
that runs
  when the continuation builtin's frame execution resumes.
- An implementation of Array.prototype.forEach in TurboFan that can be used to 
  inline it. The inlined forEach implementation uses the checkpoints mechanism
  described above to deopt in the middle of the forEach in the cases that
invariants
  are violated. There is a slightly different continuation stub for each deopt 
  point in the forEach implementation to ensure the correct side-effects, i.e.
that 
  the deopt of the builtin isn't programmatically observable.
==========

to

==========
Inline Array.prototype.forEach in TurboFan

This CL contains a few pieces:

- A new mechanism to create "JavaScriptBuiltinContinuation" checkpoints in
TurboFan 
  graphs, which--when triggered--swizzle the values in the the FrameState to be 
  parameters to a typically TF-generated builtin that resumes execution to
finish
  the slow-case functionality.
- Continuation builtins that have special handling in the deoptimizer and their
own 
  new frame type to ensure that the values they need to begin executing can be
stashed
  away and restored immediately before the builtin is called via a trampoline
that runs
  when the continuation builtin's frame execution resumes.
- An implementation of Array.prototype.forEach in TurboFan that can be used to 
  inline it. The inlined forEach implementation uses the checkpoints mechanism
  described above to deopt in the middle of the forEach in the cases that
invariants
  are violated. There is a slightly different continuation stub for each deopt 
  point in the forEach implementation to ensure the correct side-effects, i.e.
that 
  the deopt of the builtin isn't programmatically observable.
==========

[danno, 2017-05-16 07:28:39]

Description was changed from

==========
Inline Array.prototype.forEach in TurboFan

This CL contains a few pieces:

- A new mechanism to create "JavaScriptBuiltinContinuation" checkpoints in
TurboFan 
  graphs, which--when triggered--swizzle the values in the the FrameState to be 
  parameters to a typically TF-generated builtin that resumes execution to
finish
  the slow-case functionality.
- Continuation builtins that have special handling in the deoptimizer and their
own 
  new frame type to ensure that the values they need to begin executing can be
stashed
  away and restored immediately before the builtin is called via a trampoline
that runs
  when the continuation builtin's frame execution resumes.
- An implementation of Array.prototype.forEach in TurboFan that can be used to 
  inline it. The inlined forEach implementation uses the checkpoints mechanism
  described above to deopt in the middle of the forEach in the cases that
invariants
  are violated. There is a slightly different continuation stub for each deopt 
  point in the forEach implementation to ensure the correct side-effects, i.e.
that 
  the deopt of the builtin isn't programmatically observable.
==========

to

==========
Inline Array.prototype.forEach in TurboFan

This CL contains a few pieces:

- A new mechanism to create "JavaScriptBuiltinContinuation" checkpoints in
TurboFan 
  graphs, which--when triggered--swizzle the values in the the FrameState to be 
  parameters to a typically TF-generated builtin that resumes execution to
finish
  the slow-case functionality.
- Continuation builtins that have special handling in the deoptimizer and their
own 
  new frame type to ensure that the values they need to begin executing can be
stashed
  away and restored immediately before the builtin is called via a trampoline
that runs
  when the continuation builtin's frame execution resumes.
- An implementation of Array.prototype.forEach in TurboFan that can be used to 
  inline it. The inlined forEach implementation uses the checkpoints mechanism
  described above to deopt in the middle of the forEach in the cases that
optimization
  invariants are violated. There is a slightly different continuation stub for
each
  deopt point in the forEach implementation to ensure the correct side-effects,
i.e.
  that the deopt of the builtin isn't programmatically observable.
==========

[Jarin, 2017-05-16 20:00:04]

Only looked at deoptimizer.cc. 

The overall approach looks good, I have only minor comments there. Could you add
more explanations to the comment before the function? In particular, the
handling of the lazy deopt return value and the tagging quirks would deserve
more documentation.

https://codereview.chromium.org/2803853005/diff/80001/src/deoptimizer.cc
File src/deoptimizer.cc (right):

https://codereview.chromium.org/2803853005/diff/80001/src/deoptimizer.cc#newc...
src/deoptimizer.cc:2258: register_values[i] = 0;
I am surprised this works. vector::reserve only reserves capacity, but it does
not change the size.

I think you want register_values.resize(total_registers, 0) instead of the
reservation and the loop.

https://codereview.chromium.org/2803853005/diff/80001/src/deoptimizer.cc#newc...
src/deoptimizer.cc:2274: }
The checking above seems hard to follow. How about 

if (type == MachineType::Int32()) {
  // An int32 argument is only supported for the arguments count parameter
  // to a JavaScript builtin stub.
  CHECK_EQ(code, kJavaScriptCallArgCountRegister.code());
  java_script_builtin = true;
} else {
  // Any other argument must be a tagged value.
  CHECK(IsAnyTagged(type.representation());
}

https://codereview.chromium.org/2803853005/diff/80001/src/deoptimizer.cc#newc...
src/deoptimizer.cc:2299: "placeholder for return result on lazy deopt ");
This would deserve a separate paragraph in the comment preceding the function.

[danno, 2017-05-17 10:54:18]

Please take another look. For a feeling of how both JavaScript and normal
builtin continuations work, I added the bug fix for v8:6373. If this is shaping
up as you'd like, then I will split the CL back up and land in pieces.

https://codereview.chromium.org/2803853005/diff/80001/src/deoptimizer.cc
File src/deoptimizer.cc (right):

https://codereview.chromium.org/2803853005/diff/80001/src/deoptimizer.cc#newc...
src/deoptimizer.cc:2258: register_values[i] = 0;
On 2017/05/16 20:00:04, Jarin wrote:
> I am surprised this works. vector::reserve only reserves capacity, but it does
> not change the size.
> 
> I think you want register_values.resize(total_registers, 0) instead of the
> reservation and the loop.

Done.

https://codereview.chromium.org/2803853005/diff/80001/src/deoptimizer.cc#newc...
src/deoptimizer.cc:2274: }
On 2017/05/16 20:00:04, Jarin wrote:
> The checking above seems hard to follow. How about 
> 
> if (type == MachineType::Int32()) {
>   // An int32 argument is only supported for the arguments count parameter
>   // to a JavaScript builtin stub.
>   CHECK_EQ(code, kJavaScriptCallArgCountRegister.code());
>   java_script_builtin = true;
> } else {
>   // Any other argument must be a tagged value.
>   CHECK(IsAnyTagged(type.representation());
> }

Done.

https://codereview.chromium.org/2803853005/diff/80001/src/deoptimizer.cc#newc...
src/deoptimizer.cc:2299: "placeholder for return result on lazy deopt ");
On 2017/05/16 20:00:04, Jarin wrote:
> This would deserve a separate paragraph in the comment preceding the function.

Done.

[Benedikt Meurer, 2017-05-18 06:32:52]

https://codereview.chromium.org/2803853005/diff/100001/src/builtins/builtins-...
File src/builtins/builtins-conversion-gen.cc (right):

https://codereview.chromium.org/2803853005/diff/100001/src/builtins/builtins-...
src/builtins/builtins-conversion-gen.cc:255:
TF_BUILTIN(ToBooleanLazyDeoptContinuation, CodeStubAssembler) {
That should be in a separate follow-up CL.

https://codereview.chromium.org/2803853005/diff/100001/src/builtins/ia32/buil...
File src/builtins/ia32/builtins-ia32.cc (right):

https://codereview.chromium.org/2803853005/diff/100001/src/builtins/ia32/buil...
src/builtins/ia32/builtins-ia32.cc:1417: static void
Generate_ContinueToBuiltinHelper(MacroAssembler* masm,
Nit: No static inside anonymous namespace.

https://codereview.chromium.org/2803853005/diff/120001/src/compiler/frame-sta...
File src/compiler/frame-states.h (right):

https://codereview.chromium.org/2803853005/diff/120001/src/compiler/frame-sta...
src/compiler/frame-states.h:168: enum CheckpointMode { CREATE_CHECKPOINT,
DONT_CREATE_CHECKPOINT };
Nit: enum class ContinuationFrameStateType { kEager, kLazy };

https://codereview.chromium.org/2803853005/diff/120001/src/compiler/frame-sta...
src/compiler/frame-states.h:170: std::pair<Node*, Node*>
CreateStubBuiltinContinuationFrameState(
The effect doesn't seem to make sense here. How about just having an API here
that is responsible for creating the appropriate FrameState and then create and
wire up the CheckPoint independently at the use sites.

https://codereview.chromium.org/2803853005/diff/120001/src/compiler/frame-sta...
src/compiler/frame-states.h:172: int parameter_count, Node* effect, Node*
control, Node* outer_frame_state,
You don't need the effect, control inputs here.

https://codereview.chromium.org/2803853005/diff/120001/src/compiler/frame-sta...
src/compiler/frame-states.h:175: std::pair<Node*, Node*>
CreateJavaScriptBuiltinContinuationFrameState(
Same as above.

[danno, 2017-05-19 21:08:52]

The CQ bit was checked by danno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-19 21:08:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-19 21:10:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-19 21:10:04]

Dry run: Try jobs failed on following builders:
  v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/26171)

[danno, 2017-05-19 21:12:32]

The CQ bit was checked by danno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-19 21:12:34]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-19 21:17:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-19 21:17:04]

Dry run: Try jobs failed on following builders:
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/27944)

[danno, 2017-05-19 21:20:50]

The CQ bit was checked by danno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-19 21:21:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-19 22:09:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-19 22:09:10]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)

[danno, 2017-05-22 06:34:41]

The CQ bit was checked by danno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 06:34:49]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danno, 2017-05-22 06:35:24]

Description was changed from

==========
Inline Array.prototype.forEach in TurboFan

This CL contains a few pieces:

- A new mechanism to create "JavaScriptBuiltinContinuation" checkpoints in
TurboFan 
  graphs, which--when triggered--swizzle the values in the the FrameState to be 
  parameters to a typically TF-generated builtin that resumes execution to
finish
  the slow-case functionality.
- Continuation builtins that have special handling in the deoptimizer and their
own 
  new frame type to ensure that the values they need to begin executing can be
stashed
  away and restored immediately before the builtin is called via a trampoline
that runs
  when the continuation builtin's frame execution resumes.
- An implementation of Array.prototype.forEach in TurboFan that can be used to 
  inline it. The inlined forEach implementation uses the checkpoints mechanism
  described above to deopt in the middle of the forEach in the cases that
optimization
  invariants are violated. There is a slightly different continuation stub for
each
  deopt point in the forEach implementation to ensure the correct side-effects,
i.e.
  that the deopt of the builtin isn't programmatically observable.
==========

to

==========
Inline Array.prototype.forEach in TurboFan

This CL contains a few pieces:

- A new mechanism to create "BuiltinContinuation" checkpoints in TurboFan 
  graphs, which--when triggered--swizzle the values in the the FrameState to be 
  parameters to a typically TF-generated builtin that resumes execution to
finish
  the slow-case functionality.
- Continuation builtins that have special handling in the deoptimizer and their
own 
  new frame type to ensure that the values they need to begin executing can be
stashed
  away and restored immediately before the builtin is called via a trampoline
that runs
  when the continuation builtin's frame execution resumes.
- An implementation of Array.prototype.forEach in TurboFan that can be used to 
  inline it. The inlined forEach implementation uses the checkpoints mechanism
  described above to deopt in the middle of the forEach in the cases that
optimization
  invariants are violated. There is a slightly different continuation stub for
each
  deopt point in the forEach implementation to ensure the correct side-effects,
i.e.
  that the deopt of the builtin isn't programmatically observable.
==========

[danno, 2017-05-22 06:35:25]

Description was changed from

==========
Inline Array.prototype.forEach in TurboFan

This CL contains a few pieces:

- A new mechanism to create "JavaScriptBuiltinContinuation" checkpoints in
TurboFan 
  graphs, which--when triggered--swizzle the values in the the FrameState to be 
  parameters to a typically TF-generated builtin that resumes execution to
finish
  the slow-case functionality.
- Continuation builtins that have special handling in the deoptimizer and their
own 
  new frame type to ensure that the values they need to begin executing can be
stashed
  away and restored immediately before the builtin is called via a trampoline
that runs
  when the continuation builtin's frame execution resumes.
- An implementation of Array.prototype.forEach in TurboFan that can be used to 
  inline it. The inlined forEach implementation uses the checkpoints mechanism
  described above to deopt in the middle of the forEach in the cases that
optimization
  invariants are violated. There is a slightly different continuation stub for
each
  deopt point in the forEach implementation to ensure the correct side-effects,
i.e.
  that the deopt of the builtin isn't programmatically observable.
==========

to

==========
Inline Array.prototype.forEach in TurboFan

This CL contains a few pieces:

- A new mechanism to create "BuiltinContinuation" checkpoints in TurboFan 
  graphs, which--when triggered--swizzle the values in the the FrameState to be 
  parameters to a typically TF-generated builtin that resumes execution to
finish
  the slow-case functionality.
- Continuation builtins that have special handling in the deoptimizer and their
own 
  new frame type to ensure that the values they need to begin executing can be
stashed
  away and restored immediately before the builtin is called via a trampoline
that runs
  when the continuation builtin's frame execution resumes.
- An implementation of Array.prototype.forEach in TurboFan that can be used to 
  inline it. The inlined forEach implementation uses the checkpoints mechanism
  described above to deopt in the middle of the forEach in the cases that
optimization
  invariants are violated. There is a slightly different continuation stub for
each
  deopt point in the forEach implementation to ensure the correct side-effects,
i.e.
  that the deopt of the builtin isn't programmatically observable.
==========

[commit-bot: I haz the power, 2017-05-22 07:09:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-22 07:09:03]

Dry run: This issue passed the CQ dry run.

[danno, 2017-05-22 10:28:11]

Feedback addressed. Please take another look.

https://codereview.chromium.org/2803853005/diff/100001/src/builtins/builtins-...
File src/builtins/builtins-conversion-gen.cc (right):

https://codereview.chromium.org/2803853005/diff/100001/src/builtins/builtins-...
src/builtins/builtins-conversion-gen.cc:255:
TF_BUILTIN(ToBooleanLazyDeoptContinuation, CodeStubAssembler) {
On 2017/05/18 06:32:52, Benedikt Meurer wrote:
> That should be in a separate follow-up CL.

Done.

https://codereview.chromium.org/2803853005/diff/100001/src/builtins/ia32/buil...
File src/builtins/ia32/builtins-ia32.cc (right):

https://codereview.chromium.org/2803853005/diff/100001/src/builtins/ia32/buil...
src/builtins/ia32/builtins-ia32.cc:1417: static void
Generate_ContinueToBuiltinHelper(MacroAssembler* masm,
On 2017/05/18 06:32:52, Benedikt Meurer wrote:
> Nit: No static inside anonymous namespace.

Done.

https://codereview.chromium.org/2803853005/diff/120001/src/compiler/frame-sta...
File src/compiler/frame-states.h (right):

https://codereview.chromium.org/2803853005/diff/120001/src/compiler/frame-sta...
src/compiler/frame-states.h:168: enum CheckpointMode { CREATE_CHECKPOINT,
DONT_CREATE_CHECKPOINT };
On 2017/05/18 06:32:52, Benedikt Meurer wrote:
> Nit: enum class ContinuationFrameStateType { kEager, kLazy };

Done.

https://codereview.chromium.org/2803853005/diff/120001/src/compiler/frame-sta...
src/compiler/frame-states.h:170: std::pair<Node*, Node*>
CreateStubBuiltinContinuationFrameState(
On 2017/05/18 06:32:52, Benedikt Meurer wrote:
> The effect doesn't seem to make sense here. How about just having an API here
> that is responsible for creating the appropriate FrameState and then create
and
> wire up the CheckPoint independently at the use sites.

Done.

https://codereview.chromium.org/2803853005/diff/120001/src/compiler/frame-sta...
src/compiler/frame-states.h:172: int parameter_count, Node* effect, Node*
control, Node* outer_frame_state,
On 2017/05/18 06:32:52, Benedikt Meurer wrote:
> You don't need the effect, control inputs here.

Done.

https://codereview.chromium.org/2803853005/diff/120001/src/compiler/frame-sta...
src/compiler/frame-states.h:175: std::pair<Node*, Node*>
CreateJavaScriptBuiltinContinuationFrameState(
On 2017/05/18 06:32:52, Benedikt Meurer wrote:
> Same as above.

Done.

[Benedikt Meurer, 2017-05-22 19:29:40]

Next round of comments (only looked at compiler).

https://codereview.chromium.org/2803853005/diff/240001/src/compiler/js-call-r...
File src/compiler/js-call-reducer.cc (right):

https://codereview.chromium.org/2803853005/diff/240001/src/compiler/js-call-r...
src/compiler/js-call-reducer.cc:373: if
(!NodeProperties::InferReceiverMaps(receiver, effect, &receiver_maps)) {
This returns an enum. You only want to deal with reliable receiver maps for now.

https://codereview.chromium.org/2803853005/diff/240001/src/compiler/js-call-r...
src/compiler/js-call-reducer.cc:438: USE(length);
This is unnecessary.

https://codereview.chromium.org/2803853005/diff/240001/src/compiler/js-call-r...
src/compiler/js-call-reducer.cc:460: javascript()->Call(5, p.frequency()),
fncallback, this_arg, element, k,
This doesn't properly rewire IfException edges. Can you add an mjsunit test for
this? And either implement the rewiring or disable this optimization if node has
exception projection.

https://codereview.chromium.org/2803853005/diff/240001/src/compiler/js-call-r...
src/compiler/js-call-reducer.cc:462: USE(call_back);
You don't seem to need the call_back variable.

https://codereview.chromium.org/2803853005/diff/240001/src/compiler/js-call-r...
src/compiler/js-call-reducer.cc:463: USE(this_arg);
This is unnecessary.

https://codereview.chromium.org/2803853005/diff/240001/src/compiler/js-call-r...
src/compiler/js-call-reducer.cc:474: NodeProperties::ReplaceUses(node,
jsgraph()->UndefinedConstant(), effect,
Can you use ReplaceWithValue instead of this magic (and the lines below)?

https://codereview.chromium.org/2803853005/diff/240001/src/compiler/js-call-r...
File src/compiler/js-call-reducer.h (right):

https://codereview.chromium.org/2803853005/diff/240001/src/compiler/js-call-r...
src/compiler/js-call-reducer.h:9: #include "src/builtins/builtins.h"
We don't need this header here.

[Jarin, 2017-05-24 06:41:22]

lgtm, modulo nits and Benedikt's comments.

https://codereview.chromium.org/2803853005/diff/240001/src/builtins/builtins-...
File src/builtins/builtins-definitions.h (right):

https://codereview.chromium.org/2803853005/diff/240001/src/builtins/builtins-...
src/builtins/builtins-definitions.h:172: /* directly.                           
                                  */ \
Nit: This would read better if it was written as a sequence of steps.

Something like:
  1. Trampoline restores input register values that 
     the builtin expects from a BuiltinContinuationFrame.
  2. Trampoline tears down BuiltinContinuationFrame.
  3. Trampoline jumps to the builtin's address.
  4. Builtin executes as if invoked by the frame above it.
  5. When the builtin returns, execution resumes normally 
     in the calling frame, processing any return result from 
     the JavaScript builtin as if it had called the builtin
     directly.

The trampolines below implement steps 1-3.

https://codereview.chromium.org/2803853005/diff/240001/src/builtins/ia32/buil...
File src/builtins/ia32/builtins-ia32.cc (right):

https://codereview.chromium.org/2803853005/diff/240001/src/builtins/ia32/buil...
src/builtins/ia32/builtins-ia32.cc:1480: __ ret(0);  // Return to IC Miss stub,
continuation still on stack.
Fix comment?

https://codereview.chromium.org/2803853005/diff/240001/src/builtins/x64/built...
File src/builtins/x64/builtins-x64.cc (right):

https://codereview.chromium.org/2803853005/diff/240001/src/builtins/x64/built...
src/builtins/x64/builtins-x64.cc:1435: // registers.
Improve comment? (We do not really tail call runtime from here...)

https://codereview.chromium.org/2803853005/diff/240001/src/builtins/x64/built...
src/builtins/x64/builtins-x64.cc:1443: __ ret(0);  // Return to IC Miss stub,
continuation still on stack.
Fix comment.

https://codereview.chromium.org/2803853005/diff/240001/src/builtins/x64/built...
src/builtins/x64/builtins-x64.cc:1456: rax);
Is this the bit that overwrites the hole inserted by the deoptimizer? Please add
a comment to that effect.

https://codereview.chromium.org/2803853005/diff/240001/src/compiler/frame-sta...
File src/compiler/frame-states.cc (right):

https://codereview.chromium.org/2803853005/diff/240001/src/compiler/frame-sta...
src/compiler/frame-states.cc:121: op, params_node, node0, node0,
js_graph->UndefinedConstant(),
node0 --> js_graph->EmptyStateValues()

https://codereview.chromium.org/2803853005/diff/240001/src/compiler/frame-sta...
File src/compiler/frame-states.h (right):

https://codereview.chromium.org/2803853005/diff/240001/src/compiler/frame-sta...
src/compiler/frame-states.h:87: kBuiltinContinuation   // Represents a
continuation to a stub
I am sorry for this nit, I just have to say this: add a dot to the end.

https://codereview.chromium.org/2803853005/diff/240001/src/compiler/js-call-r...
File src/compiler/js-call-reducer.cc (right):

https://codereview.chromium.org/2803853005/diff/240001/src/compiler/js-call-r...
src/compiler/js-call-reducer.cc:385: Node* k = jsgraph()->Constant(0);
a.k.a. ZeroConstant()

[Michael Starzinger, 2017-05-24 13:54:59]

https://codereview.chromium.org/2803853005/diff/240001/src/builtins/builtins-...
File src/builtins/builtins-definitions.h (right):

https://codereview.chromium.org/2803853005/diff/240001/src/builtins/builtins-...
src/builtins/builtins-definitions.h:172: /* directly.                           
                                  */ \
On 2017/05/24 06:41:22, Jarin wrote:
> Nit: This would read better if it was written as a sequence of steps.
> 
> Something like:
>   1. Trampoline restores input register values that 
>      the builtin expects from a BuiltinContinuationFrame.
>   2. Trampoline tears down BuiltinContinuationFrame.
>   3. Trampoline jumps to the builtin's address.
>   4. Builtin executes as if invoked by the frame above it.
>   5. When the builtin returns, execution resumes normally 
>      in the calling frame, processing any return result from 
>      the JavaScript builtin as if it had called the builtin
>      directly.
> 
> The trampolines below implement steps 1-3.

+1

https://codereview.chromium.org/2803853005/diff/240001/src/builtins/builtins.h
File src/builtins/builtins.h (right):

https://codereview.chromium.org/2803853005/diff/240001/src/builtins/builtins....
src/builtins/builtins.h:11: #include "src/utils.h"
nit: Assuming this is only needed for BailoutId, we could instead
forward-declare "class BailoutId" below.

https://codereview.chromium.org/2803853005/diff/240001/src/builtins/ia32/buil...
File src/builtins/ia32/builtins-ia32.cc (right):

https://codereview.chromium.org/2803853005/diff/240001/src/builtins/ia32/buil...
src/builtins/ia32/builtins-ia32.cc:1504: __ add(esp, Immediate(kPointerSize));
nit: __ Drop(1);

https://codereview.chromium.org/2803853005/diff/240001/src/compiler/frame-sta...
File src/compiler/frame-states.h (right):

https://codereview.chromium.org/2803853005/diff/240001/src/compiler/frame-sta...
src/compiler/frame-states.h:166: class Node;
nit: Please hoist forward declarations all the way to the top of the file.

https://codereview.chromium.org/2803853005/diff/240001/src/compiler/js-call-r...
File src/compiler/js-call-reducer.cc (right):

https://codereview.chromium.org/2803853005/diff/240001/src/compiler/js-call-r...
src/compiler/js-call-reducer.cc:474: NodeProperties::ReplaceUses(node,
jsgraph()->UndefinedConstant(), effect,
On 2017/05/22 19:29:40, Benedikt Meurer wrote:
> Can you use ReplaceWithValue instead of this magic (and the lines below)?

+1, otherwise we might run into similar issues as here:
https://chromium-review.googlesource.com/508712

https://codereview.chromium.org/2803853005/diff/240001/src/compiler/js-native...
File src/compiler/js-native-context-specialization.h (right):

https://codereview.chromium.org/2803853005/diff/240001/src/compiler/js-native...
src/compiler/js-native-context-specialization.h:9: #include
"src/builtins/builtins.h"
nit: Seems like a left-over. Can we drop this one?

https://codereview.chromium.org/2803853005/diff/240001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/2803853005/diff/240001/src/flag-definitions.h...
src/flag-definitions.h:805: DEFINE_BOOL(inline_array_builtins, true,
nit: Actually a TurboFan flag. Can we name this FLAG_turbo_inline_array_builtins
and move it into the TurboFan section of this file?

https://codereview.chromium.org/2803853005/diff/240001/test/mjsunit/optimized...
File test/mjsunit/optimized-foreach.js (right):

https://codereview.chromium.org/2803853005/diff/240001/test/mjsunit/optimized...
test/mjsunit/optimized-foreach.js:1: // Copyright 2017 the V8 project authors.
All rights reserved.
We should add test coverage for the following:
- Call to a.forEach where "a" is inline allocated and potentially escape
analyzed (i.e. by just having the array literal in the same function as the call
to "forEach").
- Call to a.forEach is done inside a try-catch block and the callback function
being called actually throws.
- Bonus points for the above: Throw after deoptimizing the caller (i.e. throwing
into a lazily deoptimized caller).
- Assuming that the below tests all inline the callback function, add a test
where the callback function is specifically not inlined (i.e. using
%NeverOptimizeFunction on callback function).

[Michael Starzinger, 2017-05-24 14:14:59]

The following test case seems to crash in the GC:

(function() {
  var result = 0;
  var lazyDeopt = function(deopt) {
    var b = [1,2,3];
    var sum = function(v,i,o) {
      result += i;
      if (i == 1 && deopt) {
        %DeoptimizeFunction(lazyDeopt);
      }
      gc(); gc();
      print("MARK", i, o);
    };
    %NeverOptimizeFunction(sum);
    b.forEach(sum);
  }
  lazyDeopt();
  lazyDeopt();
  %OptimizeFunctionOnNextCall(lazyDeopt);
  lazyDeopt();
  lazyDeopt(true);
  lazyDeopt();
})();

[danno, 2017-05-31 00:32:41]

The CQ bit was checked by danno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 00:32:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-31 00:38:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-31 00:38:02]

Dry run: Try jobs failed on following builders:
  v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...)

[danno, 2017-06-04 23:17:33]

The CQ bit was checked by danno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-04 23:17:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-04 23:23:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-04 23:23:10]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/26835)
  v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...)

[danno, 2017-06-05 17:31:09]

The CQ bit was checked by danno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-05 17:31:28]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-05 18:18:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-05 18:18:34]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)

[danno, 2017-06-06 07:27:56]

The CQ bit was checked by danno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-06 07:28:07]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-06 07:59:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-06 07:59:09]

Dry run: This issue passed the CQ dry run.

[danno, 2017-06-06 12:04:53]

Feedback addressed (hopefully all) and new tests added. Unfortunately,
addressing one of the bugs that mstarzinger pointed out required a non-trivial
amount of rework (Thanks Michi, super catch!!). 

Please take another look.

https://codereview.chromium.org/2803853005/diff/240001/src/builtins/builtins-...
File src/builtins/builtins-definitions.h (right):

https://codereview.chromium.org/2803853005/diff/240001/src/builtins/builtins-...
src/builtins/builtins-definitions.h:172: /* directly.                           
                                  */ \
On 2017/05/24 06:41:22, Jarin wrote:
> Nit: This would read better if it was written as a sequence of steps.
> 
> Something like:
>   1. Trampoline restores input register values that 
>      the builtin expects from a BuiltinContinuationFrame.
>   2. Trampoline tears down BuiltinContinuationFrame.
>   3. Trampoline jumps to the builtin's address.
>   4. Builtin executes as if invoked by the frame above it.
>   5. When the builtin returns, execution resumes normally 
>      in the calling frame, processing any return result from 
>      the JavaScript builtin as if it had called the builtin
>      directly.
> 
> The trampolines below implement steps 1-3.

Done.

https://codereview.chromium.org/2803853005/diff/240001/src/builtins/builtins-...
src/builtins/builtins-definitions.h:172: /* directly.                           
                                  */ \
On 2017/05/24 13:54:59, Michael Starzinger wrote:
> On 2017/05/24 06:41:22, Jarin wrote:
> > Nit: This would read better if it was written as a sequence of steps.
> > 
> > Something like:
> >   1. Trampoline restores input register values that 
> >      the builtin expects from a BuiltinContinuationFrame.
> >   2. Trampoline tears down BuiltinContinuationFrame.
> >   3. Trampoline jumps to the builtin's address.
> >   4. Builtin executes as if invoked by the frame above it.
> >   5. When the builtin returns, execution resumes normally 
> >      in the calling frame, processing any return result from 
> >      the JavaScript builtin as if it had called the builtin
> >      directly.
> > 
> > The trampolines below implement steps 1-3.
> 
> +1

Done.

https://codereview.chromium.org/2803853005/diff/240001/src/builtins/builtins.h
File src/builtins/builtins.h (right):

https://codereview.chromium.org/2803853005/diff/240001/src/builtins/builtins....
src/builtins/builtins.h:11: #include "src/utils.h"
On 2017/05/24 13:54:59, Michael Starzinger wrote:
> nit: Assuming this is only needed for BailoutId, we could instead
> forward-declare "class BailoutId" below.

Done.

https://codereview.chromium.org/2803853005/diff/240001/src/builtins/ia32/buil...
File src/builtins/ia32/builtins-ia32.cc (right):

https://codereview.chromium.org/2803853005/diff/240001/src/builtins/ia32/buil...
src/builtins/ia32/builtins-ia32.cc:1480: __ ret(0);  // Return to IC Miss stub,
continuation still on stack.
On 2017/05/24 06:41:22, Jarin wrote:
> Fix comment?

Done, here an elsewhere.

https://codereview.chromium.org/2803853005/diff/240001/src/builtins/ia32/buil...
src/builtins/ia32/builtins-ia32.cc:1504: __ add(esp, Immediate(kPointerSize));
On 2017/05/24 13:54:59, Michael Starzinger wrote:
> nit: __ Drop(1);

Done.

https://codereview.chromium.org/2803853005/diff/240001/src/builtins/x64/built...
File src/builtins/x64/builtins-x64.cc (right):

https://codereview.chromium.org/2803853005/diff/240001/src/builtins/x64/built...
src/builtins/x64/builtins-x64.cc:1435: // registers.
On 2017/05/24 06:41:22, Jarin wrote:
> Improve comment? (We do not really tail call runtime from here...)

Done.

https://codereview.chromium.org/2803853005/diff/240001/src/builtins/x64/built...
src/builtins/x64/builtins-x64.cc:1443: __ ret(0);  // Return to IC Miss stub,
continuation still on stack.
On 2017/05/24 06:41:22, Jarin wrote:
> Fix comment.

Done.

https://codereview.chromium.org/2803853005/diff/240001/src/builtins/x64/built...
src/builtins/x64/builtins-x64.cc:1456: rax);
On 2017/05/24 06:41:22, Jarin wrote:
> Is this the bit that overwrites the hole inserted by the deoptimizer? Please
add
> a comment to that effect.

Done.

https://codereview.chromium.org/2803853005/diff/240001/src/compiler/frame-sta...
File src/compiler/frame-states.cc (right):

https://codereview.chromium.org/2803853005/diff/240001/src/compiler/frame-sta...
src/compiler/frame-states.cc:121: op, params_node, node0, node0,
js_graph->UndefinedConstant(),
On 2017/05/24 06:41:22, Jarin wrote:
> node0 --> js_graph->EmptyStateValues()

Done.

https://codereview.chromium.org/2803853005/diff/240001/src/compiler/frame-sta...
File src/compiler/frame-states.h (right):

https://codereview.chromium.org/2803853005/diff/240001/src/compiler/frame-sta...
src/compiler/frame-states.h:87: kBuiltinContinuation   // Represents a
continuation to a stub
On 2017/05/24 06:41:22, Jarin wrote:
> I am sorry for this nit, I just have to say this: add a dot to the end.

Done.

https://codereview.chromium.org/2803853005/diff/240001/src/compiler/frame-sta...
src/compiler/frame-states.h:166: class Node;
On 2017/05/24 13:54:59, Michael Starzinger wrote:
> nit: Please hoist forward declarations all the way to the top of the file.

Done.

https://codereview.chromium.org/2803853005/diff/240001/src/compiler/js-call-r...
File src/compiler/js-call-reducer.cc (right):

https://codereview.chromium.org/2803853005/diff/240001/src/compiler/js-call-r...
src/compiler/js-call-reducer.cc:373: if
(!NodeProperties::InferReceiverMaps(receiver, effect, &receiver_maps)) {
On 2017/05/22 19:29:40, Benedikt Meurer wrote:
> This returns an enum. You only want to deal with reliable receiver maps for
now.

Done.

https://codereview.chromium.org/2803853005/diff/240001/src/compiler/js-call-r...
src/compiler/js-call-reducer.cc:385: Node* k = jsgraph()->Constant(0);
On 2017/05/24 06:41:22, Jarin wrote:
> a.k.a. ZeroConstant()

Done.

https://codereview.chromium.org/2803853005/diff/240001/src/compiler/js-call-r...
src/compiler/js-call-reducer.cc:438: USE(length);
On 2017/05/22 19:29:40, Benedikt Meurer wrote:
> This is unnecessary.

Done.

https://codereview.chromium.org/2803853005/diff/240001/src/compiler/js-call-r...
src/compiler/js-call-reducer.cc:460: javascript()->Call(5, p.frequency()),
fncallback, this_arg, element, k,
On 2017/05/22 19:29:40, Benedikt Meurer wrote:
> This doesn't properly rewire IfException edges. Can you add an mjsunit test
for
> this? And either implement the rewiring or disable this optimization if node
has
> exception projection.

Done.

https://codereview.chromium.org/2803853005/diff/240001/src/compiler/js-call-r...
src/compiler/js-call-reducer.cc:462: USE(call_back);
On 2017/05/22 19:29:40, Benedikt Meurer wrote:
> You don't seem to need the call_back variable.

Done.

https://codereview.chromium.org/2803853005/diff/240001/src/compiler/js-call-r...
src/compiler/js-call-reducer.cc:463: USE(this_arg);
On 2017/05/22 19:29:40, Benedikt Meurer wrote:
> This is unnecessary.

Done.

https://codereview.chromium.org/2803853005/diff/240001/src/compiler/js-call-r...
src/compiler/js-call-reducer.cc:474: NodeProperties::ReplaceUses(node,
jsgraph()->UndefinedConstant(), effect,
On 2017/05/24 13:54:59, Michael Starzinger wrote:
> On 2017/05/22 19:29:40, Benedikt Meurer wrote:
> > Can you use ReplaceWithValue instead of this magic (and the lines below)?
> 
> +1, otherwise we might run into similar issues as here:
> https://chromium-review.googlesource.com/508712

Done.

https://codereview.chromium.org/2803853005/diff/240001/src/compiler/js-call-r...
src/compiler/js-call-reducer.cc:474: NodeProperties::ReplaceUses(node,
jsgraph()->UndefinedConstant(), effect,
On 2017/05/22 19:29:40, Benedikt Meurer wrote:
> Can you use ReplaceWithValue instead of this magic (and the lines below)?

Done.

https://codereview.chromium.org/2803853005/diff/240001/src/compiler/js-call-r...
File src/compiler/js-call-reducer.h (right):

https://codereview.chromium.org/2803853005/diff/240001/src/compiler/js-call-r...
src/compiler/js-call-reducer.h:9: #include "src/builtins/builtins.h"
On 2017/05/22 19:29:40, Benedikt Meurer wrote:
> We don't need this header here.

Done.

https://codereview.chromium.org/2803853005/diff/240001/src/compiler/js-native...
File src/compiler/js-native-context-specialization.h (right):

https://codereview.chromium.org/2803853005/diff/240001/src/compiler/js-native...
src/compiler/js-native-context-specialization.h:9: #include
"src/builtins/builtins.h"
On 2017/05/24 13:54:59, Michael Starzinger wrote:
> nit: Seems like a left-over. Can we drop this one?

Done.

https://codereview.chromium.org/2803853005/diff/240001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/2803853005/diff/240001/src/flag-definitions.h...
src/flag-definitions.h:805: DEFINE_BOOL(inline_array_builtins, true,
On 2017/05/24 13:54:59, Michael Starzinger wrote:
> nit: Actually a TurboFan flag. Can we name this
FLAG_turbo_inline_array_builtins
> and move it into the TurboFan section of this file?

Done.

https://codereview.chromium.org/2803853005/diff/240001/test/mjsunit/optimized...
File test/mjsunit/optimized-foreach.js (right):

https://codereview.chromium.org/2803853005/diff/240001/test/mjsunit/optimized...
test/mjsunit/optimized-foreach.js:1: // Copyright 2017 the V8 project authors.
All rights reserved.
On 2017/05/24 13:54:59, Michael Starzinger wrote:
> We should add test coverage for the following:
> - Call to a.forEach where "a" is inline allocated and potentially escape
> analyzed (i.e. by just having the array literal in the same function as the
call
> to "forEach").
> - Call to a.forEach is done inside a try-catch block and the callback function
> being called actually throws.
> - Bonus points for the above: Throw after deoptimizing the caller (i.e.
throwing
> into a lazily deoptimized caller).
> - Assuming that the below tests all inline the callback function, add a test
> where the callback function is specifically not inlined (i.e. using
> %NeverOptimizeFunction on callback function).

Done.

[Michael Starzinger, 2017-06-06 14:06:48]

Looking good. As discussed offline I found one more case where the stack trace
is off and "loses" the Array.forEach frame. Not sure how crucial this is for the
first version, I would be fine with landing without addressing this one. Your
call ...

------

(function() {
  var a = [1,2,3];
  var result = 0;
  var lazyDeopt = function() {
    var sum = function(v,i,o) {
      result += i;
      if (i == 1) {
        %DeoptimizeFunction(lazyDeopt);
        print("Throwing ..");
        throw new Error();
      }
    };
    a.forEach(sum);
  }
  assertThrows(() => lazyDeopt());
  assertThrows(() => lazyDeopt());
  try {
    lazyDeopt();
  } catch (e) {
    print("Yes, I caught (A):", e);
    print(e.stack);
  }
  %OptimizeFunctionOnNextCall(lazyDeopt);
  try {
    lazyDeopt();
  } catch (e) {
    print("Yes, I caught (B):", e);
    print(e.stack);
  }
})();

------

Throwing ..
Throwing ..
Throwing ..
Yes, I caught (A): Error
Error
    at sum (test/mjsunit/foo.js:61:15)
    at Array.forEach (<anonymous>)
    at lazyDeopt (test/mjsunit/foo.js:64:7)
    at test/mjsunit/foo.js:69:5
    at test/mjsunit/foo.js:81:3
Throwing ..
Yes, I caught (B): Error
Error
    at sum (test/mjsunit/foo.js:61:15)
    at lazyDeopt (test/mjsunit/foo.js:64:7)
    at test/mjsunit/foo.js:76:5
    at test/mjsunit/foo.js:81:3

[Michael Starzinger, 2017-06-06 14:32:54]

https://codereview.chromium.org/2803853005/diff/360001/src/compiler/code-gene...
File src/compiler/code-generator.cc (right):

https://codereview.chromium.org/2803853005/diff/360001/src/compiler/code-gene...
src/compiler/code-generator.cc:10: #include "src/callable.h"
nit: Looks like a left-over, shouldn't be needed.

https://codereview.chromium.org/2803853005/diff/360001/src/compiler/js-call-r...
File src/compiler/js-call-reducer.h (right):

https://codereview.chromium.org/2803853005/diff/360001/src/compiler/js-call-r...
src/compiler/js-call-reducer.h:24: class MachineOperatorBuilder;
nit: Looks like a left-over, shouldn't be needed.

https://codereview.chromium.org/2803853005/diff/360001/src/compiler/js-native...
File src/compiler/js-native-context-specialization.cc (right):

https://codereview.chromium.org/2803853005/diff/360001/src/compiler/js-native...
src/compiler/js-native-context-specialization.cc:12: #include
"src/compiler/frame-states.h"
nit: Can't find a reason this is needed in this file. Could it be dropped?

[danno, 2017-06-07 08:51:16]

The CQ bit was checked by danno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-07 08:51:19]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danno, 2017-06-07 09:00:56]

Michi's feedback addressed, please take another look.

https://codereview.chromium.org/2803853005/diff/360001/src/compiler/code-gene...
File src/compiler/code-generator.cc (right):

https://codereview.chromium.org/2803853005/diff/360001/src/compiler/code-gene...
src/compiler/code-generator.cc:10: #include "src/callable.h"
On 2017/06/06 14:32:54, Michael Starzinger wrote:
> nit: Looks like a left-over, shouldn't be needed.

Done.

https://codereview.chromium.org/2803853005/diff/360001/src/compiler/js-call-r...
File src/compiler/js-call-reducer.h (right):

https://codereview.chromium.org/2803853005/diff/360001/src/compiler/js-call-r...
src/compiler/js-call-reducer.h:24: class MachineOperatorBuilder;
On 2017/06/06 14:32:54, Michael Starzinger wrote:
> nit: Looks like a left-over, shouldn't be needed.

Done.

https://codereview.chromium.org/2803853005/diff/360001/src/compiler/js-native...
File src/compiler/js-native-context-specialization.cc (right):

https://codereview.chromium.org/2803853005/diff/360001/src/compiler/js-native...
src/compiler/js-native-context-specialization.cc:12: #include
"src/compiler/frame-states.h"
On 2017/06/06 14:32:54, Michael Starzinger wrote:
> nit: Can't find a reason this is needed in this file. Could it be dropped?

Done.

[commit-bot: I haz the power, 2017-06-07 09:23:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-07 09:23:04]

Dry run: This issue passed the CQ dry run.

[Michael Starzinger, 2017-06-07 09:47:04]

LGTM from my end. Just one final set of nits. Thanks!

https://codereview.chromium.org/2803853005/diff/380001/src/builtins/builtins.cc
File src/builtins/builtins.cc (right):

https://codereview.chromium.org/2803853005/diff/380001/src/builtins/builtins....
src/builtins/builtins.cc:38: UNIMPLEMENTED();
nit: s/UNIMPLEMENTED/UNREACHABLE/

https://codereview.chromium.org/2803853005/diff/380001/src/builtins/builtins....
src/builtins/builtins.cc:51: UNIMPLEMENTED();
nit: s/UNIMPLEMENTED/UNREACHABLE/

https://codereview.chromium.org/2803853005/diff/380001/src/builtins/builtins....
src/builtins/builtins.cc:150: #undef CASE
nit: s/CASE/CASE_OTHER/ in the #undef as well.

https://codereview.chromium.org/2803853005/diff/380001/src/deoptimize-reason.h
File src/deoptimize-reason.h (right):

https://codereview.chromium.org/2803853005/diff/380001/src/deoptimize-reason....
src/deoptimize-reason.h:17: V(ContinueToBuiltin, "continue to builtin")         
                      \
nit: This deopt reason seems to be unused, can we drop it?

[danno, 2017-06-07 11:23:00]

The CQ bit was checked by danno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-07 11:23:10]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-07 11:54:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-07 11:54:10]

Dry run: This issue passed the CQ dry run.

[danno, 2017-06-07 13:21:16]

The CQ bit was checked by danno@chromium.org

[danno, 2017-06-07 13:21:16]

The patchset sent to the CQ was uploaded after l-g-t-m from jarin@chromium.org,
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/2803853005/#ps420001
(title: "Disable new array builtins by default")

[commit-bot: I haz the power, 2017-06-07 13:21:23]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-07 13:23:34]

CQ is committing da patch.

Bot data: {"patchset_id": 420001, "attempt_start_ts": 1496841676147040,
"parent_rev": "cf8f7bdc9d5ee314a10fe043e421ec95604d4eca", "commit_rev":
"90c3a2d54b956d06ace2c97c6bfdb10ea9cb8e58"}

[commit-bot: I haz the power, 2017-06-07 13:23:42]

Description was changed from

==========
Inline Array.prototype.forEach in TurboFan

This CL contains a few pieces:

- A new mechanism to create "BuiltinContinuation" checkpoints in TurboFan 
  graphs, which--when triggered--swizzle the values in the the FrameState to be 
  parameters to a typically TF-generated builtin that resumes execution to
finish
  the slow-case functionality.
- Continuation builtins that have special handling in the deoptimizer and their
own 
  new frame type to ensure that the values they need to begin executing can be
stashed
  away and restored immediately before the builtin is called via a trampoline
that runs
  when the continuation builtin's frame execution resumes.
- An implementation of Array.prototype.forEach in TurboFan that can be used to 
  inline it. The inlined forEach implementation uses the checkpoints mechanism
  described above to deopt in the middle of the forEach in the cases that
optimization
  invariants are violated. There is a slightly different continuation stub for
each
  deopt point in the forEach implementation to ensure the correct side-effects,
i.e.
  that the deopt of the builtin isn't programmatically observable.
==========

to

==========
Inline Array.prototype.forEach in TurboFan

This CL contains a few pieces:

- A new mechanism to create "BuiltinContinuation" checkpoints in TurboFan 
  graphs, which--when triggered--swizzle the values in the the FrameState to be 
  parameters to a typically TF-generated builtin that resumes execution to
finish
  the slow-case functionality.
- Continuation builtins that have special handling in the deoptimizer and their
own 
  new frame type to ensure that the values they need to begin executing can be
stashed
  away and restored immediately before the builtin is called via a trampoline
that runs
  when the continuation builtin's frame execution resumes.
- An implementation of Array.prototype.forEach in TurboFan that can be used to 
  inline it. The inlined forEach implementation uses the checkpoints mechanism
  described above to deopt in the middle of the forEach in the cases that
optimization
  invariants are violated. There is a slightly different continuation stub for
each
  deopt point in the forEach implementation to ensure the correct side-effects,
i.e.
  that the deopt of the builtin isn't programmatically observable.

Review-Url: https://codereview.chromium.org/2803853005
Cr-Commit-Position: refs/heads/master@{#45764}
Committed:
https://chromium.googlesource.com/v8/v8/+/90c3a2d54b956d06ace2c97c6bfdb10ea9c...
==========

[commit-bot: I haz the power, 2017-06-07 13:23:44]

Committed patchset #22 (id:420001) as
https://chromium.googlesource.com/v8/v8/+/90c3a2d54b956d06ace2c97c6bfdb10ea9c...

[danno, 2017-06-22 14:43:54]

Feedback addressed, landed.

https://codereview.chromium.org/2803853005/diff/380001/src/builtins/builtins.cc
File src/builtins/builtins.cc (right):

https://codereview.chromium.org/2803853005/diff/380001/src/builtins/builtins....
src/builtins/builtins.cc:38: UNIMPLEMENTED();
On 2017/06/07 09:47:04, Michael Starzinger wrote:
> nit: s/UNIMPLEMENTED/UNREACHABLE/

Done.

https://codereview.chromium.org/2803853005/diff/380001/src/builtins/builtins....
src/builtins/builtins.cc:51: UNIMPLEMENTED();
On 2017/06/07 09:47:04, Michael Starzinger wrote:
> nit: s/UNIMPLEMENTED/UNREACHABLE/

Done.

https://codereview.chromium.org/2803853005/diff/380001/src/builtins/builtins....
src/builtins/builtins.cc:150: #undef CASE
On 2017/06/07 09:47:04, Michael Starzinger wrote:
> nit: s/CASE/CASE_OTHER/ in the #undef as well.

Done.

https://codereview.chromium.org/2803853005/diff/380001/src/deoptimize-reason.h
File src/deoptimize-reason.h (right):

https://codereview.chromium.org/2803853005/diff/380001/src/deoptimize-reason....
src/deoptimize-reason.h:17: V(ContinueToBuiltin, "continue to builtin")         
                      \
On 2017/06/07 09:47:04, Michael Starzinger wrote:
> nit: This deopt reason seems to be unused, can we drop it?

Done.