Change calling convention of CallApiGetterStub to accept the AccessorInfo

Change calling convention of CallApiGetterStub to accept the AccessorInfo

MIPS port contributed by Balazs Kilvady <balazs.kilvady@imgtec.com>;

Committed: https://crrev.com/d2b0a4b727f77f97960c7fa71da3431591dc959f
Cr-Commit-Position: refs/heads/master@{#35606}

[Toon Verwaest, 2016-04-15 12:46:45]

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

[Toon Verwaest, 2016-04-15 12:46:45]

@rodolph.perfetta: I have a question regarding what I'm trying to do here on
ARM. The ApiGetterStub used to get an ExternalReference/DIRECT_GETTER_CALL,
which the simulator picks up. Now I want to dynamically load the target from the
AccessorInfo object. How do I help the simulator understand that I'm trying to
call to a dynamically computed C++ address?

[Rodolph Perfetta (ARM), 2016-04-15 14:21:23]

The ExternalReference returns different addresses for functions depending
whether you are compiled for the simulator (it will point to a trampoline with
the target address inlined) or for real hardware (direct pointer). This is how
the generated code an be the same for both modes. If the address you are loading
can be stored from an ExternalReference then that would work though you would
have to reverse the redirection if you need to access the address for anything
else than a call.

Otherwise currently there are no easy way to tell the simulator the address you
are jumping to is not from the same ISA, the trampoline does that.

[Toon Verwaest, 2016-04-18 09:47:22]

rodolph: what do you think of the approach in patchset 4?

[Toon Verwaest, 2016-04-18 10:44:19]

@v8-mips-ports: could you please provide a mips port for this CL?

[balazs.kilvady, 2016-04-18 10:49:07]

We are working on it.

[Toon Verwaest, 2016-04-18 10:52:21]

Thanks!

[Toon Verwaest, 2016-04-18 10:56:56]

verwaest@chromium.org changed reviewers:
+ jkummerow@chromium.org

[Toon Verwaest, 2016-04-18 10:56:57]

Jakob: ptal

[Jakob Kummerow, 2016-04-18 13:22:35]

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

https://codereview.chromium.org/1892533004/diff/80001/src/arm64/code-stubs-ar...
src/arm64/code-stubs-arm64.cc:5916: __ Ldr(scratch3, FieldMemOperand(callback,
AccessorInfo::kNameOffset));
s/kNameOffset/kDataOffset/!

Please add a test that would have caught this.

https://codereview.chromium.org/1892533004/diff/80001/src/ia32/code-stubs-ia3...
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/1892533004/diff/80001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:5817: __ PushRoot(Heap::kUndefinedValueRootIndex); 
// ReturnValue
nit: comment is redundant (and contradictory) with previous line

https://codereview.chromium.org/1892533004/diff/80001/src/ic/handler-compiler.cc
File src/ic/handler-compiler.cc (right):

https://codereview.chromium.org/1892533004/diff/80001/src/ic/handler-compiler...
src/ic/handler-compiler.cc:364: if
(!reg.is(ApiGetterDescriptor::HolderRegister())) {
You can drop this line; MacroAssembler::Move contains the dst != src check on
all platforms.

https://codereview.chromium.org/1892533004/diff/80001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/1892533004/diff/80001/src/x64/code-stubs-x64....
src/x64/code-stubs-x64.cc:5510: // ----------- S t a t e -------------
nit: outdated comment

https://codereview.chromium.org/1892533004/diff/80001/src/x64/code-stubs-x64....
src/x64/code-stubs-x64.cc:5531: Register scratch = rax;
I can haz DCHECK(!AreAliased(...))?

[jbramley, 2016-04-18 14:46:53]

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

[jbramley, 2016-04-18 14:46:54]

On 2016/04/18 09:47:22, Toon Verwaest wrote:
> rodolph: what do you think of the approach in patchset 4?

(Rodolph's not in the office at the moment so I'll reply in his absence.)

This approach looks sensible to me.

I don't really like that the layout of AccessorInfo differs when running on the
simulator because it will complicate testing and development, but I can't think
of a reasonable way to avoid it.

https://codereview.chromium.org/1892533004/diff/80001/src/ia32/code-stubs-ia3...
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/1892533004/diff/80001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:5853: __ mov(scratch, FieldOperand(callback,
AccessorInfo::kGetterOffset));
For consistency, all platforms should use kJsGetterOffset, unless there's a good
reason not to.

[Toon Verwaest, 2016-04-18 15:10:01]

Addressed comments. There were tests for arm64; I just hadn't run them yet.
Fixed.

[Toon Verwaest, 2016-04-18 15:11:12]

@jacob: Thanks! I agree with all your points. Addressed your actionable items.

[balazs.kilvady, 2016-04-18 18:21:04]

MIPS port uploaded to: https://codereview.chromium.org/1896963002/

[Jakob Kummerow, 2016-04-19 07:34:47]

LGTM (don't forget to patch in the MIPS port)

[Toon Verwaest, 2016-04-19 08:07:53]

Description was changed from

==========
Change calling convention of CallApiGetterStub to accept the AccessorInfo

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

to

==========
Change calling convention of CallApiGetterStub to accept the AccessorInfo

MIPS port contributed by Balazs Kilvady <balazs.kilvady@imgtec.com>
==========

[Toon Verwaest, 2016-04-19 08:19:33]

The CQ bit was checked by verwaest@chromium.org

[Toon Verwaest, 2016-04-19 08:19:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
jkummerow@chromium.org
Link to the patchset: https://codereview.chromium.org/1892533004/#ps140001
(title: "Importing mips patch from 1896963002")

[commit-bot: I haz the power, 2016-04-19 08:19:42]

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

[Rodolph.Perfetta_arm.com, 2016-04-19 08:19:50]

I am out of office until the 25th of April 2016  I will reply to your message on
my return.

Please contact Martyn Capewell (martyn.capewell@arm.com) for any urgent matter.


Regards,
Rodolph.
IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium. Thank you.

[commit-bot: I haz the power, 2016-04-19 08:46:09]

Description was changed from

==========
Change calling convention of CallApiGetterStub to accept the AccessorInfo

MIPS port contributed by Balazs Kilvady <balazs.kilvady@imgtec.com>
==========

to

==========
Change calling convention of CallApiGetterStub to accept the AccessorInfo

MIPS port contributed by Balazs Kilvady <balazs.kilvady@imgtec.com>
==========

[commit-bot: I haz the power, 2016-04-19 08:46:10]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-04-19 08:46:39]

Description was changed from

==========
Change calling convention of CallApiGetterStub to accept the AccessorInfo

MIPS port contributed by Balazs Kilvady <balazs.kilvady@imgtec.com>
==========

to

==========
Change calling convention of CallApiGetterStub to accept the AccessorInfo

MIPS port contributed by Balazs Kilvady <balazs.kilvady@imgtec.com>

Committed: https://crrev.com/d2b0a4b727f77f97960c7fa71da3431591dc959f
Cr-Commit-Position: refs/heads/master@{#35606}
==========

[commit-bot: I haz the power, 2016-04-19 08:46:40]

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

[balazs.kilvady, 2016-04-19 16:49:12]

A few cctest fails on MIPS HW while they are passed on simulator. I am working
on a fix.

[Toon Verwaest, 2016-04-19 20:02:51]

Auch. Thanks for looking into this!

[Michael Hablich, 2016-04-20 07:23:11]

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

The reason for reverting is: Blocks roll:
https://codereview.chromium.org/1896103002/

Can also be seen on internal waterfall:
https://chromegw.corp.google.com/i/internal.client.v8/builders/v8_cros_perf_1....

[Toon Verwaest, 2016-04-20 08:23:34]

Due to cross-compiling my USE_SIMULATOR-based object layout doesn't work. Given
that AccessorInfo objects are deserialized per-isolate, and not as widely used
anymore, I'm going to always add the field, and update the deserializer
deserialize the redirected accessor to the regular accessor.

[Toon Verwaest, 2016-04-20 09:12:34]

In https://codereview.chromium.org/1901423002/ I'm getting rid of basically all
added USE_SIMULATOR entries