[Interpreter] Pass context to interpreter bytecode handlers and add LoadConstextSlot

[Interpreter] Pass context to interpreter bytecode handlers and add LoadConstextSlot

Passes the current context to bytecode interpreter handlers. This is held in the
context register on all architectures except for ia32 where there are too few
registers and it is instead spilled to the stack.

Also changes Load/StoreRegister to use kMachAnyTagged representation since they
should only ever hold tagged values.

BUG=v8:4280
LOG=N

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

[rmcilroy, 2015-08-19 10:48:40]

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

[commit-bot: I haz the power, 2015-08-19 10:48:50]

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

[rmcilroy, 2015-08-19 10:52:34]

Patchset #1 (id:1) has been deleted

[rmcilroy, 2015-08-19 10:52:38]

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

[commit-bot: I haz the power, 2015-08-19 10:52:50]

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

[commit-bot: I haz the power, 2015-08-19 11:30:09]

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

[commit-bot: I haz the power, 2015-08-19 11:30:10]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2015-08-19 11:48:42]

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

[rmcilroy, 2015-08-19 11:48:42]

Michael / Orion, PTAL, thanks. This is required for calling the Add/Sub/Div/Mod
JS builtins for the respective bytecode implementations (the next CL I'll send
out).

[Michael Starzinger, 2015-08-19 15:15:37]

High level question: Do we need to keep the context in a machine register? Or
could be but it into one of the interpreter registers thereby avoiding pinning
yet another machine register.

[rmcilroy, 2015-08-19 15:42:16]

On 2015/08/19 15:15:37, Michael Starzinger wrote:
> High level question: Do we need to keep the context in a machine register? Or
> could be but it into one of the interpreter registers thereby avoiding pinning
> yet another machine register.

I don't want to have it in an interpreter Register, otherwise the
BytecodeArrayBuilder needs to know about it, which could get messy.
 
On ia32 it is already passed as a stack slot (effectively a stack located
argument to the Bytecode handler) so it isn't using an extra register there. For
other architectures we have plenty of registers and I would prefer we have it in
the standard context register, that way we don't need to load it every time we
call another JS function.

[Michael Starzinger, 2015-08-19 16:41:52]

On 2015/08/19 15:42:16, rmcilroy wrote:
> On 2015/08/19 15:15:37, Michael Starzinger wrote:
> > High level question: Do we need to keep the context in a machine register?
Or
> > could be but it into one of the interpreter registers thereby avoiding
pinning
> > yet another machine register.
> 
> I don't want to have it in an interpreter Register, otherwise the
> BytecodeArrayBuilder needs to know about it, which could get messy.
>  
> On ia32 it is already passed as a stack slot (effectively a stack located
> argument to the Bytecode handler) so it isn't using an extra register there.
For
> other architectures we have plenty of registers and I would prefer we have it
in
> the standard context register, that way we don't need to load it every time we
> call another JS function.

I would be fine with going with such an implementation. But the higher-level
issue I wanted to raise is that it's not entirely clear to me how the context
chain should be represented in the bytecode. Going with this approach I assume
means that there is one dedicated context parameter to each bytecode handler
which most handlers will pass through only some {ContextPush} and {ContextPop}
will actually update that parameter to point to something else before doing the
next interpreter dispatch. That in turn means that the translation from bytecode
to a graph would then SSA-rename this slot when compiling from bytecode. Totally
valid approach.

An alternative approach would be, to do the renaming when building the bytecode,
put the context into an interpreter register, also put context chain extensions
into (other) interpreter registers. This would make {ContextPush} and
{ContextPop} bytecodes unnecessary and the translation from bytecode to a graph
would also look different. Not saying this approach is better, just wanted to
throw it out there.

[rmcilroy, 2015-08-20 07:57:24]

On 2015/08/19 16:41:52, Michael Starzinger wrote:
> On 2015/08/19 15:42:16, rmcilroy wrote:
> > On 2015/08/19 15:15:37, Michael Starzinger wrote:
> > > High level question: Do we need to keep the context in a machine register?
> Or
> > > could be but it into one of the interpreter registers thereby avoiding
> pinning
> > > yet another machine register.
> > 
> > I don't want to have it in an interpreter Register, otherwise the
> > BytecodeArrayBuilder needs to know about it, which could get messy.
> >  
> > On ia32 it is already passed as a stack slot (effectively a stack located
> > argument to the Bytecode handler) so it isn't using an extra register there.
> For
> > other architectures we have plenty of registers and I would prefer we have
it
> in
> > the standard context register, that way we don't need to load it every time
we
> > call another JS function.
> 
> I would be fine with going with such an implementation. But the higher-level
> issue I wanted to raise is that it's not entirely clear to me how the context
> chain should be represented in the bytecode. Going with this approach I assume
> means that there is one dedicated context parameter to each bytecode handler
> which most handlers will pass through only some {ContextPush} and {ContextPop}
> will actually update that parameter to point to something else before doing
the
> next interpreter dispatch. That in turn means that the translation from
bytecode
> to a graph would then SSA-rename this slot when compiling from bytecode.
Totally
> valid approach.
> 
> An alternative approach would be, to do the renaming when building the
bytecode,
> put the context into an interpreter register, also put context chain
extensions
> into (other) interpreter registers. This would make {ContextPush} and
> {ContextPop} bytecodes unnecessary and the translation from bytecode to a
graph
> would also look different. Not saying this approach is better, just wanted to
> throw it out there.

Ahh right, I see where your coming from now, thanks. I hadn't thought of the
approach of doing SSA renaming during bytecode generation for the context chain
(I was planning on doing the ContextPush/Pop approach you mention initially).
I'll have think about this approach and what it's implications might be for both
the bytecode, the bytecode generator and interpreter. We should probably sync up
offline to chat about this - I'll schedule something if we don't get to it in
the sub team meeting.

[Michael Starzinger, 2015-08-20 09:32:02]

LGTM. As discussed offline: I am fine with landing this as it is to unblock
interpreter work. We should just keep the alternative ideas about context chain
access in mind. This might mean that we'd potentially remove this parameter
again if we decide to take an alternative approach.

https://codereview.chromium.org/1294133004/diff/20001/src/compiler/linkage.cc
File src/compiler/linkage.cc (right):

https://codereview.chromium.org/1294133004/diff/20001/src/compiler/linkage.cc...
src/compiler/linkage.cc:416: #if defined(V8_TARGET_ARCH_IA32)
Just for posterity: Not a huge fan of this #ifdef here, but let's keep it like
it is and not overly abstract is for now, because it might change in the future
anyways.

[rmcilroy, 2015-08-24 10:03:19]

On 2015/08/20 09:32:02, Michael Starzinger wrote:
> LGTM. As discussed offline: I am fine with landing this as it is to unblock
> interpreter work. We should just keep the alternative ideas about context
chain
> access in mind. This might mean that we'd potentially remove this parameter
> again if we decide to take an alternative approach.

As discussed offline, the current plan is to do context renaming to put context
chain extensions into registers, but also maintain a current context in the cp
register (or a stack slot on ia32) to avoid the need for an extra context
operand on operations which act on the current context.

I wrote some more details in the design doc at:
https://docs.google.com/document/d/11T2CRex9hXxoJwbYqVQ32yIPMh0uouUZLdyrtmMoL...

[rmcilroy, 2015-08-24 10:03:25]

The CQ bit was checked by rmcilroy@chromium.org

[commit-bot: I haz the power, 2015-08-24 10:03:29]

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

[rmcilroy, 2015-08-24 10:14:38]

https://codereview.chromium.org/1294133004/diff/20001/src/compiler/linkage.cc
File src/compiler/linkage.cc (right):

https://codereview.chromium.org/1294133004/diff/20001/src/compiler/linkage.cc...
src/compiler/linkage.cc:416: #if defined(V8_TARGET_ARCH_IA32)
On 2015/08/20 09:32:02, Michael Starzinger wrote:
> Just for posterity: Not a huge fan of this #ifdef here, but let's keep it like
> it is and not overly abstract is for now, because it might change in the
future
> anyways.

Acknowledged.

[commit-bot: I haz the power, 2015-08-24 10:25:38]

Committed patchset #1 (id:20001)

[commit-bot: I haz the power, 2015-08-24 10:25:57]

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