Write barrier for storing a code entry, and usage in CompileLazy builtin.

Write barrier for storing a code entry, and usage in CompileLazy builtin.

BUG=

Committed: https://crrev.com/477e133698aa2f0a40643f316c31d17347be2de7
Cr-Commit-Position: refs/heads/master@{#33718}

[mvstanton, 2016-01-29 12:47:40]

mvstanton@chromium.org changed reviewers:
+ bmeurer@chromium.org, ulan@chromium.org

[mvstanton, 2016-01-29 12:47:40]

Hi guys,
I only have ia32 and x64 right now.
It repairs a write barrier assert hit running the game boy benchmark. Working on
a test, and then ports.

Ulan, can you have a look at the new macro assembler instruction to verify it
matches what we talked about?
Thanks!
--Mike

[Benedikt Meurer, 2016-01-29 12:55:09]

It's a bit unfortunate that we need this at all, but seems easy enough to remove
later. So LGTM from my side.

[ulan, 2016-02-01 17:20:50]

Looking good.

https://codereview.chromium.org/1647123002/diff/1/src/ia32/macro-assembler-ia...
File src/ia32/macro-assembler-ia32.cc (right):

https://codereview.chromium.org/1647123002/diff/1/src/ia32/macro-assembler-ia...
src/ia32/macro-assembler-ia32.cc:576: Register dst) {
Let's keep argument names consistent with the header file (dst -> scratch).

https://codereview.chromium.org/1647123002/diff/1/src/ia32/macro-assembler-ia...
src/ia32/macro-assembler-ia32.cc:614: push(code_entry);
At this point the code_entry is already clobbered by CheckPageFlag.

https://codereview.chromium.org/1647123002/diff/1/src/ia32/macro-assembler-ia...
src/ia32/macro-assembler-ia32.cc:617: if (!js_function.is(eax) &&
!code_entry.is(eax)) push(eax);
Since this sequence is now used in two places. Should we extract it into
separate macro asm instructions PushCallerSaved, PopCallerSaved?

[mvstanton, 2016-02-03 14:23:16]

mvstanton@chromium.org changed reviewers:
+ yangguo@chromium.org

[mvstanton, 2016-02-03 14:23:17]

Hi guys, Ulan, I've responded to your comments and made the ports.

There is a test with a TODO(mvstanton) at one line that will have to change when
the optimized code map lookup moves to the compile-lazy builtin (next CL).

Yang, I think the serialize change is pretty boilerplate, can you have a look?
Thanks again,
--Mike

https://codereview.chromium.org/1647123002/diff/1/src/ia32/macro-assembler-ia...
File src/ia32/macro-assembler-ia32.cc (right):

https://codereview.chromium.org/1647123002/diff/1/src/ia32/macro-assembler-ia...
src/ia32/macro-assembler-ia32.cc:576: Register dst) {
On 2016/02/01 17:20:49, ulan wrote:
> Let's keep argument names consistent with the header file (dst -> scratch).

Done.

https://codereview.chromium.org/1647123002/diff/1/src/ia32/macro-assembler-ia...
src/ia32/macro-assembler-ia32.cc:614: push(code_entry);
On 2016/02/01 17:20:49, ulan wrote:
> At this point the code_entry is already clobbered by CheckPageFlag.

I changed things so that we don't clobber code_entry anymore...it's useful at
the call site.

https://codereview.chromium.org/1647123002/diff/1/src/ia32/macro-assembler-ia...
src/ia32/macro-assembler-ia32.cc:617: if (!js_function.is(eax) &&
!code_entry.is(eax)) push(eax);
On 2016/02/01 17:20:49, ulan wrote:
> Since this sequence is now used in two places. Should we extract it into
> separate macro asm instructions PushCallerSaved, PopCallerSaved?

Absolutely, done.

[Yang, 2016-02-03 14:25:49]

On 2016/02/03 14:23:17, mvstanton wrote:
> Hi guys, Ulan, I've responded to your comments and made the ports.
> 
> There is a test with a TODO(mvstanton) at one line that will have to change
when
> the optimized code map lookup moves to the compile-lazy builtin (next CL).
> 
> Yang, I think the serialize change is pretty boilerplate, can you have a look?
> Thanks again,
> --Mike
> 
>
https://codereview.chromium.org/1647123002/diff/1/src/ia32/macro-assembler-ia...
> File src/ia32/macro-assembler-ia32.cc (right):
> 
>
https://codereview.chromium.org/1647123002/diff/1/src/ia32/macro-assembler-ia...
> src/ia32/macro-assembler-ia32.cc:576: Register dst) {
> On 2016/02/01 17:20:49, ulan wrote:
> > Let's keep argument names consistent with the header file (dst -> scratch).
> 
> Done.
> 
>
https://codereview.chromium.org/1647123002/diff/1/src/ia32/macro-assembler-ia...
> src/ia32/macro-assembler-ia32.cc:614: push(code_entry);
> On 2016/02/01 17:20:49, ulan wrote:
> > At this point the code_entry is already clobbered by CheckPageFlag.
> 
> I changed things so that we don't clobber code_entry anymore...it's useful at
> the call site.
> 
>
https://codereview.chromium.org/1647123002/diff/1/src/ia32/macro-assembler-ia...
> src/ia32/macro-assembler-ia32.cc:617: if (!js_function.is(eax) &&
> !code_entry.is(eax)) push(eax);
> On 2016/02/01 17:20:49, ulan wrote:
> > Since this sequence is now used in two places. Should we extract it into
> > separate macro asm instructions PushCallerSaved, PopCallerSaved?
> 
> Absolutely, done.

serializer lgtm.

[ulan, 2016-02-04 07:00:52]

LGTM with nits.

https://codereview.chromium.org/1647123002/diff/60001/src/arm/macro-assembler...
File src/arm/macro-assembler-arm.cc (right):

https://codereview.chromium.org/1647123002/diff/60001/src/arm/macro-assembler...
src/arm/macro-assembler-arm.cc:691: DCHECK(kCallerSaved & js_function.bit());
Let's use comparison with 0 as one below to be consistent.

https://codereview.chromium.org/1647123002/diff/60001/src/arm/macro-assembler...
src/arm/macro-assembler-arm.cc:692: DCHECK((kCallerSaved & code_entry.bit()) ==
0);
DCHECK_EQ

https://codereview.chromium.org/1647123002/diff/60001/src/x64/macro-assembler...
File src/x64/macro-assembler-x64.cc (right):

https://codereview.chromium.org/1647123002/diff/60001/src/x64/macro-assembler...
src/x64/macro-assembler-x64.cc:556: DCHECK(js_function.is(rdi));
Let's move this up, to make it clear that it is a precondition of the function.

https://codereview.chromium.org/1647123002/diff/60001/src/x64/macro-assembler...
src/x64/macro-assembler-x64.cc:567: // AMD64 calling convention.
no need for "if", just DCHECK(arg_reg_1.is(rdi));

[mvstanton, 2016-02-04 07:46:48]

The CQ bit was checked by mvstanton@chromium.org

[mvstanton, 2016-02-04 07:46:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, ulan@chromium.org, yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/1647123002/#ps80001
(title: "REBASE and comment response.")

[commit-bot: I haz the power, 2016-02-04 07:46:54]

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

[mvstanton, 2016-02-04 07:47:22]

Thanks for the comments, Ulan. Submittin'.

https://codereview.chromium.org/1647123002/diff/60001/src/arm/macro-assembler...
File src/arm/macro-assembler-arm.cc (right):

https://codereview.chromium.org/1647123002/diff/60001/src/arm/macro-assembler...
src/arm/macro-assembler-arm.cc:691: DCHECK(kCallerSaved & js_function.bit());
On 2016/02/04 07:00:52, ulan wrote:
> Let's use comparison with 0 as one below to be consistent.

Done.

https://codereview.chromium.org/1647123002/diff/60001/src/arm/macro-assembler...
src/arm/macro-assembler-arm.cc:692: DCHECK((kCallerSaved & code_entry.bit()) ==
0);
On 2016/02/04 07:00:52, ulan wrote:
> DCHECK_EQ

Done.

https://codereview.chromium.org/1647123002/diff/60001/src/x64/macro-assembler...
File src/x64/macro-assembler-x64.cc (right):

https://codereview.chromium.org/1647123002/diff/60001/src/x64/macro-assembler...
src/x64/macro-assembler-x64.cc:556: DCHECK(js_function.is(rdi));
On 2016/02/04 07:00:52, ulan wrote:
> Let's move this up, to make it clear that it is a precondition of the
function.

Done.

https://codereview.chromium.org/1647123002/diff/60001/src/x64/macro-assembler...
src/x64/macro-assembler-x64.cc:567: // AMD64 calling convention.
On 2016/02/04 07:00:52, ulan wrote:
> no need for "if", just DCHECK(arg_reg_1.is(rdi));

Done.

[commit-bot: I haz the power, 2016-02-04 08:16:01]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-02-04 08:16:23]

Description was changed from

==========
Write barrier for storing a code entry, and usage in CompileLazy builtin.

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

to

==========
Write barrier for storing a code entry, and usage in CompileLazy builtin.

BUG=

Committed: https://crrev.com/477e133698aa2f0a40643f316c31d17347be2de7
Cr-Commit-Position: refs/heads/master@{#33718}
==========

[commit-bot: I haz the power, 2016-02-04 08:16:24]

Patchset 5 (id:??) landed as
https://crrev.com/477e133698aa2f0a40643f316c31d17347be2de7
Cr-Commit-Position: refs/heads/master@{#33718}