[Interpreter]: Move builtin-id from function_data to function_identifier.

[Interpreter]: Move builtin-id from function_data to function_identifier.

Functions with builtin ids can be compiled with Ignition, so it is no longer
an option to overlap the bytecode_array field with the builtin id on
the SharedFunctionInfo object. Instead overlap it with the
inferred_name, which is only used for debug and so shouldn't be required
for functions with builtin ids. This result in the inferred_name field
being renamed to function_identifier, and adding typed accessors for
inferred_name and builtin_function_id.

This is required to build the snapshot with --no-lazy.

BUG=v8:4280
LOG=N

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

[rmcilroy, 2016-03-15 13:07:38]

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

[commit-bot: I haz the power, 2016-03-15 13:07:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1801023002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1801023002/1

[rmcilroy, 2016-03-15 13:20:10]

rmcilroy@chromium.org changed reviewers:
+ mstarzinger@chromium.org, yangguo@chromium.org

[rmcilroy, 2016-03-15 13:20:10]

Michi / Yang, PTAL, thanks.

[commit-bot: I haz the power, 2016-03-15 13:24:38]

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

[commit-bot: I haz the power, 2016-03-15 13:24:39]

Dry run: Try jobs failed on following builders:
  v8_linux_arm_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel/builds/15365)
  v8_win64_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/4507)
  v8_win64_rel_ng_triggered on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng_triggered/b...)

[Michael Starzinger, 2016-03-15 14:19:13]

https://codereview.chromium.org/1801023002/diff/1/src/compiler.cc
File src/compiler.cc (left):

https://codereview.chromium.org/1801023002/diff/1/src/compiler.cc#oldcode789
src/compiler.cc:789: static bool UseIgnition(CompilationInfo* info) {
As discussed offline: We now need to guard and blacklist the three generator
builtins (i.e. kGeneratorObjectNext, kGeneratorObjectReturn and
kGeneratorObjectThrow) here.

https://codereview.chromium.org/1801023002/diff/1/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/1801023002/diff/1/src/objects-inl.h#newcode5956
src/objects-inl.h:5956: void
SharedFunctionInfo::set_builtin_function_id(BuiltinFunctionId id) {
nit: Can we add a DCHECK(function_identifier()->IsUndefined() ||
HasBuiltinFunctionId()) here?

[Michael Starzinger, 2016-03-15 14:27:27]

LGTM.

[rmcilroy, 2016-03-15 17:31:46]

https://codereview.chromium.org/1801023002/diff/1/src/compiler.cc
File src/compiler.cc (left):

https://codereview.chromium.org/1801023002/diff/1/src/compiler.cc#oldcode789
src/compiler.cc:789: static bool UseIgnition(CompilationInfo* info) {
On 2016/03/15 14:19:13, Michael Starzinger wrote:
> As discussed offline: We now need to guard and blacklist the three generator
> builtins (i.e. kGeneratorObjectNext, kGeneratorObjectReturn and
> kGeneratorObjectThrow) here.

Yup, missed this. Done.

https://codereview.chromium.org/1801023002/diff/1/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/1801023002/diff/1/src/objects-inl.h#newcode5956
src/objects-inl.h:5956: void
SharedFunctionInfo::set_builtin_function_id(BuiltinFunctionId id) {
On 2016/03/15 14:19:13, Michael Starzinger wrote:
> nit: Can we add a DCHECK(function_identifier()->IsUndefined() ||
> HasBuiltinFunctionId()) here?

As discussed offline, this doesn't work since all functions get a inferred name
even before the function is assigned a builtin id.

[Michael Starzinger, 2016-03-15 17:39:42]

https://codereview.chromium.org/1801023002/diff/1/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/1801023002/diff/1/src/objects-inl.h#newcode5956
src/objects-inl.h:5956: void
SharedFunctionInfo::set_builtin_function_id(BuiltinFunctionId id) {
On 2016/03/15 17:31:46, rmcilroy wrote:
> On 2016/03/15 14:19:13, Michael Starzinger wrote:
> > nit: Can we add a DCHECK(function_identifier()->IsUndefined() ||
> > HasBuiltinFunctionId()) here?
> 
> As discussed offline, this doesn't work since all functions get a inferred
name
> even before the function is assigned a builtin id.

Acknowledged. Works for me, thanks for clarifying.

https://codereview.chromium.org/1801023002/diff/40001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/1801023002/diff/40001/src/compiler.cc#newcode804
src/compiler.cc:804: if (IsGeneratorFunctionOrBuiltin(info)) {
nit: I am not sure combining these two flags into one function improves
readability. Me personally I would rather have them side-by-side and inlined
into the UseIgnition predicate. Because I can very well imagine a generator
implementation, that no longer requires the magical %_GeneratorNext and friends
but Ignition still not supporting yield statements. Would you be opposed to
flatten the condition out as follows?

  // TODO(4681): Generator functions are not yet supported.	
  if ((info->has_shared_info() && info->shared_info()->is_generator()) ||	
      (info->has_literal() && IsGeneratorFunction(info->literal()->kind()))) {	
    return false;	
  }

  // TODO(4681): Resuming a suspended frame is not supported.
  if (info->has_shared_info() &&
      info()->shared_info()->HasBuiltinFunctionId() &&
      (info()->shared_info()->builtin_function_id() == kGeneratorObjectNext ||
       info()->shared_info()->builtin_function_id() == kGeneratorObjectReturn ||
       info()->shared_info()->builtin_function_id() == kGeneratorObjectThrow)) {
    return false;
  }

IMHO this is way easier to read and to maintain.

[Yang, 2016-03-16 12:45:20]

LGTM

https://codereview.chromium.org/1801023002/diff/40001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/1801023002/diff/40001/src/objects-inl.h#newco...
src/objects-inl.h:5968: DCHECK(function_identifier()->IsUndefined() ||
HasBuiltinFunctionId());
Can we make this UNREACHABLE instead? This should not happen, or should it?

[rmcilroy, 2016-03-16 13:24:24]

https://codereview.chromium.org/1801023002/diff/40001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/1801023002/diff/40001/src/compiler.cc#newcode804
src/compiler.cc:804: if (IsGeneratorFunctionOrBuiltin(info)) {
On 2016/03/15 17:39:41, Michael Starzinger wrote:
> nit: I am not sure combining these two flags into one function improves
> readability. Me personally I would rather have them side-by-side and inlined
> into the UseIgnition predicate. Because I can very well imagine a generator
> implementation, that no longer requires the magical %_GeneratorNext and
friends
> but Ignition still not supporting yield statements. Would you be opposed to
> flatten the condition out as follows?
> 
>   // TODO(4681): Generator functions are not yet supported.	
>   if ((info->has_shared_info() && info->shared_info()->is_generator()) ||	
>       (info->has_literal() && IsGeneratorFunction(info->literal()->kind())))
{	
>     return false;	
>   }
> 
>   // TODO(4681): Resuming a suspended frame is not supported.
>   if (info->has_shared_info() &&
>       info()->shared_info()->HasBuiltinFunctionId() &&
>       (info()->shared_info()->builtin_function_id() == kGeneratorObjectNext ||
>        info()->shared_info()->builtin_function_id() == kGeneratorObjectReturn
||
>        info()->shared_info()->builtin_function_id() == kGeneratorObjectThrow))
{
>     return false;
>   }
> 
> IMHO this is way easier to read and to maintain.

Sure thing - I was trying to reuse the SharedFunctionInfo, but this works for
me. Done.

https://codereview.chromium.org/1801023002/diff/40001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/1801023002/diff/40001/src/objects-inl.h#newco...
src/objects-inl.h:5968: DCHECK(function_identifier()->IsUndefined() ||
HasBuiltinFunctionId());
On 2016/03/16 12:45:20, Yang wrote:
> Can we make this UNREACHABLE instead? This should not happen, or should it?

Done. Let's see what the bots say.

[rmcilroy, 2016-03-16 13:24:28]

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

[commit-bot: I haz the power, 2016-03-16 13:24:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1801023002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1801023002/60001

[commit-bot: I haz the power, 2016-03-16 13:27:47]

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

[commit-bot: I haz the power, 2016-03-16 13:27:48]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/...)
  v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)

[rmcilroy, 2016-03-16 14:18:12]

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

[commit-bot: I haz the power, 2016-03-16 14:18:16]

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

[commit-bot: I haz the power, 2016-03-16 14:38:01]

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

[commit-bot: I haz the power, 2016-03-16 14:38:02]

Dry run: Try jobs failed on following builders:
  v8_win_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/4443)
  v8_win_rel_ng_triggered on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng_triggered/bui...)

[rmcilroy, 2016-03-17 14:41:23]

https://codereview.chromium.org/1801023002/diff/80001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/1801023002/diff/80001/src/objects-inl.h#newco...
src/objects-inl.h:5968: UNREACHABLE();
Yang: This ends up getting hit in ParseLazy when the Function literal's
inferred_name is set to the shared_function_info's inferred_name
(https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/parsing/par...).
Are you OK with just removing this unreachable and returning the empty string
for these builtins?

[Yang, 2016-03-17 15:01:00]

On 2016/03/17 14:41:23, rmcilroy wrote:
> https://codereview.chromium.org/1801023002/diff/80001/src/objects-inl.h
> File src/objects-inl.h (right):
> 
>
https://codereview.chromium.org/1801023002/diff/80001/src/objects-inl.h#newco...
> src/objects-inl.h:5968: UNREACHABLE();
> Yang: This ends up getting hit in ParseLazy when the Function literal's
> inferred_name is set to the shared_function_info's inferred_name
>
(https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/parsing/par...).
> Are you OK with just removing this unreachable and returning the empty string
> for these builtins?

I see. LGTM.

[rmcilroy, 2016-03-17 16:14:30]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2016-03-17 16:14:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org, yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/1801023002/#ps100001
(title: "Remove UNREACHABLE()")

[commit-bot: I haz the power, 2016-03-17 16:14:35]

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

[commit-bot: I haz the power, 2016-03-17 16:37:44]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-03-17 16:38:05]

Description was changed from

==========
[Interpreter]: Move builtin-id from function_data to function_identifier.

Functions with builtin ids can be compiled with Ignition, so it is no longer
an option to overlap the bytecode_array field with the builtin id on
the SharedFunctionInfo object. Instead overlap it with the
inferred_name, which is only used for debug and so shouldn't be required
for functions with builtin ids. This result in the inferred_name field
being renamed to function_identifier, and adding typed accessors for
inferred_name and builtin_function_id.

This is required to build the snapshot with --no-lazy.

BUG=v8:4280
LOG=N
==========

to

==========
[Interpreter]: Move builtin-id from function_data to function_identifier.

Functions with builtin ids can be compiled with Ignition, so it is no longer
an option to overlap the bytecode_array field with the builtin id on
the SharedFunctionInfo object. Instead overlap it with the
inferred_name, which is only used for debug and so shouldn't be required
for functions with builtin ids. This result in the inferred_name field
being renamed to function_identifier, and adding typed accessors for
inferred_name and builtin_function_id.

This is required to build the snapshot with --no-lazy.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/6bdb9705122a93ade754c663c1907f5778d3abd6
Cr-Commit-Position: refs/heads/master@{#34867}
==========

[commit-bot: I haz the power, 2016-03-17 16:38:08]

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