[Interpreter] Add explicit StackCheck bytecodes on function entry and back branches.

[Interpreter] Add explicit StackCheck bytecodes on function entry and back branches.

Moves the stack check from the function entry trampoline to instead be
after function activation using an explicit StackCheck bytecode. Also
add stack checks on back edges of loops.

BUG=v8:4280, v8:4678
LOG=N

Committed: https://crrev.com/1ce720f2a481cf07159eb97d3c48a707fddaf08c
Cr-Commit-Position: refs/heads/master@{#33730}

[rmcilroy, 2016-02-03 15:45:22]

rmcilroy@chromium.org changed reviewers:
+ mstarzinger@chromium.org, oth@chromium.org

[rmcilroy, 2016-02-03 15:45:22]

This is still WIP (need to fix a load of tests yet and port to other
architectures), but I just want to check whether this was what you had in mind
for stack check operations in Ignition. 

Michi / Orion: does this look like a reasonable approach?

[Michael Starzinger, 2016-02-03 16:13:18]

Yes, approach looks good!

https://codereview.chromium.org/1665853002/diff/1/src/compiler/bytecode-graph...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1665853002/diff/1/src/compiler/bytecode-graph...
src/compiler/bytecode-graph-builder.cc:564: void
BytecodeGraphBuilder::CreateGraphBody() {
I think we can get rid of the CreateGraphBody method completely, it should
coincide with VisitBytecodes AFAICT. Also, everything in the TODO is already
done as part of the bytecode stream.

https://codereview.chromium.org/1665853002/diff/1/src/compiler/interpreter-as...
File src/compiler/interpreter-assembler.cc (right):

https://codereview.chromium.org/1665853002/diff/1/src/compiler/interpreter-as...
src/compiler/interpreter-assembler.cc:714: Node* condition =
raw_assembler_->IntPtrGreaterThanOrEqual(sp, stack_limit);
Not entirely sure about what code is produced here, but this needs to be an
unsigned comparison. So I think we might need to add a "uint" class of methods
to the raw machine assembler, the machine operators themselves already exist of
course.

[oth, 2016-02-03 16:35:14]

lgtm, modulo Michi's comments and the mechanical test updates.

[rmcilroy, 2016-02-04 11:51:39]

Patchset #2 (id:20001) has been deleted

[rmcilroy, 2016-02-04 11:51:59]

Description was changed from

==========
[Interpreter] Add explicit StackCheck bytecodes on function entry and back
branches.

Moves the stack check from the function entry trampoline to instead be
after function activation using an explicit StackCheck bytecode. Also
add stack checks on back edges of loops.

BUG=v8:4280
LOG=N
==========

to

==========
[Interpreter] Add explicit StackCheck bytecodes on function entry and back
branches.

Moves the stack check from the function entry trampoline to instead be
after function activation using an explicit StackCheck bytecode. Also
add stack checks on back edges of loops.

BUG=v8:4280,v8:4678
LOG=N
==========

[rmcilroy, 2016-02-04 11:54:58]

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

[commit-bot: I haz the power, 2016-02-04 11:55:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1665853002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1665853002/40001

[rmcilroy, 2016-02-04 11:55:15]

Comments addressed and tests fixed, PTAL, thanks.

https://codereview.chromium.org/1665853002/diff/1/src/compiler/bytecode-graph...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1665853002/diff/1/src/compiler/bytecode-graph...
src/compiler/bytecode-graph-builder.cc:564: void
BytecodeGraphBuilder::CreateGraphBody() {
On 2016/02/03 16:13:17, Michael Starzinger wrote:
> I think we can get rid of the CreateGraphBody method completely, it should
> coincide with VisitBytecodes AFAICT. Also, everything in the TODO is already
> done as part of the bytecode stream.

Done.

https://codereview.chromium.org/1665853002/diff/1/src/compiler/interpreter-as...
File src/compiler/interpreter-assembler.cc (right):

https://codereview.chromium.org/1665853002/diff/1/src/compiler/interpreter-as...
src/compiler/interpreter-assembler.cc:714: Node* condition =
raw_assembler_->IntPtrGreaterThanOrEqual(sp, stack_limit);
On 2016/02/03 16:13:18, Michael Starzinger wrote:
> Not entirely sure about what code is produced here, but this needs to be an
> unsigned comparison. So I think we might need to add a "uint" class of methods
> to the raw machine assembler, the machine operators themselves already exist
of
> course.

Good catch. Done.

[rmcilroy, 2016-02-04 11:56:45]

+ssanfilippo FYI

[Michael Starzinger, 2016-02-04 12:07:36]

LGTM. Nice! Didn't look at test-bytecode-generator.cc all too much.

[rmcilroy, 2016-02-04 12:09:13]

On 2016/02/04 12:07:36, Michael Starzinger wrote:
> Didn't look at test-bytecode-generator.cc all too much.

I don't blame you... :)

[commit-bot: I haz the power, 2016-02-04 12:10:01]

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

[commit-bot: I haz the power, 2016-02-04 12:10:02]

Dry run: Try jobs failed on following builders:
  v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...)

[rmcilroy, 2016-02-04 12:13:26]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2016-02-04 12:13:27]

The patchset sent to the CQ was uploaded after l-g-t-m from oth@chromium.org,
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1665853002/#ps60001
(title: "Fix unittests.")

[commit-bot: I haz the power, 2016-02-04 12:13:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1665853002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1665853002/60001

[commit-bot: I haz the power, 2016-02-04 12:33:17]

Description was changed from

==========
[Interpreter] Add explicit StackCheck bytecodes on function entry and back
branches.

Moves the stack check from the function entry trampoline to instead be
after function activation using an explicit StackCheck bytecode. Also
add stack checks on back edges of loops.

BUG=v8:4280,v8:4678
LOG=N
==========

to

==========
[Interpreter] Add explicit StackCheck bytecodes on function entry and back
branches.

Moves the stack check from the function entry trampoline to instead be
after function activation using an explicit StackCheck bytecode. Also
add stack checks on back edges of loops.

BUG=v8:4280,v8:4678
LOG=N
==========

[commit-bot: I haz the power, 2016-02-04 12:33:18]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-02-04 12:33:56]

Description was changed from

==========
[Interpreter] Add explicit StackCheck bytecodes on function entry and back
branches.

Moves the stack check from the function entry trampoline to instead be
after function activation using an explicit StackCheck bytecode. Also
add stack checks on back edges of loops.

BUG=v8:4280,v8:4678
LOG=N
==========

to

==========
[Interpreter] Add explicit StackCheck bytecodes on function entry and back
branches.

Moves the stack check from the function entry trampoline to instead be
after function activation using an explicit StackCheck bytecode. Also
add stack checks on back edges of loops.

BUG=v8:4280,v8:4678
LOG=N

Committed: https://crrev.com/1ce720f2a481cf07159eb97d3c48a707fddaf08c
Cr-Commit-Position: refs/heads/master@{#33730}
==========

[commit-bot: I haz the power, 2016-02-04 12:33:57]

Patchset 3 (id:??) landed as
https://crrev.com/1ce720f2a481cf07159eb97d3c48a707fddaf08c
Cr-Commit-Position: refs/heads/master@{#33730}

[Michael Achenbach, 2016-02-04 13:12:43]

machenbach@chromium.org changed reviewers:
+ machenbach@chromium.org

[Michael Achenbach, 2016-02-04 13:12:44]

CC mtrofin for the greedy allocator failures.

[Mircea Trofin, 2016-02-04 18:39:07]

On 2016/02/04 13:12:44, Michael Achenbach wrote:
> CC mtrofin for the greedy allocator failures.

It appears this CL invalidates split-edge form, and that greedy simply
highlighted that by happening to split in such a way that ResolveControlFlow
uncovered the issue.

I created a CL, 1668953002, which passes before this change, and fails after
(the full build actually fails, as part of a post-binaries build script run)

[rmcilroy, 2016-02-05 12:28:43]

On 2016/02/04 18:39:07, Mircea Trofin wrote:
> On 2016/02/04 13:12:44, Michael Achenbach wrote:
> > CC mtrofin for the greedy allocator failures.
> 
> It appears this CL invalidates split-edge form, and that greedy simply
> highlighted that by happening to split in such a way that ResolveControlFlow
> uncovered the issue.
> 
> I created a CL, 1668953002, which passes before this change, and fails after
> (the full build actually fails, as part of a post-binaries build script run)

Apologies for breaking greedy allocator with this, thanks for uncovering the
issue. I have a fix in https://codereview.chromium.org/1671653002/.

[Mircea Trofin, 2016-02-05 16:13:29]

On 2016/02/05 12:28:43, rmcilroy wrote:
> On 2016/02/04 18:39:07, Mircea Trofin wrote:
> > On 2016/02/04 13:12:44, Michael Achenbach wrote:
> > > CC mtrofin for the greedy allocator failures.
> > 
> > It appears this CL invalidates split-edge form, and that greedy simply
> > highlighted that by happening to split in such a way that ResolveControlFlow
> > uncovered the issue.
> > 
> > I created a CL, 1668953002, which passes before this change, and fails after
> > (the full build actually fails, as part of a post-binaries build script run)
> 
> Apologies for breaking greedy allocator with this, thanks for uncovering the
> issue. I have a fix in https://codereview.chromium.org/1671653002/.

To be clear, you didn't break the greedy allocator. The split-edge form
assumption 
is throughout the register allocation pipeline (linear scan or greedy). It just
so happened 
that, for the tests we have, greedy stumbled upon it and not linear scan.

[rmcilroy, 2016-02-05 16:42:04]

On 2016/02/05 16:13:29, Mircea Trofin wrote:
> On 2016/02/05 12:28:43, rmcilroy wrote:
> > On 2016/02/04 18:39:07, Mircea Trofin wrote:
> > > On 2016/02/04 13:12:44, Michael Achenbach wrote:
> > > > CC mtrofin for the greedy allocator failures.
> > > 
> > > It appears this CL invalidates split-edge form, and that greedy simply
> > > highlighted that by happening to split in such a way that
ResolveControlFlow
> > > uncovered the issue.
> > > 
> > > I created a CL, 1668953002, which passes before this change, and fails
after
> > > (the full build actually fails, as part of a post-binaries build script
run)
> > 
> > Apologies for breaking greedy allocator with this, thanks for uncovering the
> > issue. I have a fix in https://codereview.chromium.org/1671653002/.
> 
> To be clear, you didn't break the greedy allocator. The split-edge form
> assumption 
> is throughout the register allocation pipeline (linear scan or greedy). It
just
> so happened 
> that, for the tests we have, greedy stumbled upon it and not linear scan.

Yes I understood that - I meant I broke the greedy bot.