[Interpreter] Ensure StackCheck bytecode handler is in split-edge form.

[Interpreter] Ensure StackCheck bytecode handler is in split-edge form.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/b9f42396e753d007131372c710b51313dd1f53da
Cr-Commit-Position: refs/heads/master@{#33785}

[rmcilroy, 2016-02-05 12:26:52]

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

[rmcilroy, 2016-02-05 12:26:53]

Michi, PTAL, thanks.

[Michael Starzinger, 2016-02-05 14:15:33]

https://codereview.chromium.org/1671653002/diff/20001/src/compiler/interprete...
File src/compiler/interpreter-assembler.cc (right):

https://codereview.chromium.org/1671653002/diff/20001/src/compiler/interprete...
src/compiler/interpreter-assembler.cc:720: raw_assembler_->Fallthrough(&ok);
Please don't add the "Fallthrough" helper to the RMA, this is very confusing.
The raw machine assembler is not in charge of maintaining edge-split form.
Similarly it is always possible to produce irreducible graphs with the RMA. IMHO
the layer above needs to maintain those invariants. Let's just model this here
as follows.

  raw_assembler_->Branch(condition, &overflow_no, &overflow_yes);
  raw_assembler_->Bind(&overflow_yes);
  CallRuntime(Runtime::kStackGuard);
  raw_assembler_->Goto(&end);
  raw_assembler_->Bind(&overflow_no);
  raw_assembler_->Goto(&end);
  raw_assembler_->Bind(&end);

The jump-threading will take care of optimizing these jumps.

[Benedikt Meurer, 2016-02-05 14:16:00]

On 2016/02/05 14:15:33, Michael Starzinger wrote:
>
https://codereview.chromium.org/1671653002/diff/20001/src/compiler/interprete...
> File src/compiler/interpreter-assembler.cc (right):
> 
>
https://codereview.chromium.org/1671653002/diff/20001/src/compiler/interprete...
> src/compiler/interpreter-assembler.cc:720: raw_assembler_->Fallthrough(&ok);
> Please don't add the "Fallthrough" helper to the RMA, this is very confusing.
> The raw machine assembler is not in charge of maintaining edge-split form.
> Similarly it is always possible to produce irreducible graphs with the RMA.
IMHO
> the layer above needs to maintain those invariants. Let's just model this here
> as follows.
> 
>   raw_assembler_->Branch(condition, &overflow_no, &overflow_yes);
>   raw_assembler_->Bind(&overflow_yes);
>   CallRuntime(Runtime::kStackGuard);
>   raw_assembler_->Goto(&end);
>   raw_assembler_->Bind(&overflow_no);
>   raw_assembler_->Goto(&end);
>   raw_assembler_->Bind(&end);
> 
> The jump-threading will take care of optimizing these jumps.

+1

[rmcilroy, 2016-02-05 14:33:48]

Description was changed from

==========
[Interpreter] Replace Goto with Fallthrough in StackCheck.

Ensure that we are in split-edge form by replacing the dummy Goto in
the StackCheck handler with a Fallthrough instead.

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

to

==========
[Interpreter] Ensure StackCheck bytecode handler is in split-edge form.

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

[rmcilroy, 2016-02-05 14:34:36]

https://codereview.chromium.org/1671653002/diff/20001/src/compiler/interprete...
File src/compiler/interpreter-assembler.cc (right):

https://codereview.chromium.org/1671653002/diff/20001/src/compiler/interprete...
src/compiler/interpreter-assembler.cc:720: raw_assembler_->Fallthrough(&ok);
On 2016/02/05 14:15:33, Michael Starzinger wrote:
> Please don't add the "Fallthrough" helper to the RMA, this is very confusing.
> The raw machine assembler is not in charge of maintaining edge-split form.
> Similarly it is always possible to produce irreducible graphs with the RMA.
IMHO
> the layer above needs to maintain those invariants. Let's just model this here
> as follows.
> 
>   raw_assembler_->Branch(condition, &overflow_no, &overflow_yes);
>   raw_assembler_->Bind(&overflow_yes);
>   CallRuntime(Runtime::kStackGuard);
>   raw_assembler_->Goto(&end);
>   raw_assembler_->Bind(&overflow_no);
>   raw_assembler_->Goto(&end);
>   raw_assembler_->Bind(&end);
> 
> The jump-threading will take care of optimizing these jumps.

Done.

[Michael Starzinger, 2016-02-05 14:35:43]

LGTM. Thanks!

[rmcilroy, 2016-02-05 14:39:04]

The CQ bit was checked by rmcilroy@chromium.org

[commit-bot: I haz the power, 2016-02-05 14:39:10]

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

[commit-bot: I haz the power, 2016-02-05 15:10:03]

Description was changed from

==========
[Interpreter] Ensure StackCheck bytecode handler is in split-edge form.

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

to

==========
[Interpreter] Ensure StackCheck bytecode handler is in split-edge form.

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

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

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-02-05 15:10:51]

Description was changed from

==========
[Interpreter] Ensure StackCheck bytecode handler is in split-edge form.

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

to

==========
[Interpreter] Ensure StackCheck bytecode handler is in split-edge form.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/b9f42396e753d007131372c710b51313dd1f53da
Cr-Commit-Position: refs/heads/master@{#33785}
==========

[commit-bot: I haz the power, 2016-02-05 15:10:52]

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