[ast] Annotate CallRuntime ast node

[ast] Annotate Runtime ast nodes

Changes output from
CALL RUNTIME (context function) code = 0x3e9ea90a2049 at -1
to
CALL RUNTIME async_function_promise_create code = 0x3e9ea90a2049 at -1

This makes the ast more useful. I didn't annotate all the runtime calls, 
only some for now. We can annotate others if necessary.

Review-Url: https://codereview.chromium.org/2654113002
Cr-Commit-Position: refs/heads/master@{#42671}
Committed: https://chromium.googlesource.com/v8/v8/+/c6f925d7ed002e4bbd57a674cf90627384c81964

[gsathya, 2017-01-25 17:23:06]

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

[commit-bot: I haz the power, 2017-01-25 17:23:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gsathya, 2017-01-25 17:24:03]

Description was changed from

==========
[ast] Annotate Runtime ast nodes

Changes output from
CALL RUNTIME (context function) code = 0x3e9ea90a2049 at -1
to
CALL RUNTIME (context function) name = AsyncFunctionPromiseCreate code =
0x3e9ea90a2049 at -1

This makes the ast more useful
==========

to

==========
[ast] Annotate Runtime ast nodes

Changes output from
CALL RUNTIME (context function) code = 0x3e9ea90a2049 at -1
to
CALL RUNTIME (context function) name = AsyncFunctionPromiseCreate code =
0x3e9ea90a2049 at -1

This makes the ast more useful. I didn't annotate all the runtime calls, only
some for now. We can annotate others if necessary.
==========

[gsathya, 2017-01-25 17:24:03]

gsathya@chromium.org changed reviewers:
+ adamk@chromium.org, neis@chromium.org

[gsathya, 2017-01-25 17:24:16]

Description was changed from

==========
[ast] Annotate Runtime ast nodes

Changes output from
CALL RUNTIME (context function) code = 0x3e9ea90a2049 at -1
to
CALL RUNTIME (context function) name = AsyncFunctionPromiseCreate code =
0x3e9ea90a2049 at -1

This makes the ast more useful. I didn't annotate all the runtime calls, only
some for now. We can annotate others if necessary.
==========

to

==========
[ast] Annotate Runtime ast nodes

Changes output from
CALL RUNTIME (context function) code = 0x3e9ea90a2049 at -1
to
CALL RUNTIME (context function) name = AsyncFunctionPromiseCreate code =
0x3e9ea90a2049 at -1

This makes the ast more useful. I didn't annotate all the runtime calls, 
only some for now. We can annotate others if necessary.
==========

[commit-bot: I haz the power, 2017-01-25 17:54:26]

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

[commit-bot: I haz the power, 2017-01-25 17:54:27]

Dry run: This issue passed the CQ dry run.

[gsathya, 2017-01-25 18:30:48]

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

[commit-bot: I haz the power, 2017-01-25 18:30:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 19:02:54]

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

[commit-bot: I haz the power, 2017-01-25 19:02:55]

Dry run: This issue passed the CQ dry run.

[adamk, 2017-01-25 19:23:54]

I like the idea, but I don't think we should spend any AST memory on this.
Caitlin did something almost exactly like this in
https://codereview.chromium.org/2593823002 for the bytecode golden files. Could
you move the NameForNativeContextIntrinsicIndex() from that CL to somewhere
commonly accessible, and update CallRuntime::debug_name() to use it?

[gsathya, 2017-01-25 20:00:35]

Patchset #3 (id:40001) has been deleted

[gsathya, 2017-01-25 20:00:39]

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

[commit-bot: I haz the power, 2017-01-25 20:00:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gsathya, 2017-01-25 20:01:05]

Description was changed from

==========
[ast] Annotate Runtime ast nodes

Changes output from
CALL RUNTIME (context function) code = 0x3e9ea90a2049 at -1
to
CALL RUNTIME (context function) name = AsyncFunctionPromiseCreate code =
0x3e9ea90a2049 at -1

This makes the ast more useful. I didn't annotate all the runtime calls, 
only some for now. We can annotate others if necessary.
==========

to

==========
[ast] Annotate Runtime ast nodes

Changes output from
CALL RUNTIME (context function) code = 0x3e9ea90a2049 at -1
to
CALL RUNTIME async_function_promise_create code = 0x3e9ea90a2049 at -1

This makes the ast more useful. I didn't annotate all the runtime calls, 
only some for now. We can annotate others if necessary.
==========

[gsathya, 2017-01-25 20:04:15]

On 2017/01/25 19:23:54, adamk wrote:
> I like the idea, but I don't think we should spend any AST memory on this.
> Caitlin did something almost exactly like this in
> https://codereview.chromium.org/2593823002 for the bytecode golden files.
Could
> you move the NameForNativeContextIntrinsicIndex() from that CL to somewhere
> commonly accessible, and update CallRuntime::debug_name() to use it?

I just copied NameForNativeContextIntrinsicIndex over to ast.h because 1) build
the switch statement only for debug build (--print-ast is debug only) 2) the
test version used only a subset of the context fields

[adamk, 2017-01-25 20:26:43]

https://codereview.chromium.org/2654113002/diff/50001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2654113002/diff/50001/src/ast/ast.h#newcode115
src/ast/ast.h:115: static const char*
NameForNativeContextIntrinsicIndex(uint32_t idx) {
Rather than copying this, what about exposing it in contexts.h? If we go that
route, I'd also suggest moving the implementation of CallRuntime::debug_name
into ast.cc.

[commit-bot: I haz the power, 2017-01-25 20:33:23]

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

[commit-bot: I haz the power, 2017-01-25 20:33:24]

Dry run: This issue passed the CQ dry run.

[gsathya, 2017-01-25 21:05:19]

https://codereview.chromium.org/2654113002/diff/50001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2654113002/diff/50001/src/ast/ast.h#newcode115
src/ast/ast.h:115: static const char*
NameForNativeContextIntrinsicIndex(uint32_t idx) {
On 2017/01/25 20:26:43, adamk wrote:
> Rather than copying this, what about exposing it in contexts.h? If we go that
> route, I'd also suggest moving the implementation of CallRuntime::debug_name
> into ast.cc.

Why is moving to contexts.h better?

[adamk, 2017-01-25 21:11:18]

https://codereview.chromium.org/2654113002/diff/50001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2654113002/diff/50001/src/ast/ast.h#newcode115
src/ast/ast.h:115: static const char*
NameForNativeContextIntrinsicIndex(uint32_t idx) {
On 2017/01/25 21:05:19, gsathya wrote:
> On 2017/01/25 20:26:43, adamk wrote:
> > Rather than copying this, what about exposing it in contexts.h? If we go
that
> > route, I'd also suggest moving the implementation of CallRuntime::debug_name
> > into ast.cc.
> 
> Why is moving to contexts.h better?

I'd rather not have two copies of this function. Maybe I misunderstood the
reason for putting this here: is this not exactly the same as what's in caitp's
patch?

[gsathya, 2017-01-25 21:17:17]

https://codereview.chromium.org/2654113002/diff/50001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2654113002/diff/50001/src/ast/ast.h#newcode115
src/ast/ast.h:115: static const char*
NameForNativeContextIntrinsicIndex(uint32_t idx) {
On 2017/01/25 21:11:17, adamk wrote:
> On 2017/01/25 21:05:19, gsathya wrote:
> > On 2017/01/25 20:26:43, adamk wrote:
> > > Rather than copying this, what about exposing it in contexts.h? If we go
> that
> > > route, I'd also suggest moving the implementation of
CallRuntime::debug_name
> > > into ast.cc.
> > 
> > Why is moving to contexts.h better?
> 
> I'd rather not have two copies of this function. Maybe I misunderstood the
> reason for putting this here: is this not exactly the same as what's in
caitp's
> patch?

It's not exactly the same. As mentioned in my previous comment, caitp's version
uses only a subset of the native context fields (only the intrinsics). It's fine
to merge them though. Also as mentioned in my previous comment, what about the
#ifdef DEBUG? I dont see why we need this for release builds. I can't reuse this
in the test if it's debug only.

[adamk, 2017-01-25 23:13:58]

https://codereview.chromium.org/2654113002/diff/50001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2654113002/diff/50001/src/ast/ast.h#newcode115
src/ast/ast.h:115: static const char*
NameForNativeContextIntrinsicIndex(uint32_t idx) {
On 2017/01/25 21:17:17, gsathya wrote:
> On 2017/01/25 21:11:17, adamk wrote:
> > On 2017/01/25 21:05:19, gsathya wrote:
> > > On 2017/01/25 20:26:43, adamk wrote:
> > > > Rather than copying this, what about exposing it in contexts.h? If we go
> > that
> > > > route, I'd also suggest moving the implementation of
> CallRuntime::debug_name
> > > > into ast.cc.
> > > 
> > > Why is moving to contexts.h better?
> > 
> > I'd rather not have two copies of this function. Maybe I misunderstood the
> > reason for putting this here: is this not exactly the same as what's in
> caitp's
> > patch?
> 
> It's not exactly the same. As mentioned in my previous comment, caitp's
version
> uses only a subset of the native context fields (only the intrinsics). It's
fine
> to merge them though. Also as mentioned in my previous comment, what about the
> #ifdef DEBUG? I dont see why we need this for release builds. I can't reuse
this
> in the test if it's debug only.

Sorry, missed the details of the macros. I'm ok with the duplication now, but
I'd prefer if this code lived in ast.cc (just to keep implementation details
hidden).

[gsathya, 2017-01-25 23:26:54]

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

[commit-bot: I haz the power, 2017-01-25 23:27:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gsathya, 2017-01-25 23:27:25]

https://codereview.chromium.org/2654113002/diff/50001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2654113002/diff/50001/src/ast/ast.h#newcode115
src/ast/ast.h:115: static const char*
NameForNativeContextIntrinsicIndex(uint32_t idx) {
On 2017/01/25 23:13:58, adamk wrote:
> On 2017/01/25 21:17:17, gsathya wrote:
> > On 2017/01/25 21:11:17, adamk wrote:
> > > On 2017/01/25 21:05:19, gsathya wrote:
> > > > On 2017/01/25 20:26:43, adamk wrote:
> > > > > Rather than copying this, what about exposing it in contexts.h? If we
go
> > > that
> > > > > route, I'd also suggest moving the implementation of
> > CallRuntime::debug_name
> > > > > into ast.cc.
> > > > 
> > > > Why is moving to contexts.h better?
> > > 
> > > I'd rather not have two copies of this function. Maybe I misunderstood the
> > > reason for putting this here: is this not exactly the same as what's in
> > caitp's
> > > patch?
> > 
> > It's not exactly the same. As mentioned in my previous comment, caitp's
> version
> > uses only a subset of the native context fields (only the intrinsics). It's
> fine
> > to merge them though. Also as mentioned in my previous comment, what about
the
> > #ifdef DEBUG? I dont see why we need this for release builds. I can't reuse
> this
> > in the test if it's debug only.
> 
> Sorry, missed the details of the macros. I'm ok with the duplication now, but
> I'd prefer if this code lived in ast.cc (just to keep implementation details
> hidden).

Done.

[adamk, 2017-01-25 23:31:23]

lgtm

[commit-bot: I haz the power, 2017-01-25 23:58:07]

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

[commit-bot: I haz the power, 2017-01-25 23:58:08]

Dry run: This issue passed the CQ dry run.

[gsathya, 2017-01-26 00:00:03]

The CQ bit was checked by gsathya@chromium.org

[commit-bot: I haz the power, 2017-01-26 00:00:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-26 00:02:03]

CQ is committing da patch.

Bot data: {"patchset_id": 70001, "attempt_start_ts": 1485388803893540,
"parent_rev": "bc7eb04d925fa3d4b07c163f6708808177c6c843", "commit_rev":
"c6f925d7ed002e4bbd57a674cf90627384c81964"}

[commit-bot: I haz the power, 2017-01-26 00:02:08]

Description was changed from

==========
[ast] Annotate Runtime ast nodes

Changes output from
CALL RUNTIME (context function) code = 0x3e9ea90a2049 at -1
to
CALL RUNTIME async_function_promise_create code = 0x3e9ea90a2049 at -1

This makes the ast more useful. I didn't annotate all the runtime calls, 
only some for now. We can annotate others if necessary.
==========

to

==========
[ast] Annotate Runtime ast nodes

Changes output from
CALL RUNTIME (context function) code = 0x3e9ea90a2049 at -1
to
CALL RUNTIME async_function_promise_create code = 0x3e9ea90a2049 at -1

This makes the ast more useful. I didn't annotate all the runtime calls, 
only some for now. We can annotate others if necessary.

Review-Url: https://codereview.chromium.org/2654113002
Cr-Commit-Position: refs/heads/master@{#42671}
Committed:
https://chromium.googlesource.com/v8/v8/+/c6f925d7ed002e4bbd57a674cf90627384c...
==========

[commit-bot: I haz the power, 2017-01-26 00:02:09]

Committed patchset #4 (id:70001) as
https://chromium.googlesource.com/v8/v8/+/c6f925d7ed002e4bbd57a674cf90627384c...

[neis, 2017-01-26 08:04:53]

Nice!

[caitp, 2017-01-29 05:32:33]

caitp@igalia.com changed reviewers:
+ caitp@igalia.com

[caitp, 2017-01-29 05:32:33]

https://codereview.chromium.org/2654113002/diff/70001/src/ast/ast.cc
File src/ast/ast.cc (right):

https://codereview.chromium.org/2654113002/diff/70001/src/ast/ast.cc#newcode1084
src/ast/ast.cc:1084: return NameForNativeContextIntrinsicIndex(context_index_);
Hey, sorry I didn't catch this sooner. This makes intrinsics not installed on
the native context print the useless "UnknownIntrinsicIndex".

This needs a `is_jsruntime()` predicate like in the release build case below