[refactoring] Split CodeAssemblerState out of CodeAssembler

[refactoring] Split CodeAssemblerState out of CodeAssembler

This is in preparation for introducing more specialized
CodeStubAssembler subclasses. The state object can be handed
around, while the Assembler instances are temporary-scoped.

BUG=v8:5628
Committed: https://crrev.com/913da29ea211c4b6d4680b5dae7da8f96a43fdde
Cr-Commit-Position: refs/heads/master@{#41015}

[Jakob Kummerow, 2016-11-15 09:51:49]

jkummerow@chromium.org changed reviewers:
+ ishell@chromium.org, mstarzinger@chromium.org

[Jakob Kummerow, 2016-11-15 09:51:50]

PTAL :-)

(Michi - compiler, interpreter; Igor - everything else)

The majority of the changes is s/assembler->/assembler./. This paves the way for
giving all these places a similar treatment as the KeyedStoreGenericAssembler,
i.e. using local specialized CSA subclasses.

https://codereview.chromium.org/2498073002/diff/1/src/builtins/builtins.h
File src/builtins/builtins.h (left):

https://codereview.chromium.org/2498073002/diff/1/src/builtins/builtins.h#old...
src/builtins/builtins.h:819: static void
Generate_DatePrototype_GetField(CodeStubAssembler* masm,
Moved this to an anonymous namespace; no need to declare it in the header.

https://codereview.chromium.org/2498073002/diff/1/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/2498073002/diff/1/src/code-stubs.cc#newcode1697
src/code-stubs.cc:1697: assembler.Return(Generate(&assembler,
assembler.Parameter(0),             \
I can't use named constants here because we don't have any. I don't think this
code works as intended; I also don't think it's used :-)
But that's a cleanup for another day.

https://codereview.chromium.org/2498073002/diff/1/src/code-stubs.h
File src/code-stubs.h (left):

https://codereview.chromium.org/2498073002/diff/1/src/code-stubs.h#oldcode393
src/code-stubs.h:393: #define DEFINE_TURBOFAN_BINARY_OP_CODE_STUB(NAME, SUPER)  
                    \
This was dead code.

https://codereview.chromium.org/2498073002/diff/1/src/code-stubs.h#oldcode411
src/code-stubs.h:411: void GenerateAssembly(CodeStubAssembler* assembler) const
override {        \
Moved this implementation to the .cc file.

https://codereview.chromium.org/2498073002/diff/1/src/code-stubs.h#oldcode419
src/code-stubs.h:419: #define DEFINE_TURBOFAN_UNARY_OP_CODE_STUB(NAME, SUPER)   
            \
This was dead code.

https://codereview.chromium.org/2498073002/diff/1/src/code-stubs.h#oldcode436
src/code-stubs.h:436: void GenerateAssembly(CodeStubAssembler* assembler) const
override {        \
Moved this implementation to the .cc file.

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

https://codereview.chromium.org/2498073002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:23: using compiler::CodeAssemblerState;
I'll drop this in a new patch set (or use it everywhere -- wdyt?)

[Michael Starzinger, 2016-11-15 11:45:21]

Looked at "interpreter" and "compiler", looking good. Just a couple of nits and
minor comments.

https://codereview.chromium.org/2498073002/diff/1/src/compiler/code-assembler.h
File src/compiler/code-assembler.h (right):

https://codereview.chromium.org/2498073002/diff/1/src/compiler/code-assembler...
src/compiler/code-assembler.h:181: class CodeAssemblerState;
nit: Please move this forward declaration up to the other ones at the top of
this file.

https://codereview.chromium.org/2498073002/diff/1/src/compiler/code-assembler...
src/compiler/code-assembler.h:520: class V8_EXPORT_PRIVATE CodeAssemblerState {
nit: Can we make this final?

https://codereview.chromium.org/2498073002/diff/1/src/compiler/code-assembler...
src/compiler/code-assembler.h:536: Handle<Code> GenerateCode();
Can we move this back to the {CodeAssembler} class? That way this class is pure
container class without logic. The only way the state of this class changes is
by using the {CodeAssmelber} to mutate it. WDYT?

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

https://codereview.chromium.org/2498073002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:23: using compiler::CodeAssemblerState;
On 2016/11/15 09:51:50, Jakob Kummerow wrote:
> I'll drop this in a new patch set (or use it everywhere -- wdyt?)

I think it is fine to use here (or somewhere else for that matter).

https://codereview.chromium.org/2498073002/diff/1/test/cctest/compiler/code-a...
File test/cctest/compiler/code-assembler-tester.h (right):

https://codereview.chromium.org/2498073002/diff/1/test/cctest/compiler/code-a...
test/cctest/compiler/code-assembler-tester.h:15: class AssemblerTesterData {
nit: Why not just keep this class named "CodeAssemblerTester". I know the
semantics of this class changed, but I think the name still applies. Also then
the file-name still makes sense.

Also, I like that it is no longer templatized! :)

https://codereview.chromium.org/2498073002/diff/1/test/unittests/interpreter/...
File test/unittests/interpreter/interpreter-assembler-unittest.h (right):

https://codereview.chromium.org/2498073002/diff/1/test/unittests/interpreter/...
test/unittests/interpreter/interpreter-assembler-unittest.h:22: class
InterpreterAssemblerTestState : public compiler::CodeAssemblerState {
Ah, now I see why making {CodeAssemblerState} wouldn't work right away. Could we
instead remove this helper class and make the {InterpreterAssemblerForTest}
class embed a {CodeAssemblerState} field. That way it is all packed into one
class and the unit tests don't need to care about a separate state at all.

[Michael Starzinger, 2016-11-15 11:53:08]

https://codereview.chromium.org/2498073002/diff/1/src/interpreter/interpreter.cc
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/2498073002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.cc:80: compiler::CodeAssemblerState state(          
                           \
Actually, come to think of it, we could use the same approach I suggested for
the unit-test here. Make the {CodeAssemblerState} an embedded field inside of
{InterpreterAssembler}. In the case of the interpreter the state is never
separated from the assembler and coupling them together makes the
{InterpreterAssembler} easier to understand. WDYT?

[Igor Sheludko, 2016-11-15 13:06:35]

lgtm with suggestions:

https://codereview.chromium.org/2498073002/diff/1/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/2498073002/diff/1/src/code-stubs.cc#newcode1697
src/code-stubs.cc:1697: assembler.Return(Generate(&assembler,
assembler.Parameter(0),             \
On 2016/11/15 09:51:50, Jakob Kummerow wrote:
> I can't use named constants here because we don't have any. I don't think this
> code works as intended; I also don't think it's used :-)
> But that's a cleanup for another day.

It's actually a CountOpDescriptor. So if you define a parameter names in it by
  DEFINE_PARAMETERS(kOperand, kVector, kSlot)
Then you can use the names here.

https://codereview.chromium.org/2498073002/diff/1/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/2498073002/diff/1/src/code-stubs.h#newcode764
src/code-stubs.h:764: void GenerateAssembly(compiler::CodeAssemblerState* state)
const override;
While you are here, what if we remove this and ...

https://codereview.chromium.org/2498073002/diff/1/src/code-stubs.h#newcode770
src/code-stubs.h:770: DEFINE_CODE_STUB(StoreInterceptor, TurboFanCodeStub);
... use here DEFINE_TURBOFAN_CODE_STUB instead? Same for the other
TurboFanCodeStubs.

https://codereview.chromium.org/2498073002/diff/1/src/compiler/code-assembler.cc
File src/compiler/code-assembler.cc (right):

https://codereview.chromium.org/2498073002/diff/1/src/compiler/code-assembler...
src/compiler/code-assembler.cc:1059:
assembler->state_->variables_.insert(impl_);
It looks like we need CodeAssemblerState instead of CodeAssembler here.

https://codereview.chromium.org/2498073002/diff/1/src/compiler/code-assembler...
src/compiler/code-assembler.cc:1095: for (auto var :
assembler_->state_->variables_) {
Same here.

https://codereview.chromium.org/2498073002/diff/1/src/compiler/code-assembler.h
File src/compiler/code-assembler.h (right):

https://codereview.chromium.org/2498073002/diff/1/src/compiler/code-assembler...
src/compiler/code-assembler.h:520: class V8_EXPORT_PRIVATE CodeAssemblerState {
On 2016/11/15 11:45:20, Michael Starzinger wrote:
> nit: Can we make this final?

Thinking out loud: we could even hide this class inside .cc file if we add
static methods that create CodeAssemblerState object to CodeAssembler class.

https://codereview.chromium.org/2498073002/diff/1/src/compiler/code-assembler...
src/compiler/code-assembler.h:536: Handle<Code> GenerateCode();
On 2016/11/15 11:45:20, Michael Starzinger wrote:
> Can we move this back to the {CodeAssembler} class? That way this class is
pure
> container class without logic. The only way the state of this class changes is
> by using the {CodeAssmelber} to mutate it. WDYT?

If we do that then we will not be able to generate builtins code from the state
as we do in BuildWithCodeStubAssembler*().

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

https://codereview.chromium.org/2498073002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:23: using compiler::CodeAssemblerState;
On 2016/11/15 11:45:20, Michael Starzinger wrote:
> On 2016/11/15 09:51:50, Jakob Kummerow wrote:
> > I'll drop this in a new patch set (or use it everywhere -- wdyt?)
> 
> I think it is fine to use here (or somewhere else for that matter).

I also think it's fine to use "using" declaration inside .cc file.

https://codereview.chromium.org/2498073002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:26:
InterpreterAssembler::InterpreterAssembler(compiler::CodeAssemblerState* state,
... and you can drop the compiler:: part here.

https://codereview.chromium.org/2498073002/diff/1/src/interpreter/interpreter.cc
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/2498073002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.cc:80: compiler::CodeAssemblerState state(          
                           \
On 2016/11/15 11:53:08, Michael Starzinger wrote:
> Actually, come to think of it, we could use the same approach I suggested for
> the unit-test here. Make the {CodeAssemblerState} an embedded field inside of
> {InterpreterAssembler}. In the case of the interpreter the state is never
> separated from the assembler and coupling them together makes the
> {InterpreterAssembler} easier to understand. WDYT?

Then we will be passing a pointer to not yet constructed embedded field to the
base class constructor.

https://codereview.chromium.org/2498073002/diff/1/test/unittests/interpreter/...
File test/unittests/interpreter/interpreter-assembler-unittest.cc (right):

https://codereview.chromium.org/2498073002/diff/1/test/unittests/interpreter/...
test/unittests/interpreter/interpreter-assembler-unittest.cc:309:
InterpreterAssemblerForTest m(&state, bytecode);
... constructor function that creates an InterpreterAssemblerForTest from this
and bytecode.

https://codereview.chromium.org/2498073002/diff/1/test/unittests/interpreter/...
File test/unittests/interpreter/interpreter-assembler-unittest.h (right):

https://codereview.chromium.org/2498073002/diff/1/test/unittests/interpreter/...
test/unittests/interpreter/interpreter-assembler-unittest.h:22: class
InterpreterAssemblerTestState : public compiler::CodeAssemblerState {
On 2016/11/15 11:45:21, Michael Starzinger wrote:
> Ah, now I see why making {CodeAssemblerState} wouldn't work right away. Could
we
> instead remove this helper class and make the {InterpreterAssemblerForTest}
> class embed a {CodeAssemblerState} field. That way it is all packed into one
> class and the unit tests don't need to care about a separate state at all.

Or alternatively we can have a helper function that creates an
InterpreterAssemblerForTest from (InterpreterAssemblerTest* test, Bytecode
bytecode):
  InterpreterAssemblerForTest MakeInterpreterAssembler(InterpreterAssemblerTest*
test, Bytecode bytecode);

[Jakob Kummerow, 2016-11-15 13:40:32]

https://codereview.chromium.org/2498073002/diff/1/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/2498073002/diff/1/src/code-stubs.cc#newcode1697
src/code-stubs.cc:1697: assembler.Return(Generate(&assembler,
assembler.Parameter(0),             \
On 2016/11/15 13:06:35, Igor Sheludko wrote:
> On 2016/11/15 09:51:50, Jakob Kummerow wrote:
> > I can't use named constants here because we don't have any. I don't think
this
> > code works as intended; I also don't think it's used :-)
> > But that's a cleanup for another day.
> 
> It's actually a CountOpDescriptor. So if you define a parameter names in it by
>   DEFINE_PARAMETERS(kOperand, kVector, kSlot)
> Then you can use the names here.

I still think it's an unrelated change :-)
I can do a follow-up CL.

https://codereview.chromium.org/2498073002/diff/1/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/2498073002/diff/1/src/code-stubs.h#newcode764
src/code-stubs.h:764: void GenerateAssembly(compiler::CodeAssemblerState* state)
const override;
On 2016/11/15 13:06:35, Igor Sheludko wrote:
> While you are here, what if we remove this and ...

Done.

https://codereview.chromium.org/2498073002/diff/1/src/code-stubs.h#newcode770
src/code-stubs.h:770: DEFINE_CODE_STUB(StoreInterceptor, TurboFanCodeStub);
On 2016/11/15 13:06:35, Igor Sheludko wrote:
> ... use here DEFINE_TURBOFAN_CODE_STUB instead? Same for the other
> TurboFanCodeStubs.

Brilliant.

https://codereview.chromium.org/2498073002/diff/1/src/compiler/code-assembler.cc
File src/compiler/code-assembler.cc (right):

https://codereview.chromium.org/2498073002/diff/1/src/compiler/code-assembler...
src/compiler/code-assembler.cc:1059:
assembler->state_->variables_.insert(impl_);
On 2016/11/15 13:06:35, Igor Sheludko wrote:
> It looks like we need CodeAssemblerState instead of CodeAssembler here.

Done.

https://codereview.chromium.org/2498073002/diff/1/src/compiler/code-assembler...
src/compiler/code-assembler.cc:1095: for (auto var :
assembler_->state_->variables_) {
On 2016/11/15 13:06:35, Igor Sheludko wrote:
> Same here.

Done.

https://codereview.chromium.org/2498073002/diff/1/src/compiler/code-assembler.h
File src/compiler/code-assembler.h (right):

https://codereview.chromium.org/2498073002/diff/1/src/compiler/code-assembler...
src/compiler/code-assembler.h:181: class CodeAssemblerState;
On 2016/11/15 11:45:20, Michael Starzinger wrote:
> nit: Please move this forward declaration up to the other ones at the top of
> this file.

Done.

https://codereview.chromium.org/2498073002/diff/1/src/compiler/code-assembler...
src/compiler/code-assembler.h:520: class V8_EXPORT_PRIVATE CodeAssemblerState {
On 2016/11/15 11:45:20, Michael Starzinger wrote:
> nit: Can we make this final?

I'd love to; but see comment on unit test.

https://codereview.chromium.org/2498073002/diff/1/src/compiler/code-assembler...
src/compiler/code-assembler.h:520: class V8_EXPORT_PRIVATE CodeAssemblerState {
On 2016/11/15 13:06:35, Igor Sheludko wrote:
> On 2016/11/15 11:45:20, Michael Starzinger wrote:
> > nit: Can we make this final?
> 
> Thinking out loud: we could even hide this class inside .cc file if we add
> static methods that create CodeAssemblerState object to CodeAssembler class.

We could do that. But then we can't heap-allocate them any more. Do we want
that?

https://codereview.chromium.org/2498073002/diff/1/src/compiler/code-assembler...
src/compiler/code-assembler.h:536: Handle<Code> GenerateCode();
On 2016/11/15 11:45:20, Michael Starzinger wrote:
> Can we move this back to the {CodeAssembler} class? That way this class is
pure
> container class without logic. The only way the state of this class changes is
> by using the {CodeAssmelber} to mutate it. WDYT?

Summarizing offline discussion:
- The argument in favor of moving this back is so that the State is just a data
container with no logic at all. That design preserves maximum freedom from the
compiler:: side to change under-the-hood implementation details.
- The argument in favor of keeping it on the State object is that the various
*Assembler classes are responsible for accumulating things in the State
container. Taking the accumulated stuff out of the container is a separate use
case; having to instantiate a CodeAssembler just to be able to do that doesn't
seem to add much value.

Compromise idea: make it a static function!

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

https://codereview.chromium.org/2498073002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:23: using compiler::CodeAssemblerState;
On 2016/11/15 11:45:20, Michael Starzinger wrote:
> On 2016/11/15 09:51:50, Jakob Kummerow wrote:
> > I'll drop this in a new patch set (or use it everywhere -- wdyt?)
> 
> I think it is fine to use here (or somewhere else for that matter).

Done.

https://codereview.chromium.org/2498073002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:26:
InterpreterAssembler::InterpreterAssembler(compiler::CodeAssemblerState* state,
On 2016/11/15 13:06:35, Igor Sheludko wrote:
> ... and you can drop the compiler:: part here.

Right, that's what I meant. Done.

https://codereview.chromium.org/2498073002/diff/1/src/interpreter/interpreter.cc
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/2498073002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.cc:80: compiler::CodeAssemblerState state(          
                           \
On 2016/11/15 11:53:08, Michael Starzinger wrote:
> Actually, come to think of it, we could use the same approach I suggested for
> the unit-test here. Make the {CodeAssemblerState} an embedded field inside of
> {InterpreterAssembler}. In the case of the interpreter the state is never
> separated from the assembler and coupling them together makes the
> {InterpreterAssembler} easier to understand. WDYT?

For now that would work; but our plans for further cleanup are to have the
interpreter pass State objects (with partially created stubs in them) around to
other generators, in order to share implementation with stub/builtin generation.
To keep the flexibility needed for that, it makes sense not to embed the State
object.

https://codereview.chromium.org/2498073002/diff/1/test/cctest/compiler/code-a...
File test/cctest/compiler/code-assembler-tester.h (right):

https://codereview.chromium.org/2498073002/diff/1/test/cctest/compiler/code-a...
test/cctest/compiler/code-assembler-tester.h:15: class AssemblerTesterData {
On 2016/11/15 11:45:20, Michael Starzinger wrote:
> nit: Why not just keep this class named "CodeAssemblerTester". I know the
> semantics of this class changed, but I think the name still applies. Also then
> the file-name still makes sense.
> 
> Also, I like that it is no longer templatized! :)

Done.

https://codereview.chromium.org/2498073002/diff/1/test/unittests/interpreter/...
File test/unittests/interpreter/interpreter-assembler-unittest.h (right):

https://codereview.chromium.org/2498073002/diff/1/test/unittests/interpreter/...
test/unittests/interpreter/interpreter-assembler-unittest.h:22: class
InterpreterAssemblerTestState : public compiler::CodeAssemblerState {
On 2016/11/15 11:45:21, Michael Starzinger wrote:
> Ah, now I see why making {CodeAssemblerState} wouldn't work right away. Could
we
> instead remove this helper class and make the {InterpreterAssemblerForTest}
> class embed a {CodeAssemblerState} field. That way it is all packed into one
> class and the unit tests don't need to care about a separate state at all.

Summarizing offline discussion: we have several options:

(1) Keep this overriding class.

(2) This class is just convenience; what we do in its constructor, we could just
do at every instantiation site. That would significantly increase the amount of
boilerplate code in the test.

(3) Encapsulate the boilerplate, but without overriding. That would lead to a
similar solution as the CodeAssemblerTester used in other unit tests
(essentially, TestState has-a CodeAssemblerState instead of is-a).

(4) Embed a CodeAssemblerState into InterpreterAssemblerForTest. However, that
class derives from InterpreterAssembler and hence must pass a State* to its
super constructor. At the time of the super constructor call, its embedded
fields are not yet initialized. This seems brittle.

We agreed that option (1) is preferable over (2) - (4), so that's what we'll do
until we come up with a better idea.

[Igor Sheludko, 2016-11-15 14:02:53]

lgtm

https://codereview.chromium.org/2498073002/diff/1/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/2498073002/diff/1/src/code-stubs.cc#newcode1697
src/code-stubs.cc:1697: assembler.Return(Generate(&assembler,
assembler.Parameter(0),             \
On 2016/11/15 13:40:31, Jakob Kummerow wrote:
> On 2016/11/15 13:06:35, Igor Sheludko wrote:
> > On 2016/11/15 09:51:50, Jakob Kummerow wrote:
> > > I can't use named constants here because we don't have any. I don't think
> this
> > > code works as intended; I also don't think it's used :-)
> > > But that's a cleanup for another day.
> > 
> > It's actually a CountOpDescriptor. So if you define a parameter names in it
by
> >   DEFINE_PARAMETERS(kOperand, kVector, kSlot)
> > Then you can use the names here.
> 
> I still think it's an unrelated change :-)
> I can do a follow-up CL.

Acknowledged.

https://codereview.chromium.org/2498073002/diff/1/src/compiler/code-assembler.h
File src/compiler/code-assembler.h (right):

https://codereview.chromium.org/2498073002/diff/1/src/compiler/code-assembler...
src/compiler/code-assembler.h:520: class V8_EXPORT_PRIVATE CodeAssemblerState {
On 2016/11/15 13:40:32, Jakob Kummerow wrote:
> On 2016/11/15 13:06:35, Igor Sheludko wrote:
> > On 2016/11/15 11:45:20, Michael Starzinger wrote:
> > > nit: Can we make this final?
> > 
> > Thinking out loud: we could even hide this class inside .cc file if we add
> > static methods that create CodeAssemblerState object to CodeAssembler class.
> 
> We could do that. But then we can't heap-allocate them any more. Do we want
> that?

Ah, I missed the DISALLOW_COPY_AND_ASSIGN thing.

https://codereview.chromium.org/2498073002/diff/1/src/compiler/code-assembler...
src/compiler/code-assembler.h:536: Handle<Code> GenerateCode();
On 2016/11/15 13:40:32, Jakob Kummerow wrote:
> On 2016/11/15 11:45:20, Michael Starzinger wrote:
> > Can we move this back to the {CodeAssembler} class? That way this class is
> pure
> > container class without logic. The only way the state of this class changes
is
> > by using the {CodeAssmelber} to mutate it. WDYT?
> 
> Summarizing offline discussion:
> - The argument in favor of moving this back is so that the State is just a
data
> container with no logic at all. That design preserves maximum freedom from the
> compiler:: side to change under-the-hood implementation details.
> - The argument in favor of keeping it on the State object is that the various
> *Assembler classes are responsible for accumulating things in the State
> container. Taking the accumulated stuff out of the container is a separate use
> case; having to instantiate a CodeAssembler just to be able to do that doesn't
> seem to add much value.
> 
> Compromise idea: make it a static function!

Awesome!

https://codereview.chromium.org/2498073002/diff/20001/src/compiler/code-assem...
File src/compiler/code-assembler.cc (right):

https://codereview.chromium.org/2498073002/diff/20001/src/compiler/code-assem...
src/compiler/code-assembler.cc:65: Handle<Code>
CodeAssembler::GenerateCode(CodeAssemblerState* state) {
This code could probably be moved down where it was :)

[Michael Starzinger, 2016-11-15 15:09:48]

LGTM from my end. Thanks!

https://codereview.chromium.org/2498073002/diff/40001/src/compiler/code-assem...
File src/compiler/code-assembler.h (right):

https://codereview.chromium.org/2498073002/diff/40001/src/compiler/code-assem...
src/compiler/code-assembler.h:534: Handle<Code> GenerateCode();
nit: Implementation is gone, lets drop the declaration.

[Jakob Kummerow, 2016-11-15 15:21:13]

Thanks, landing.

https://codereview.chromium.org/2498073002/diff/20001/src/compiler/code-assem...
File src/compiler/code-assembler.cc (right):

https://codereview.chromium.org/2498073002/diff/20001/src/compiler/code-assem...
src/compiler/code-assembler.cc:65: Handle<Code>
CodeAssembler::GenerateCode(CodeAssemblerState* state) {
On 2016/11/15 14:02:53, Igor Sheludko wrote:
> This code could probably be moved down where it was :)

Done.

https://codereview.chromium.org/2498073002/diff/40001/src/compiler/code-assem...
File src/compiler/code-assembler.h (right):

https://codereview.chromium.org/2498073002/diff/40001/src/compiler/code-assem...
src/compiler/code-assembler.h:534: Handle<Code> GenerateCode();
On 2016/11/15 15:09:47, Michael Starzinger wrote:
> nit: Implementation is gone, lets drop the declaration.

Done.

[Jakob Kummerow, 2016-11-15 15:21:19]

The CQ bit was checked by jkummerow@chromium.org

[Jakob Kummerow, 2016-11-15 15:21:20]

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

[commit-bot: I haz the power, 2016-11-15 15:21:33]

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, 2016-11-15 15:23:20]

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

[commit-bot: I haz the power, 2016-11-15 15:23:21]

Try jobs failed on following builders:
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/28580)

[Jakob Kummerow, 2016-11-15 15:52:27]

The CQ bit was checked by jkummerow@chromium.org

[Jakob Kummerow, 2016-11-15 15:52:28]

The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org,
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/2498073002/#ps80001
(title: "rebased again")

[commit-bot: I haz the power, 2016-11-15 15:52:39]

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, 2016-11-15 15:54:06]

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

[commit-bot: I haz the power, 2016-11-15 15:54:07]

Try jobs failed on following builders:
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/16234)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)

[Jakob Kummerow, 2016-11-15 16:05:34]

The CQ bit was checked by jkummerow@chromium.org

[commit-bot: I haz the power, 2016-11-15 16:05:43]

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, 2016-11-15 16:08:38]

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

[commit-bot: I haz the power, 2016-11-15 16:08:40]

Try jobs failed on following builders:
  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_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/2...)

[Jakob Kummerow, 2016-11-15 17:19:08]

The CQ bit was checked by jkummerow@chromium.org

[Jakob Kummerow, 2016-11-15 17:19:09]

The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org,
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/2498073002/#ps100001
(title: "rebased again")

[commit-bot: I haz the power, 2016-11-15 17:19:15]

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, 2016-11-15 17:25:09]

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

[commit-bot: I haz the power, 2016-11-15 17:25:11]

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

[Jakob Kummerow, 2016-11-15 22:06:01]

The CQ bit was checked by jkummerow@chromium.org

[Jakob Kummerow, 2016-11-15 22:06:02]

The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org,
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/2498073002/#ps140001
(title: "one more attempt")

[commit-bot: I haz the power, 2016-11-15 22:06:14]

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, 2016-11-15 22:57:15]

Committed patchset #8 (id:140001)

[Michael Achenbach, 2016-11-16 07:53:29]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/2504913002/ by machenbach@chromium.org.

The reason for reverting is:
https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20shared doesn't
want to compile. Missing export annotation?.

[commit-bot: I haz the power, 2016-11-17 22:35:18]

Description was changed from

==========
[refactoring] Split CodeAssemblerState out of CodeAssembler

This is in preparation for introducing more specialized
CodeStubAssembler subclasses. The state object can be handed
around, while the Assembler instances are temporary-scoped.

BUG=v8:5628
==========

to

==========
[refactoring] Split CodeAssemblerState out of CodeAssembler

This is in preparation for introducing more specialized
CodeStubAssembler subclasses. The state object can be handed
around, while the Assembler instances are temporary-scoped.

BUG=v8:5628

Committed: https://crrev.com/913da29ea211c4b6d4680b5dae7da8f96a43fdde
Cr-Commit-Position: refs/heads/master@{#41015}
==========

[commit-bot: I haz the power, 2016-11-17 22:35:20]

Patchset 8 (id:??) landed as
https://crrev.com/913da29ea211c4b6d4680b5dae7da8f96a43fdde
Cr-Commit-Position: refs/heads/master@{#41015}