Switch CallConstructStub to take new.target in register.

Switch CallConstructStub to take new.target in register.

This changes the calling convention of the CallConstructStub to take
the original constructor (i.e. new.target in JS-speak) in a register
instead of magically via the operand stack. For optimizing compilers
the operand stack doesn't exist, hence cannot be peeked into.

R=mvstanton@chromium.org

Committed: https://crrev.com/1d9d895754e1d1cf824c11a9cce5e495fa47d5e2
Cr-Commit-Position: refs/heads/master@{#29681}

[Michael Starzinger, 2015-07-14 11:43:30]

Note that this only contains the ARM port, but I wanted to get your general
opinion on this early, before doing the ports.

[Michael Starzinger, 2015-07-14 15:02:47]

mstarzinger@chromium.org changed reviewers:
+ balazs.kilvady@imgtec.com, jacob.bramley@arm.com

[Michael Starzinger, 2015-07-14 15:02:47]

Ported to most architectures. Also adding the appropriate architecture experts
to take a look at the ports.

Jacob: PTAL at the ARM ports, thanks!
Balazs: PTAL at the MIPS ports, thanks!

https://codereview.chromium.org/1237813002/diff/20001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/1237813002/diff/20001/src/arm/code-stubs-arm....
src/arm/code-stubs-arm.cc:2622: __ push(r4);
Potentially applies to all architectures: Note that I realize that on some
architectures we could probably avoid this push and the pop a few lines below,
by massaging GenerateRecordCallTarget appropriately so that it preserves the
register in question. However I would like to do this in a follow-up CL if
possible. But I am of course happy to leave a TODO for all architectures where
this might apply, just let me know.

[jbramley, 2015-07-14 16:57:35]

jacob.bramley@arm.com changed reviewers:
+ rodolph.perfetta@arm.com

[jbramley, 2015-07-14 16:57:36]

ARM and ARM64 looks sensible. Minor comments only.

I won't be in the office tomorrow so I've put Rodolph on the list in case you
want another ARM review.

https://codereview.chromium.org/1237813002/diff/20001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/1237813002/diff/20001/src/arm/code-stubs-arm....
src/arm/code-stubs-arm.cc:2622: __ push(r4);
On 2015/07/14 15:02:47, Michael Starzinger wrote:
> Potentially applies to all architectures: Note that I realize that on some
> architectures we could probably avoid this push and the pop a few lines below,
> by massaging GenerateRecordCallTarget appropriately so that it preserves the
> register in question. However I would like to do this in a follow-up CL if
> possible. But I am of course happy to leave a TODO for all architectures where
> this might apply, just let me know.

A TODO is probably a good idea, but it's up to you. On ARM64,
GenerateRecordCallTarget takes a list of scratch registers, and that approach
works quite well.

https://codereview.chromium.org/1237813002/diff/20001/src/arm64/code-stubs-ar...
File src/arm64/code-stubs-arm64.cc (right):

https://codereview.chromium.org/1237813002/diff/20001/src/arm64/code-stubs-ar...
src/arm64/code-stubs-arm64.cc:3004: GenerateRecordCallTarget(masm, x0, function,
x2, x3, x4, x5, x11);
ARM64's GenerateRecordCallTarget allows the scratch registers to be specified,
so it should be possible to just specify something other than x4 and skip the
push/pop sequence.

https://codereview.chromium.org/1237813002/diff/20001/src/arm64/full-codegen-...
File src/arm64/full-codegen-arm64.cc (right):

https://codereview.chromium.org/1237813002/diff/20001/src/arm64/full-codegen-...
src/arm64/full-codegen-arm64.cc:3093: __ mov(x4, result_register());
We prefer the MacroAssembler's "__ Mov(...)" in ARM64, for consistency, even
though it doesn't make any difference in this particular case. (The lower-case
Assembler versions are usually reserved for critical sections in
InstructionAccurateScopes.)

[paul.l..., 2015-07-14 16:58:17]

paul.lind@imgtec.com changed reviewers:
+ paul.lind@imgtec.com
- rodolph.perfetta@arm.com

[paul.l..., 2015-07-14 16:58:18]

mips32 port looks good.

Some register usage/naming issues on mips64: the mips64 ABI allows passing of 8
arguments in registers, and therefore, the 'conventional' names of registers 4
through 15 were changed for mips64, as compared to mips32 (allows 4 arguments
passed in registers): 

mips32 regs 4-15: [a0-a3],[t0-t7]
mips64 regs 4-15: [a0-a7],[t0-t3]

We decided to follow these conventions within the mips64 port, since this is in
common usage in other projects.

I've noted the changes inline.

https://codereview.chromium.org/1237813002/diff/20001/src/mips64/builtins-mip...
File src/mips64/builtins-mips64.cc (right):

https://codereview.chromium.org/1237813002/diff/20001/src/mips64/builtins-mip...
src/mips64/builtins-mips64.cc:1511: __ ld(t0, MemOperand(fp, kNewTargetOffset));
t0 -> a4

https://codereview.chromium.org/1237813002/diff/20001/src/mips64/code-stubs-m...
File src/mips64/code-stubs-mips64.cc (right):

https://codereview.chromium.org/1237813002/diff/20001/src/mips64/code-stubs-m...
src/mips64/code-stubs-mips64.cc:2784: // t0 : original constructor (for
IsSuperConstructorCall)
Please change t0 -> a4

https://codereview.chromium.org/1237813002/diff/20001/src/mips64/code-stubs-m...
src/mips64/code-stubs-mips64.cc:2790: __ Branch(&slow, ne, a4,
Operand(JS_FUNCTION_TYPE));
Please change these 3 uses of a4 -> a5

https://codereview.chromium.org/1237813002/diff/20001/src/mips64/code-stubs-m...
src/mips64/code-stubs-mips64.cc:2794: __ push(t0);
Please use a4 here and in pop() below.

https://codereview.chromium.org/1237813002/diff/20001/src/mips64/code-stubs-m...
src/mips64/code-stubs-mips64.cc:2824: __ mov(a3, t0);
t0 -> a4

https://codereview.chromium.org/1237813002/diff/20001/src/mips64/code-stubs-m...
src/mips64/code-stubs-mips64.cc:2839: // a4: object type
a4 -> a5 for object type

https://codereview.chromium.org/1237813002/diff/20001/src/mips64/code-stubs-m...
src/mips64/code-stubs-mips64.cc:2842: __ Branch(&non_function_call, ne, a4,
Operand(JS_FUNCTION_PROXY_TYPE));
a4 -> a5

https://codereview.chromium.org/1237813002/diff/20001/src/mips64/full-codegen...
File src/mips64/full-codegen-mips64.cc (right):

https://codereview.chromium.org/1237813002/diff/20001/src/mips64/full-codegen...
src/mips64/full-codegen-mips64.cc:3388: __ mov(t0, result_register());
t0 -> a4 here, and in comment above.

https://codereview.chromium.org/1237813002/diff/20001/src/mips64/full-codegen...
src/mips64/full-codegen-mips64.cc:4348: __ ld(t0, MemOperand(sp, 1 *
kPointerSize));
t0 -> a4 here and in comment.

https://codereview.chromium.org/1237813002/diff/20001/src/mips64/interface-de...
File src/mips64/interface-descriptors-mips64.cc (right):

https://codereview.chromium.org/1237813002/diff/20001/src/mips64/interface-de...
src/mips64/interface-descriptors-mips64.cc:176: Register registers[] = {a0, a1,
t0, a2};
t0 ->a4 here and in comment above (line 173)

[mvstanton, 2015-07-14 20:46:14]

LGTM on the rest of the CL.

[Michael Starzinger, 2015-07-15 07:36:04]

mstarzinger@chromium.org changed reviewers:
+ rodolph.perfetta@arm.com

[Michael Starzinger, 2015-07-15 07:36:36]

Addressed comments. Thanks for all the feedback!

https://codereview.chromium.org/1237813002/diff/20001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/1237813002/diff/20001/src/arm/code-stubs-arm....
src/arm/code-stubs-arm.cc:2622: __ push(r4);
On 2015/07/14 16:57:35, jbramley wrote:
> On 2015/07/14 15:02:47, Michael Starzinger wrote:
> > Potentially applies to all architectures: Note that I realize that on some
> > architectures we could probably avoid this push and the pop a few lines
below,
> > by massaging GenerateRecordCallTarget appropriately so that it preserves the
> > register in question. However I would like to do this in a follow-up CL if
> > possible. But I am of course happy to leave a TODO for all architectures
where
> > this might apply, just let me know.
> 
> A TODO is probably a good idea, but it's up to you. On ARM64,
> GenerateRecordCallTarget takes a list of scratch registers, and that approach
> works quite well.

Done. Added a TODO here.

https://codereview.chromium.org/1237813002/diff/20001/src/arm64/code-stubs-ar...
File src/arm64/code-stubs-arm64.cc (right):

https://codereview.chromium.org/1237813002/diff/20001/src/arm64/code-stubs-ar...
src/arm64/code-stubs-arm64.cc:3004: GenerateRecordCallTarget(masm, x0, function,
x2, x3, x4, x5, x11);
On 2015/07/14 16:57:35, jbramley wrote:
> ARM64's GenerateRecordCallTarget allows the scratch registers to be specified,
> so it should be possible to just specify something other than x4 and skip the
> push/pop sequence.

Acknowledged. I initially tried that, but CallStubInRecordCallTarget still
clobbers x4. But I left a TODO here.

https://codereview.chromium.org/1237813002/diff/20001/src/arm64/full-codegen-...
File src/arm64/full-codegen-arm64.cc (right):

https://codereview.chromium.org/1237813002/diff/20001/src/arm64/full-codegen-...
src/arm64/full-codegen-arm64.cc:3093: __ mov(x4, result_register());
On 2015/07/14 16:57:35, jbramley wrote:
> We prefer the MacroAssembler's "__ Mov(...)" in ARM64, for consistency, even
> though it doesn't make any difference in this particular case. (The lower-case
> Assembler versions are usually reserved for critical sections in
> InstructionAccurateScopes.)

Done. One of these days I am gonna remember that, I promise. :)

https://codereview.chromium.org/1237813002/diff/20001/src/mips64/builtins-mip...
File src/mips64/builtins-mips64.cc (right):

https://codereview.chromium.org/1237813002/diff/20001/src/mips64/builtins-mip...
src/mips64/builtins-mips64.cc:1511: __ ld(t0, MemOperand(fp, kNewTargetOffset));
On 2015/07/14 16:58:17, paul.l... wrote:
> t0 -> a4

Done.

https://codereview.chromium.org/1237813002/diff/20001/src/mips64/code-stubs-m...
File src/mips64/code-stubs-mips64.cc (right):

https://codereview.chromium.org/1237813002/diff/20001/src/mips64/code-stubs-m...
src/mips64/code-stubs-mips64.cc:2784: // t0 : original constructor (for
IsSuperConstructorCall)
On 2015/07/14 16:58:17, paul.l... wrote:
> Please change t0 -> a4

Done.

https://codereview.chromium.org/1237813002/diff/20001/src/mips64/code-stubs-m...
src/mips64/code-stubs-mips64.cc:2790: __ Branch(&slow, ne, a4,
Operand(JS_FUNCTION_TYPE));
On 2015/07/14 16:58:18, paul.l... wrote:
> Please change these 3 uses of a4 -> a5

Done.

https://codereview.chromium.org/1237813002/diff/20001/src/mips64/code-stubs-m...
src/mips64/code-stubs-mips64.cc:2794: __ push(t0);
On 2015/07/14 16:58:17, paul.l... wrote:
> Please use a4 here and in pop() below.

Done.

https://codereview.chromium.org/1237813002/diff/20001/src/mips64/code-stubs-m...
src/mips64/code-stubs-mips64.cc:2824: __ mov(a3, t0);
On 2015/07/14 16:58:18, paul.l... wrote:
> t0 -> a4

Done.

https://codereview.chromium.org/1237813002/diff/20001/src/mips64/code-stubs-m...
src/mips64/code-stubs-mips64.cc:2839: // a4: object type
On 2015/07/14 16:58:18, paul.l... wrote:
> a4 -> a5 for object type

Done.

https://codereview.chromium.org/1237813002/diff/20001/src/mips64/code-stubs-m...
src/mips64/code-stubs-mips64.cc:2842: __ Branch(&non_function_call, ne, a4,
Operand(JS_FUNCTION_PROXY_TYPE));
On 2015/07/14 16:58:18, paul.l... wrote:
> a4 -> a5

Done.

https://codereview.chromium.org/1237813002/diff/20001/src/mips64/full-codegen...
File src/mips64/full-codegen-mips64.cc (right):

https://codereview.chromium.org/1237813002/diff/20001/src/mips64/full-codegen...
src/mips64/full-codegen-mips64.cc:3388: __ mov(t0, result_register());
On 2015/07/14 16:58:18, paul.l... wrote:
> t0 -> a4 here, and in comment above.

Done.

https://codereview.chromium.org/1237813002/diff/20001/src/mips64/full-codegen...
src/mips64/full-codegen-mips64.cc:4348: __ ld(t0, MemOperand(sp, 1 *
kPointerSize));
On 2015/07/14 16:58:18, paul.l... wrote:
> t0 -> a4 here and in comment.

Done.

https://codereview.chromium.org/1237813002/diff/20001/src/mips64/interface-de...
File src/mips64/interface-descriptors-mips64.cc (right):

https://codereview.chromium.org/1237813002/diff/20001/src/mips64/interface-de...
src/mips64/interface-descriptors-mips64.cc:176: Register registers[] = {a0, a1,
t0, a2};
On 2015/07/14 16:58:18, paul.l... wrote:
> t0 ->a4 here and in comment above (line 173)

Done.

[paul.l..., 2015-07-15 14:16:27]

MIPS part LGTM.

[martyn.capewell, 2015-07-15 14:24:14]

ARM changes LGTM.

[Michael Starzinger, 2015-07-15 14:35:00]

The CQ bit was checked by mstarzinger@chromium.org

[Michael Starzinger, 2015-07-15 14:35:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
mvstanton@chromium.org
Link to the patchset: https://codereview.chromium.org/1237813002/#ps40001
(title: "Addressed comments about ARM and MIPS.")

[commit-bot: I haz the power, 2015-07-15 14:35:13]

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

[commit-bot: I haz the power, 2015-07-15 14:37:01]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-07-15 14:37:26]

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