Move FastAccessorAssembler from RawMachineAssembler to CodeStubAssembler.

Move FastAccessorAssembler from RawMachineAssembler to CodeStubAssembler.

(The goal is to have CodeStubAssembler be the sole assembler-like user of
the TF compiler pipeline; with RMA being a private implementation detail
and FAA being a client.)

BUG=chromium:508898
LOG=Y

Committed: https://crrev.com/6c8fc936d2637160101655801494cfd5c4f00707
Cr-Commit-Position: refs/heads/master@{#34852}

[vogelheim, 2016-02-09 17:06:19]

Description was changed from

==========
Move FastAccessorAssembler from RawMachineAssembler to CodeStubAssembler.

(The goal is to have CodeStubAssembler be the sole assembler-like user of
the TF compiler pipeline; with RMA being a private implementation detail
and FAA being a client.)

BUG=
==========

to

==========
Move FastAccessorAssembler from RawMachineAssembler to CodeStubAssembler.

(The goal is to have CodeStubAssembler be the sole assembler-like user of
the TF compiler pipeline; with RMA being a private implementation detail
and FAA being a client.)

BUG=chromium:508898
LOG=Y
==========

[vogelheim, 2016-02-18 14:12:41]

vogelheim@chromium.org changed reviewers:
+ danno@chromium.org

[vogelheim, 2016-02-18 14:12:42]

Please have a look...

I guess the main issue is whether we should move CodeStubAssembler::Label out of
its enclosing class, so I don't have include the header in my header. I'm happy
either way.

[danno, 2016-02-18 15:01:15]

https://codereview.chromium.org/1674633002/diff/120001/src/compiler/code-stub...
File src/compiler/code-stub-assembler.h (right):

https://codereview.chromium.org/1674633002/diff/120001/src/compiler/code-stub...
src/compiler/code-stub-assembler.h:198: Node* CallN(CallDescriptor* descriptor,
Node* code_target, Node** args);
Nit: The TailCallStub call doesn't append the "N" above despite the args list.
Which one is more consistent with the rest of the code?

https://codereview.chromium.org/1674633002/diff/120001/src/compiler/code-stub...
src/compiler/code-stub-assembler.h:222: Node* LoadObjectFieldTyped(Node* object,
int offset, MachineType rep);
Any reason to not combine these with the LoadObjectField method and have the
MachineType rep be a default argument with value kTagged?

https://codereview.chromium.org/1674633002/diff/120001/src/compiler/fast-acce...
File src/compiler/fast-accessor-assembler.h (right):

https://codereview.chromium.org/1674633002/diff/120001/src/compiler/fast-acce...
src/compiler/fast-accessor-assembler.h:1: // Copyright 2015 the V8 project
authors. All rights reserved.
Since this class no longer uses the raw machine assembler, could you please move
it out of the compiler directory?

[vogelheim, 2016-02-22 11:39:43]

https://codereview.chromium.org/1674633002/diff/140001/src/DEPS
File src/DEPS (right):

https://codereview.chromium.org/1674633002/diff/140001/src/DEPS#newcode5
src/DEPS:5: "+src/compiler/linkage.h",
Is this ok? I need Linkage::GetStubCallDescriptor.

An alternative would be to have a sort of
CodeStubAssembler::CallWithCustomStubDescriptorOrSomething(...) that would
handle this internally.

The main drawback I'm seeing is that the only reason I need this in the first
place is that the ApiFunction stub is apparently a bit funny, and so this would
really be a use case specific method masquerading as a general purpose one.

[vogelheim, 2016-03-09 12:55:34]

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

[vogelheim, 2016-03-09 12:55:35]

Please have another look.

Thanks to the ApiCallbackStub, this no longer needs to muck around with any call
descriptors and can just use CallStub, as it was intended. This allows a clean
removal from compiler directory + namespace.

I also fixed the Context; the previous version just had a dummy, undefined
Context, while you can now ask the CodeStubAssembler to give you the context
param, based on whatever descriptor the CodeStubAssembler is generating code
for.

mstarzinger: Owners review for compiler-dir. Should be the last one for a
while... :-)

[Michael Starzinger, 2016-03-09 13:22:06]

LGTM with one comment to address.

https://codereview.chromium.org/1674633002/diff/160001/src/compiler/code-stub...
File src/compiler/code-stub-assembler.cc (right):

https://codereview.chromium.org/1674633002/diff/160001/src/compiler/code-stub...
src/compiler/code-stub-assembler.cc:132: :
Linkage::GetStubCallContextParamIndex(param_count);
I am not convinced this computation is correct for the stub-case, because basing
the calculation off of just the stack-parameters is not sufficient for stubs.
Please only implement the JSFuncionCall case for now and leave
DCHECK(descriptor->IsJSFunctionCall()) here for now until we have test coverage
for the stub case. Or alternatively move this method back to the
FastAccessorAssembler for now. Either is fine for me.

[vogelheim, 2016-03-09 13:33:09]

https://codereview.chromium.org/1674633002/diff/160001/src/compiler/code-stub...
File src/compiler/code-stub-assembler.cc (right):

https://codereview.chromium.org/1674633002/diff/160001/src/compiler/code-stub...
src/compiler/code-stub-assembler.cc:132: :
Linkage::GetStubCallContextParamIndex(param_count);
On 2016/03/09 13:22:05, Michael Starzinger wrote:
> I am not convinced this computation is correct for the stub-case, because
basing
> the calculation off of just the stack-parameters is not sufficient for stubs.
> Please only implement the JSFuncionCall case for now and leave
> DCHECK(descriptor->IsJSFunctionCall()) here for now until we have test
coverage
> for the stub case. Or alternatively move this method back to the
> FastAccessorAssembler for now. Either is fine for me.

Done, by making it JSCall only.

Moving it into the calling class would have required exposing the call
descriptor, which so far the CodeStubAssembler doesn't do. I think the design is
that CodeStubA~ hides all compiler-ish stuff from the caller, so it seems better
to keep it that way.

[danno, 2016-03-15 14:16:03]

https://codereview.chromium.org/1674633002/diff/200001/src/compiler/code-stub...
File src/compiler/code-stub-assembler.h (right):

https://codereview.chromium.org/1674633002/diff/200001/src/compiler/code-stub...
src/compiler/code-stub-assembler.h:149: Node* ContextParameter();  // Only for
JSCall linkage.
I'd prefer that this JavaScript-ism wasn't in the general CodeStubAssembler. Can
you move it to the subclass? It only ends up doing some math and then calling
Parameter(), and it would be great to keep JSCall-specific stuff out of this
class.

[vogelheim, 2016-03-15 16:00:03]

https://codereview.chromium.org/1674633002/diff/200001/src/compiler/code-stub...
File src/compiler/code-stub-assembler.h (right):

https://codereview.chromium.org/1674633002/diff/200001/src/compiler/code-stub...
src/compiler/code-stub-assembler.h:149: Node* ContextParameter();  // Only for
JSCall linkage.
On 2016/03/15 14:16:02, danno wrote:
> I'd prefer that this JavaScript-ism wasn't in the general CodeStubAssembler.
Can
> you move it to the subclass? It only ends up doing some math and then calling
> Parameter(), and it would be great to keep JSCall-specific stuff out of this
> class.

Removed.

Replaced with a 'hard coded' parameter in the client, plus a TODO + explanation
why.

[danno, 2016-03-16 21:40:36]

lgtm

[vogelheim, 2016-03-17 10:26:13]

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

[commit-bot: I haz the power, 2016-03-17 10:26:23]

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

[commit-bot: I haz the power, 2016-03-17 11:01:49]

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

[commit-bot: I haz the power, 2016-03-17 11:01:50]

Dry run: This issue passed the CQ dry run.

[vogelheim, 2016-03-17 11:02:28]

The CQ bit was checked by vogelheim@chromium.org

[vogelheim, 2016-03-17 11:02:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1674633002/#ps220001
(title: "Remove CodeStubAssembler::ContextParameter.")

[commit-bot: I haz the power, 2016-03-17 11:02:36]

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

[commit-bot: I haz the power, 2016-03-17 11:04:56]

Description was changed from

==========
Move FastAccessorAssembler from RawMachineAssembler to CodeStubAssembler.

(The goal is to have CodeStubAssembler be the sole assembler-like user of
the TF compiler pipeline; with RMA being a private implementation detail
and FAA being a client.)

BUG=chromium:508898
LOG=Y
==========

to

==========
Move FastAccessorAssembler from RawMachineAssembler to CodeStubAssembler.

(The goal is to have CodeStubAssembler be the sole assembler-like user of
the TF compiler pipeline; with RMA being a private implementation detail
and FAA being a client.)

BUG=chromium:508898
LOG=Y
==========

[commit-bot: I haz the power, 2016-03-17 11:04:57]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-03-17 11:06:18]

Description was changed from

==========
Move FastAccessorAssembler from RawMachineAssembler to CodeStubAssembler.

(The goal is to have CodeStubAssembler be the sole assembler-like user of
the TF compiler pipeline; with RMA being a private implementation detail
and FAA being a client.)

BUG=chromium:508898
LOG=Y
==========

to

==========
Move FastAccessorAssembler from RawMachineAssembler to CodeStubAssembler.

(The goal is to have CodeStubAssembler be the sole assembler-like user of
the TF compiler pipeline; with RMA being a private implementation detail
and FAA being a client.)

BUG=chromium:508898
LOG=Y

Committed: https://crrev.com/6c8fc936d2637160101655801494cfd5c4f00707
Cr-Commit-Position: refs/heads/master@{#34852}
==========

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

Patchset 12 (id:??) landed as
https://crrev.com/6c8fc936d2637160101655801494cfd5c4f00707
Cr-Commit-Position: refs/heads/master@{#34852}