[interpreter] Add basic framework for bytecode handler code generation.

[interpreter] Add basic framework for bytecode handler code generation.

Adds basic support for generation of interpreter bytecode handler code
snippets. The InterpreterAssembler class exposes a set of low level,
interpreter specific operations which can be used to build a Turbofan
graph. The Interpreter class generates a bytecode handler snippet for
each bytecode by assembling operations using an InterpreterAssembler.

Currently only two simple bytecodes are supported: LoadLiteral0 and Return.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/7877c4e0c77b5c2b97678406eab7e9ad6eba4a4d
Cr-Commit-Position: refs/heads/master@{#29814}

[rmcilroy, 2015-07-15 10:26:07]

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

[oth, 2015-07-15 13:22:01]

LGTM. Trivial comments, but overall this looks great.

https://codereview.chromium.org/1239793002/diff/1/src/compiler/linkage-impl.h
File src/compiler/linkage-impl.h (right):

https://codereview.chromium.org/1239793002/diff/1/src/compiler/linkage-impl.h...
src/compiler/linkage-impl.h:243: DCHECK_EQ(0,
InterpreterAssembler::kBytecodePointerParameter);
One line comment explaining types purpose (reserving registers).

https://codereview.chromium.org/1239793002/diff/1/src/compiler/linkage.h
File src/compiler/linkage.h (right):

https://codereview.chromium.org/1239793002/diff/1/src/compiler/linkage.h#newc...
src/compiler/linkage.h:245: bool needs_return);
Suggest a comment on needs_return or a more indicative name (e.g.
function_returns_value).

https://codereview.chromium.org/1239793002/diff/1/src/compiler/raw-machine-as...
File src/compiler/raw-machine-assembler.h (right):

https://codereview.chromium.org/1239793002/diff/1/src/compiler/raw-machine-as...
src/compiler/raw-machine-assembler.h:54: CallDescriptor* call_descriptor);
The two constructors for RawMachineAssembler could be rolled into one, but
adding a default parameter CallDescriptor* call_descriptor = nullptr to the
existing constructor.

[rmcilroy, 2015-07-15 16:55:57]

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

[rmcilroy, 2015-07-15 16:55:58]

Thanks Orion.

Benedikt / Michael: Could you take a look please. Let me know if you would
prefer it split this up further, but I think it makes sense to review as is. If
you are happy with the general direction I will add other architectures and add
unittests to test the behavior of the generated bytecode handlers are as
expected.

https://codereview.chromium.org/1239793002/diff/1/src/compiler/linkage-impl.h
File src/compiler/linkage-impl.h (right):

https://codereview.chromium.org/1239793002/diff/1/src/compiler/linkage-impl.h...
src/compiler/linkage-impl.h:243: DCHECK_EQ(0,
InterpreterAssembler::kBytecodePointerParameter);
On 2015/07/15 13:22:01, oth wrote:
> One line comment explaining types purpose (reserving registers).

Done.

https://codereview.chromium.org/1239793002/diff/1/src/compiler/linkage.h
File src/compiler/linkage.h (right):

https://codereview.chromium.org/1239793002/diff/1/src/compiler/linkage.h#newc...
src/compiler/linkage.h:245: bool needs_return);
On 2015/07/15 13:22:01, oth wrote:
> Suggest a comment on needs_return or a more indicative name (e.g.
> function_returns_value).

Added a comment and changed the name to will_return.

https://codereview.chromium.org/1239793002/diff/1/src/compiler/raw-machine-as...
File src/compiler/raw-machine-assembler.h (right):

https://codereview.chromium.org/1239793002/diff/1/src/compiler/raw-machine-as...
src/compiler/raw-machine-assembler.h:54: CallDescriptor* call_descriptor);
On 2015/07/15 13:22:01, oth wrote:
> The two constructors for RawMachineAssembler could be rolled into one, but
> adding a default parameter CallDescriptor* call_descriptor = nullptr to the
> existing constructor.
> 

The existing constructor passes Linkage::GetSimplifiedCDescriptor(graph->zone(),
machine_sig) to call_descriptor, so a nullptr default wouldn't really work the
same way unfortunately (unless I explicitly checked for nullptr in the
constructor, but that seems nasty).

However, I agree it would be nice to avoid the double constructors, so I've
refactored the existing RawMachineAssembler to explicitly pass the correct
CallDescriptor instead. I moved this to a seperate CL at
https://codereview.chromium.org/1235313002, let me know what you think?

[rmcilroy, 2015-07-15 16:59:36]

p.s., I've updated the design in the Ignition design doc [1] to reflect the
change to a register based design. Feel free to add comments there and happy to
discuss anything over VC if you have questions / concerns.


[1]
https://docs.google.com/document/d/11T2CRex9hXxoJwbYqVQ32yIPMh0uouUZLdyrtmMoL...

[picksi, 2015-07-16 09:08:09]

picksi@google.com changed reviewers:
+ picksi@google.com

[picksi, 2015-07-16 09:08:10]

https://codereview.chromium.org/1239793002/diff/20001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1239793002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:29: static_cast<int>(Bytecode::kLast) + 1,
TENURED);
It's probably macro magic, but Bytecode::kLast is set to -1 in the enum, and I
don't see it being changed later... Should it just be declared in the enum
without being set to -1?

[rmcilroy, 2015-07-16 15:11:10]

https://codereview.chromium.org/1239793002/diff/20001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1239793002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:29: static_cast<int>(Bytecode::kLast) + 1,
TENURED);
On 2015/07/16 09:08:10, picksi wrote:
> It's probably macro magic, but Bytecode::kLast is set to -1 in the enum, and I
> don't see it being changed later... Should it just be declared in the enum
> without being set to -1?

This is macro magic ;). It is assigned -1 but then the following two lines:
#define COUNT_BYTECODE(x, _) +1
   BYTECODE_LIST(COUNT_BYTECODE)

are part of the same expression and turn the expressions after post processing
into:
kLast = -1 +1 +1 +1... etc.

I agree this is pretty horrid, however if I just put another entry at the end of
the enum (e.g., kCount) then that will be a new enum value which will need to be
handled specially in any switch statements.  Right now since kLast overlaps with
the last bytecode it doesn't require an extra "case" in switch statements, which
makes the switch code cleaner I think. If you would still prefer to avoid this
then I can change it.

[picksi, 2015-07-16 17:06:27]

That is very clever (and a little horrid!). Can we add a comment to explain how
kLast ends up being correctly generated, or are verbose comments frowned upon in
V8?

[rmcilroy, 2015-07-17 09:26:02]

Michael / Benedikt: friendly ping?


On 2015/07/16 17:06:27, picksi wrote:
> That is very clever (and a little horrid!). Can we add a comment to explain
how
> kLast ends up being correctly generated, or are verbose comments frowned upon
in
> V8?

Yes, this probably deserves a comment. Added a comment and also move around the
code to make it more clear that the count macro affects kLast.

[picksi, 2015-07-17 09:32:26]

Thanks! That is much clearer now.

[Michael Starzinger, 2015-07-17 13:20:59]

https://codereview.chromium.org/1239793002/diff/40001/src/compiler/interprete...
File src/compiler/interpreter-assembler.h (right):

https://codereview.chromium.org/1239793002/diff/40001/src/compiler/interprete...
src/compiler/interpreter-assembler.h:8: #include "src/interpreter/bytecodes.h"
Can we add the same comment that is in pipeline.h to this file as well?

// Clients of this interface shouldn't depend on lots of compiler internals.
// Do not include anything from src/compiler here!

https://codereview.chromium.org/1239793002/diff/40001/src/compiler/interprete...
src/compiler/interpreter-assembler.h:30: InterpreterAssembler(Isolate* isolate,
Zone* zone,
Do you think it makes sense to add unit tests for the InterpreterAssembler? I am
mostly thinking about unit tests for the builder functions (e.g. LoadRegister,
StoreRegister and friends), not the code generation itself.

If you agree that having unit tests has a benefit, then I would be fine with
just having one or two of them for now, so that we at least have the unit test
file ready for the tricky functions that we actually want to test. For TurboFan
unit tests we are not looking for full coverage, just for covering the thing
that are crucial and have potential to cause breakage while refactoring.

https://codereview.chromium.org/1239793002/diff/40001/src/compiler/linkage-im...
File src/compiler/linkage-impl.h (right):

https://codereview.chromium.org/1239793002/diff/40001/src/compiler/linkage-im...
src/compiler/linkage-impl.h:244: DCHECK_EQ(0,
InterpreterAssembler::kBytecodePointerParameter);
Would it make sense to move these constant (i.e. kBytecodePointerParameter an
friends) into linkage? Then we wouldn't need to include the
interpreter-assembler.h file here, but turn it around. I consider linkage.h to
be at a lower conceptual level compared to the interpreter-assembler.h file.

https://codereview.chromium.org/1239793002/diff/40001/src/compiler/linkage-im...
src/compiler/linkage-impl.h:252: if (will_return) {
The boolean "will_return" flag here makes me nervous, I need to think more about
this one. Benedikt might have opinions about that as well.

https://codereview.chromium.org/1239793002/diff/40001/src/compiler/raw-machin...
File src/compiler/raw-machine-assembler.h (right):

https://codereview.chromium.org/1239793002/diff/40001/src/compiler/raw-machin...
src/compiler/raw-machine-assembler.h:8: #include "src/compiler.h"
Do we need this include here? This smells to include a lot.

https://codereview.chromium.org/1239793002/diff/40001/src/interpreter/DEPS
File src/interpreter/DEPS (right):

https://codereview.chromium.org/1239793002/diff/40001/src/interpreter/DEPS#ne...
src/interpreter/DEPS:2: "+src/compiler/interpreter-assembler.h",
I would be fine with allowing inclusion of interpreter-assembler.h globally
(i.e. moving this line to src/DEPS). Leaving it here is also fine. Up to you.

https://codereview.chromium.org/1239793002/diff/40001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/1239793002/diff/40001/src/isolate.cc#newcode2120
src/isolate.cc:2120: interpreter_ = new interpreter::Interpreter();
Probably needs the corresponding "delete" in Isolate::Deinit as well.

https://codereview.chromium.org/1239793002/diff/40001/test/cctest/cctest.gyp
File test/cctest/cctest.gyp (right):

https://codereview.chromium.org/1239793002/diff/40001/test/cctest/cctest.gyp#...
test/cctest/cctest.gyp:87: 'interpreter/test-interpreter.cc',
These files don't seem to be in the CL. I think it would make sense to add tests
early. Do you want to add the tests to this CL or did you intend them to be in a
follow-up?

[rmcilroy, 2015-07-21 11:07:15]

Patchset #4 (id:60001) has been deleted

[rmcilroy, 2015-07-21 11:12:55]

Patchset #4 (id:80001) has been deleted

[rmcilroy, 2015-07-21 11:13:03]

Patchset #4 (id:100001) has been deleted

[rmcilroy, 2015-07-21 11:13:21]

Comments addressed, PTAL.

https://codereview.chromium.org/1239793002/diff/40001/src/compiler/interprete...
File src/compiler/interpreter-assembler.h (right):

https://codereview.chromium.org/1239793002/diff/40001/src/compiler/interprete...
src/compiler/interpreter-assembler.h:8: #include "src/interpreter/bytecodes.h"
On 2015/07/17 13:20:58, Michael Starzinger wrote:
> Can we add the same comment that is in pipeline.h to this file as well?
> 
> // Clients of this interface shouldn't depend on lots of compiler internals.
> // Do not include anything from src/compiler here!

Done. Also added similar comments for src/interpreter/interpreter.h and
src/interpreter/bytecodes.h since they should be the only entrypoints into the
interpreter for now.

https://codereview.chromium.org/1239793002/diff/40001/src/compiler/interprete...
src/compiler/interpreter-assembler.h:30: InterpreterAssembler(Isolate* isolate,
Zone* zone,
On 2015/07/17 13:20:58, Michael Starzinger wrote:
> Do you think it makes sense to add unit tests for the InterpreterAssembler? I
am
> mostly thinking about unit tests for the builder functions (e.g. LoadRegister,
> StoreRegister and friends), not the code generation itself.
> 
> If you agree that having unit tests has a benefit, then I would be fine with
> just having one or two of them for now, so that we at least have the unit test
> file ready for the tricky functions that we actually want to test. For
TurboFan
> unit tests we are not looking for full coverage, just for covering the thing
> that are crucial and have potential to cause breakage while refactoring.

Yes this probably makes sense, although I'm not exactly sure what I can test for
the LoadRegister/StoreRegister etc. type functions, but I'll have a go and add
this for the next patch set if it works.

https://codereview.chromium.org/1239793002/diff/40001/src/compiler/linkage-im...
File src/compiler/linkage-impl.h (right):

https://codereview.chromium.org/1239793002/diff/40001/src/compiler/linkage-im...
src/compiler/linkage-impl.h:244: DCHECK_EQ(0,
InterpreterAssembler::kBytecodePointerParameter);
On 2015/07/17 13:20:59, Michael Starzinger wrote:
> Would it make sense to move these constant (i.e. kBytecodePointerParameter an
> friends) into linkage? Then we wouldn't need to include the
> interpreter-assembler.h file here, but turn it around. I consider linkage.h to
> be at a lower conceptual level compared to the interpreter-assembler.h file.

Works for me. Done.

https://codereview.chromium.org/1239793002/diff/40001/src/compiler/linkage-im...
src/compiler/linkage-impl.h:252: if (will_return) {
On 2015/07/17 13:20:58, Michael Starzinger wrote:
> The boolean "will_return" flag here makes me nervous, I need to think more
about
> this one. Benedikt might have opinions about that as well.

Based on a suggestion from @danno I reworked this such that the return bytecode
will jump to an InterpreterExitTrampoline to avoid having to do an explicit
return here. The builtins are still (very much) WIP but you can get an idea of
how this will work in https://codereview.chromium.org/1245133002/).

Is this better?

https://codereview.chromium.org/1239793002/diff/40001/src/compiler/raw-machin...
File src/compiler/raw-machine-assembler.h (right):

https://codereview.chromium.org/1239793002/diff/40001/src/compiler/raw-machin...
src/compiler/raw-machine-assembler.h:8: #include "src/compiler.h"
On 2015/07/17 13:20:59, Michael Starzinger wrote:
> Do we need this include here? This smells to include a lot.

The reason I did this is that raw_machine_assembler.h doesn't "self host" all
it's needed header includes (raw_machine_assembler.cc didn't have the header at
the top of it's includes) and so relies on some other things which are included
by src/compiler/pipeline.h. It turns out all it needs is ExternalReference which
is defined in assembler.h, so I've changed this to be src/assembler.h.

https://codereview.chromium.org/1239793002/diff/40001/src/interpreter/DEPS
File src/interpreter/DEPS (right):

https://codereview.chromium.org/1239793002/diff/40001/src/interpreter/DEPS#ne...
src/interpreter/DEPS:2: "+src/compiler/interpreter-assembler.h",
On 2015/07/17 13:20:59, Michael Starzinger wrote:
> I would be fine with allowing inclusion of interpreter-assembler.h globally
> (i.e. moving this line to src/DEPS). Leaving it here is also fine. Up to you.

Let's leave it here for now and move it globally when/if required.

https://codereview.chromium.org/1239793002/diff/40001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/1239793002/diff/40001/src/isolate.cc#newcode2120
src/isolate.cc:2120: interpreter_ = new interpreter::Interpreter();
On 2015/07/17 13:20:59, Michael Starzinger wrote:
> Probably needs the corresponding "delete" in Isolate::Deinit as well.

Done.

https://codereview.chromium.org/1239793002/diff/40001/test/cctest/cctest.gyp
File test/cctest/cctest.gyp (right):

https://codereview.chromium.org/1239793002/diff/40001/test/cctest/cctest.gyp#...
test/cctest/cctest.gyp:87: 'interpreter/test-interpreter.cc',
On 2015/07/17 13:20:59, Michael Starzinger wrote:
> These files don't seem to be in the CL. I think it would make sense to add
tests
> early. Do you want to add the tests to this CL or did you intend them to be in
a
> follow-up?

Oops, this was a mistake. I'm was still working on these tests, but you can see
the WIP at https://codereview.chromium.org/1243673003/

[Michael Starzinger, 2015-07-21 13:51:45]

LGTM from my end. Please also wait on sign-off from Benedikt on this one.

https://codereview.chromium.org/1239793002/diff/40001/src/compiler/linkage-im...
File src/compiler/linkage-impl.h (right):

https://codereview.chromium.org/1239793002/diff/40001/src/compiler/linkage-im...
src/compiler/linkage-impl.h:252: if (will_return) {
On 2015/07/21 11:13:21, rmcilroy wrote:
> On 2015/07/17 13:20:58, Michael Starzinger wrote:
> > The boolean "will_return" flag here makes me nervous, I need to think more
> about
> > this one. Benedikt might have opinions about that as well.
> 
> Based on a suggestion from @danno I reworked this such that the return
bytecode
> will jump to an InterpreterExitTrampoline to avoid having to do an explicit
> return here. The builtins are still (very much) WIP but you can get an idea of
> how this will work in https://codereview.chromium.org/1245133002/).
> 
> Is this better?

Acknowledged. Yep, much better, I like.

https://codereview.chromium.org/1239793002/diff/120001/src/compiler/interpret...
File src/compiler/interpreter-assembler.h (right):

https://codereview.chromium.org/1239793002/diff/120001/src/compiler/interpret...
src/compiler/interpreter-assembler.h:84: Zone* zone_;  // Not owned.
nit: Not sure what the "Not owned" comment tries to convey, can we drop it?

https://codereview.chromium.org/1239793002/diff/120001/src/compiler/linkage-i...
File src/compiler/linkage-impl.h (right):

https://codereview.chromium.org/1239793002/diff/120001/src/compiler/linkage-i...
src/compiler/linkage-impl.h:241: DCHECK_EQ(0,
Linkage::kInterpreterBytecodeParameter);
nit: Can this be made a STATIC_ASSERT?

https://codereview.chromium.org/1239793002/diff/120001/src/compiler/linkage-i...
src/compiler/linkage-impl.h:245: DCHECK_EQ(1,
Linkage::kInterpreterDispatchTableParameter);
nit: Can this be made a STATIC_ASSERT?

https://codereview.chromium.org/1239793002/diff/120001/src/compiler/linkage.h
File src/compiler/linkage.h (right):

https://codereview.chromium.org/1239793002/diff/120001/src/compiler/linkage.h...
src/compiler/linkage.h:282: // Special parameters used to pass fixed register
data through interpreter
nit: s/parameters/parameter indices/

https://codereview.chromium.org/1239793002/diff/120001/src/interpreter/byteco...
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/1239793002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecodes.h:46: Bytecodes() { UNREACHABLE(); }
nit: Better use DISALLOW_IMPLICIT_CONSTRUCTORS(Bytecodes) here.

[rmcilroy, 2015-07-23 10:43:08]

Patchset #6 (id:160001) has been deleted

[rmcilroy, 2015-07-23 10:43:54]

Added some unittests, if you could take a last quick look at the tests, thanks.

https://codereview.chromium.org/1239793002/diff/120001/src/compiler/interpret...
File src/compiler/interpreter-assembler.h (right):

https://codereview.chromium.org/1239793002/diff/120001/src/compiler/interpret...
src/compiler/interpreter-assembler.h:84: Zone* zone_;  // Not owned.
On 2015/07/21 13:51:45, Michael Starzinger wrote:
> nit: Not sure what the "Not owned" comment tries to convey, can we drop it?

This is quite common in Chromium code [1] to identify that the object pointed to
by the pointer is not owned by this class and so shouldn't be deleted in the
destructor. It doesn't seem as common in V8 though so 'll remove it.

[1]
https://code.google.com/p/chromium/codesearch#search/&q=%22Not%20owned%22&sq=...

https://codereview.chromium.org/1239793002/diff/120001/src/compiler/linkage-i...
File src/compiler/linkage-impl.h (right):

https://codereview.chromium.org/1239793002/diff/120001/src/compiler/linkage-i...
src/compiler/linkage-impl.h:241: DCHECK_EQ(0,
Linkage::kInterpreterBytecodeParameter);
On 2015/07/21 13:51:45, Michael Starzinger wrote:
> nit: Can this be made a STATIC_ASSERT?

Done.

https://codereview.chromium.org/1239793002/diff/120001/src/compiler/linkage-i...
src/compiler/linkage-impl.h:245: DCHECK_EQ(1,
Linkage::kInterpreterDispatchTableParameter);
On 2015/07/21 13:51:45, Michael Starzinger wrote:
> nit: Can this be made a STATIC_ASSERT?

Done (also in interpreter-assembler.cc).

https://codereview.chromium.org/1239793002/diff/120001/src/compiler/linkage.h
File src/compiler/linkage.h (right):

https://codereview.chromium.org/1239793002/diff/120001/src/compiler/linkage.h...
src/compiler/linkage.h:282: // Special parameters used to pass fixed register
data through interpreter
On 2015/07/21 13:51:45, Michael Starzinger wrote:
> nit: s/parameters/parameter indices/

Done.

https://codereview.chromium.org/1239793002/diff/120001/src/interpreter/byteco...
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/1239793002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecodes.h:46: Bytecodes() { UNREACHABLE(); }
On 2015/07/21 13:51:45, Michael Starzinger wrote:
> nit: Better use DISALLOW_IMPLICIT_CONSTRUCTORS(Bytecodes) here.

Done.

[Michael Starzinger, 2015-07-23 11:05:04]

LGTM on the unit tests as well.

https://codereview.chromium.org/1239793002/diff/180001/test/unittests/compile...
File test/unittests/compiler/interpreter-assembler-unittest.h (right):

https://codereview.chromium.org/1239793002/diff/180001/test/unittests/compile...
test/unittests/compiler/interpreter-assembler-unittest.h:42: CallDescriptor*
call_descriptor() const {
nit: Could we use "using InterpreterAssembler::call_descriptor;" here instead?

https://codereview.chromium.org/1239793002/diff/180001/test/unittests/compile...
test/unittests/compiler/interpreter-assembler-unittest.h:45: Graph* graph() {
return InterpreterAssembler::graph(); }
nit: Likewise.

https://codereview.chromium.org/1239793002/diff/180001/test/unittests/compile...
File test/unittests/compiler/node-test-utils.cc (right):

https://codereview.chromium.org/1239793002/diff/180001/test/unittests/compile...
test/unittests/compiler/node-test-utils.cc:1892: Matcher<Node*>
IsIntPtr##Name(const Matcher<Node*>& lhs_matcher,             \
I would be fine with having these helper based on kPointerSize, but I think
Benedikt doesn't particularly like those and tried to avoid them. Would it be
possible to move them into the interpreter-assembler-unittest.[h/cc] file until
we have time to run this by Benedikt?

[rmcilroy, 2015-07-23 11:55:02]

Patchset #8 (id:220001) has been deleted

[rmcilroy, 2015-07-23 11:57:32]

Patchset #8 (id:240001) has been deleted

[rmcilroy, 2015-07-23 11:58:54]

Thanks for the reviews!

Benedikt, I'll land this now, but happy to make any changes you suggest once
your back in the office as a follow-on CL.

https://codereview.chromium.org/1239793002/diff/180001/test/unittests/compile...
File test/unittests/compiler/interpreter-assembler-unittest.h (right):

https://codereview.chromium.org/1239793002/diff/180001/test/unittests/compile...
test/unittests/compiler/interpreter-assembler-unittest.h:42: CallDescriptor*
call_descriptor() const {
On 2015/07/23 11:05:04, Michael Starzinger wrote:
> nit: Could we use "using InterpreterAssembler::call_descriptor;" here instead?

I learn something new about c++ every day :). Done.

https://codereview.chromium.org/1239793002/diff/180001/test/unittests/compile...
test/unittests/compiler/interpreter-assembler-unittest.h:45: Graph* graph() {
return InterpreterAssembler::graph(); }
On 2015/07/23 11:05:04, Michael Starzinger wrote:
> nit: Likewise.

Done. Also did the same for the constants below.

https://codereview.chromium.org/1239793002/diff/180001/test/unittests/compile...
File test/unittests/compiler/node-test-utils.cc (right):

https://codereview.chromium.org/1239793002/diff/180001/test/unittests/compile...
test/unittests/compiler/node-test-utils.cc:1892: Matcher<Node*>
IsIntPtr##Name(const Matcher<Node*>& lhs_matcher,             \
On 2015/07/23 11:05:04, Michael Starzinger wrote:
> I would be fine with having these helper based on kPointerSize, but I think
> Benedikt doesn't particularly like those and tried to avoid them. Would it be
> possible to move them into the interpreter-assembler-unittest.[h/cc] file
until
> we have time to run this by Benedikt?

Done.

[rmcilroy, 2015-07-23 11:59:01]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2015-07-23 11:59:01]

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/1239793002/#ps260001
(title: "Final comments.")

[commit-bot: I haz the power, 2015-07-23 11:59:06]

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

[commit-bot: I haz the power, 2015-07-23 12:05:09]

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

[commit-bot: I haz the power, 2015-07-23 12:05:09]

Try jobs failed on following builders:
  v8_mac_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/8008)

[rmcilroy, 2015-07-23 12:38:18]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2015-07-23 12:38:19]

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/1239793002/#ps300001
(title: "Fix compile error")

[commit-bot: I haz the power, 2015-07-23 12:38:25]

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

[rmcilroy, 2015-07-23 12:38:37]

The CQ bit was unchecked by rmcilroy@chromium.org

[rmcilroy, 2015-07-23 12:39:22]

Patchset #10 (id:300001) has been deleted

[rmcilroy, 2015-07-23 12:39:40]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2015-07-23 12:39:40]

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/1239793002/#ps310001
(title: "Fix compile error")

[commit-bot: I haz the power, 2015-07-23 12:39:48]

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

[commit-bot: I haz the power, 2015-07-23 13:02:44]

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

[commit-bot: I haz the power, 2015-07-23 13:02:45]

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

[rmcilroy, 2015-07-23 13:52:53]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2015-07-23 13:52:53]

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/1239793002/#ps330001
(title: "Fix GN for realz")

[commit-bot: I haz the power, 2015-07-23 13:53:07]

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

[commit-bot: I haz the power, 2015-07-23 14:21:31]

Committed patchset #11 (id:330001)

[commit-bot: I haz the power, 2015-07-23 14:21:49]

Patchset 11 (id:??) landed as
https://crrev.com/7877c4e0c77b5c2b97678406eab7e9ad6eba4a4d
Cr-Commit-Position: refs/heads/master@{#29814}