Cleanup Generate_JSConstructStubHelper a bit.

Cleanup Generate_JSConstructStubHelper a bit.

R=bmeurer@chromium.org

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

[Michael Starzinger, 2015-07-10 09:29:24]

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

[Michael Starzinger, 2015-07-10 09:29:24]

Architecture ports are still in the works, but feel free to shoot comments my
way already.
Benedikt: PTAL.
Arv: FYI.

[Benedikt Meurer, 2015-07-10 11:18:33]

LGTM, love it.

[arv (Not doing code reviews), 2015-07-10 16:21:52]

Nice, LGTM

https://codereview.chromium.org/1227163011/diff/20001/src/x64/builtins-x64.cc
File src/x64/builtins-x64.cc (right):

https://codereview.chromium.org/1227163011/diff/20001/src/x64/builtins-x64.cc...
src/x64/builtins-x64.cc:181: __ Push(rdx);
Do we care about the extra cost of the push/pop here?

[Michael Starzinger, 2015-07-10 16:39:54]

https://codereview.chromium.org/1227163011/diff/20001/src/x64/builtins-x64.cc
File src/x64/builtins-x64.cc (right):

https://codereview.chromium.org/1227163011/diff/20001/src/x64/builtins-x64.cc...
src/x64/builtins-x64.cc:181: __ Push(rdx);
On 2015/07/10 16:21:51, arv (Not doing code reviews) wrote:
> Do we care about the extra cost of the push/pop here?

The push/pop is right before and after a runtime call, which in turn goes
through the CEntryStub that sets up and tears down an entire exit-frame. Also,
slack tracking runs for exactly 8 allocations per per initial map. Also,
Runtime_FinalizeInstanceSize will perform a full traversal over the entire
transition tree of the initial map. So no, I don't think we care.

[Michael Starzinger, 2015-07-13 10:57:29]

Ported to all architectures.

[Michael Starzinger, 2015-07-13 12:10:09]

mstarzinger@chromium.org changed reviewers:
+ balazs.kilvady@imgtec.com

[Michael Starzinger, 2015-07-13 12:10:09]

+Balazs

[Michael Starzinger, 2015-07-13 12:27:13]

mstarzinger@chromium.org changed reviewers:
+ jacob.bramley@arm.com

[Michael Starzinger, 2015-07-13 12:27:14]

+Jacob (about the ARM ports)

[jbramley, 2015-07-13 12:46:53]

I realise that some code that I have commented on was just moved from
Generate_Runtime_NewObject. If you'd rather not get distracted with it now, just
let me know and I'll make a follow-up patch to clean it.

https://codereview.chromium.org/1227163011/diff/40001/src/arm/builtins-arm.cc
File src/arm/builtins-arm.cc (right):

https://codereview.chromium.org/1227163011/diff/40001/src/arm/builtins-arm.cc...
src/arm/builtins-arm.cc:357: // Original constructor and function are different.
This comment is now out of place. Consider something like this: "Fall back to
runtime if the original constructor and function are different."

https://codereview.chromium.org/1227163011/diff/40001/src/arm/builtins-arm.cc...
src/arm/builtins-arm.cc:497: __ ldr(r3, MemOperand(sp, 0 * kPointerSize));
`MemOperand(sp)` would do here.

https://codereview.chromium.org/1227163011/diff/40001/src/arm64/builtins-arm6...
File src/arm64/builtins-arm64.cc (right):

https://codereview.chromium.org/1227163011/diff/40001/src/arm64/builtins-arm6...
src/arm64/builtins-arm64.cc:354: // Original constructor and function are
different.
Same comment as for ARM.

https://codereview.chromium.org/1227163011/diff/40001/src/arm64/builtins-arm6...
src/arm64/builtins-arm64.cc:505: __ bind(&rt_call_reload_new_target);
For ARM64, we use the MacroAssembler's "Bind" for consistency, unless in a
critical section where we're using the Assembler directly.

https://codereview.chromium.org/1227163011/diff/40001/src/arm64/builtins-arm6...
src/arm64/builtins-arm64.cc:519: __ Push(original_constructor);  // argument 3:
original constructor
For ARM64, it's preferable to merge pushes wherever possible. Each call to Push
has a stack-maintenance overhead. The arguments to Push(...) are ordered the
same as if they were listed separately. In this case:

__ Push(x4, constructor, original_constructor);

https://codereview.chromium.org/1227163011/diff/40001/src/arm64/builtins-arm6...
src/arm64/builtins-arm64.cc:525: __ jmp(&count_incremented);
I think we'd normally use "__ B(&label)". ARM64 doesn't have a "jmp" instruction
but it exists in the assembler for compatibility with other bits of V8.

https://codereview.chromium.org/1227163011/diff/40001/src/arm64/builtins-arm6...
src/arm64/builtins-arm64.cc:528: __ Push(original_constructor);  // argument 2:
original constructor
Merge the pushes again.

[balazs.kilvady, 2015-07-13 13:02:31]

lgtm with minor nit(s)

https://codereview.chromium.org/1227163011/diff/40001/src/mips/builtins-mips.cc
File src/mips/builtins-mips.cc (right):

https://codereview.chromium.org/1227163011/diff/40001/src/mips/builtins-mips....
src/mips/builtins-mips.cc:513: __ push(a3);  // argument 3/2: original
constructor
I think Push(a1, a3) could be used here instead of 2 push()-es (also on arm?)

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

https://codereview.chromium.org/1227163011/diff/40001/src/mips64/builtins-mip...
src/mips64/builtins-mips64.cc:517: __ push(a3);  // argument 3/2: original
constructor
I think Push(a1, a3) could be used here instead of 2 push()-es (also on arm64?)

[Michael Starzinger, 2015-07-13 13:05:52]

Addressed comments. Thanks!

https://codereview.chromium.org/1227163011/diff/40001/src/arm/builtins-arm.cc
File src/arm/builtins-arm.cc (right):

https://codereview.chromium.org/1227163011/diff/40001/src/arm/builtins-arm.cc...
src/arm/builtins-arm.cc:357: // Original constructor and function are different.
On 2015/07/13 12:46:52, jbramley wrote:
> This comment is now out of place. Consider something like this: "Fall back to
> runtime if the original constructor and function are different."

Done. This applies to all architectures. Now that you mention it, I agree that
the formulation might be misleading. Fixed for all of them.

https://codereview.chromium.org/1227163011/diff/40001/src/arm/builtins-arm.cc...
src/arm/builtins-arm.cc:497: __ ldr(r3, MemOperand(sp, 0 * kPointerSize));
On 2015/07/13 12:46:52, jbramley wrote:
> `MemOperand(sp)` would do here.

Acknowledged. Hmm, I thought having this explicit would increase readability.
Also we already do this in Generate_NotifyDeoptimizedHelper and a couple of
asserts. But I don't feel strongly about this.

https://codereview.chromium.org/1227163011/diff/40001/src/arm64/builtins-arm6...
File src/arm64/builtins-arm64.cc (right):

https://codereview.chromium.org/1227163011/diff/40001/src/arm64/builtins-arm6...
src/arm64/builtins-arm64.cc:354: // Original constructor and function are
different.
On 2015/07/13 12:46:52, jbramley wrote:
> Same comment as for ARM.

Done. Same on all architectures.

https://codereview.chromium.org/1227163011/diff/40001/src/arm64/builtins-arm6...
src/arm64/builtins-arm64.cc:505: __ bind(&rt_call_reload_new_target);
On 2015/07/13 12:46:52, jbramley wrote:
> For ARM64, we use the MacroAssembler's "Bind" for consistency, unless in a
> critical section where we're using the Assembler directly.

Done.

https://codereview.chromium.org/1227163011/diff/40001/src/arm64/builtins-arm6...
src/arm64/builtins-arm64.cc:519: __ Push(original_constructor);  // argument 3:
original constructor
On 2015/07/13 12:46:52, jbramley wrote:
> For ARM64, it's preferable to merge pushes wherever possible. Each call to
Push
> has a stack-maintenance overhead. The arguments to Push(...) are ordered the
> same as if they were listed separately. In this case:
> 
> __ Push(x4, constructor, original_constructor);

Done.

https://codereview.chromium.org/1227163011/diff/40001/src/arm64/builtins-arm6...
src/arm64/builtins-arm64.cc:525: __ jmp(&count_incremented);
On 2015/07/13 12:46:53, jbramley wrote:
> I think we'd normally use "__ B(&label)". ARM64 doesn't have a "jmp"
instruction
> but it exists in the assembler for compatibility with other bits of V8.

Done.

https://codereview.chromium.org/1227163011/diff/40001/src/arm64/builtins-arm6...
src/arm64/builtins-arm64.cc:528: __ Push(original_constructor);  // argument 2:
original constructor
On 2015/07/13 12:46:52, jbramley wrote:
> Merge the pushes again.

Done.

[jbramley, 2015-07-13 13:10:51]

LGTM, thanks!

https://codereview.chromium.org/1227163011/diff/40001/src/arm/builtins-arm.cc
File src/arm/builtins-arm.cc (right):

https://codereview.chromium.org/1227163011/diff/40001/src/arm/builtins-arm.cc...
src/arm/builtins-arm.cc:497: __ ldr(r3, MemOperand(sp, 0 * kPointerSize));
On 2015/07/13 13:05:52, Michael Starzinger wrote:
> Acknowledged. Hmm, I thought having this explicit would increase readability.
> Also we already do this in Generate_NotifyDeoptimizedHelper and a couple of
> asserts. But I don't feel strongly about this.

Ok, I'm not really worried either way.

[Michael Starzinger, 2015-07-13 13:11:55]

Addressed comments. Thanks! Landing.

https://codereview.chromium.org/1227163011/diff/40001/src/mips/builtins-mips.cc
File src/mips/builtins-mips.cc (right):

https://codereview.chromium.org/1227163011/diff/40001/src/mips/builtins-mips....
src/mips/builtins-mips.cc:513: __ push(a3);  // argument 3/2: original
constructor
On 2015/07/13 13:02:31, balazs.kilvady wrote:
> I think Push(a1, a3) could be used here instead of 2 push()-es (also on arm?)

Done.

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

https://codereview.chromium.org/1227163011/diff/40001/src/mips64/builtins-mip...
src/mips64/builtins-mips64.cc:517: __ push(a3);  // argument 3/2: original
constructor
On 2015/07/13 13:02:31, balazs.kilvady wrote:
> I think Push(a1, a3) could be used here instead of 2 push()-es (also on
arm64?)

Done.

[jbramley, 2015-07-13 13:18:43]

On 2015/07/13 13:02:31, balazs.kilvady wrote:
>
https://codereview.chromium.org/1227163011/diff/40001/src/mips/builtins-mips....
> src/mips/builtins-mips.cc:513: __ push(a3);  // argument 3/2: original
> constructor
> I think Push(a1, a3) could be used here instead of 2 push()-es (also on arm?)

It would make sense using the ISA, but the ARM back-end doesn't seem to have a
multiple-argument `push`.

[Michael Starzinger, 2015-07-13 15:11:23]

The CQ bit was checked by mstarzinger@chromium.org

[Michael Starzinger, 2015-07-13 15:11:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, arv@chromium.org, balazs.kilvady@imgtec.com,
jacob.bramley@arm.com
Link to the patchset: https://codereview.chromium.org/1227163011/#ps100001
(title: "Comment typo.")

[commit-bot: I haz the power, 2015-07-13 15:11:41]

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

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

Committed patchset #6 (id:100001)

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

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