[compiler] Make SFI "optimize" flag a "tier up" flag

[compiler] Make SFI "optimize" flag a "tier up" flag

Reuses (and renames) the SFI "mark for optimization" flag to also permit
marking for baseline recompilation. The flag now represents a "tier up"
request, and CompileLazy can get baseline code as well as optimized
code.

BUG=v8:5512
Committed: https://crrev.com/a5ed05aa8b38a57f9c51f76faf938e053fa6d9a8
Cr-Commit-Position: refs/heads/master@{#40612}

[Leszek Swirski, 2016-10-25 09:48:12]

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

[commit-bot: I haz the power, 2016-10-25 09:48:19]

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, 2016-10-25 09:50:48]

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

[commit-bot: I haz the power, 2016-10-25 09:50:49]

Dry run: Try jobs failed on following builders:
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/15161)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/15123)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/11434)

[Leszek Swirski, 2016-10-25 09:58:43]

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

[commit-bot: I haz the power, 2016-10-25 09:58:49]

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, 2016-10-25 10:25:43]

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

[commit-bot: I haz the power, 2016-10-25 10:25:43]

Dry run: This issue passed the CQ dry run.

[Leszek Swirski, 2016-10-25 10:53:26]

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

[commit-bot: I haz the power, 2016-10-25 10:53:33]

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, 2016-10-25 10:54:44]

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

[commit-bot: I haz the power, 2016-10-25 10:54:45]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/11438)

[Leszek Swirski, 2016-10-25 10:56:27]

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

[commit-bot: I haz the power, 2016-10-25 10:56:33]

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, 2016-10-25 10:57:43]

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

[commit-bot: I haz the power, 2016-10-25 10:57:44]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/15168)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/11440)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/27241)

[Leszek Swirski, 2016-10-25 10:59:13]

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

[commit-bot: I haz the power, 2016-10-25 10:59:23]

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, 2016-10-25 11:43:27]

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

[commit-bot: I haz the power, 2016-10-25 11:43:28]

Dry run: This issue passed the CQ dry run.

[Leszek Swirski, 2016-10-25 12:30:17]

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

[commit-bot: I haz the power, 2016-10-25 12:30:21]

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

[Michael Achenbach, 2016-10-25 13:57:16]

The CQ bit was unchecked by machenbach@chromium.org

[Michael Achenbach, 2016-10-25 13:57:24]

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

[commit-bot: I haz the power, 2016-10-25 13:57:29]

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

[Michael Achenbach, 2016-10-25 14:02:38]

There seems to be an infra bug on the CQ waiting for v8_linux_rel_ng (that
triggered the cancelled v8_linux_rel_ng_triggered). Hope it will run into some
timeout soon.

[Michael Achenbach, 2016-10-25 14:03:08]

On 2016/10/25 14:02:38, machenbach (slow) wrote:
> There seems to be an infra bug on the CQ waiting for v8_linux_rel_ng (that
> triggered the cancelled v8_linux_rel_ng_triggered). Hope it will run into some
> timeout soon.

If this doesn't happen, maybe just upload another empty patchset. Sorry for the
inconvenience.

[Leszek Swirski, 2016-10-25 14:03:45]

On 2016/10/25 14:03:08, machenbach (slow) wrote:
> On 2016/10/25 14:02:38, machenbach (slow) wrote:
> > There seems to be an infra bug on the CQ waiting for v8_linux_rel_ng (that
> > triggered the cancelled v8_linux_rel_ng_triggered). Hope it will run into
some
> > timeout soon.
> 
> If this doesn't happen, maybe just upload another empty patchset. Sorry for
the
> inconvenience.

No worries, thanks for the heads up

[commit-bot: I haz the power, 2016-10-25 15:05:45]

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

[commit-bot: I haz the power, 2016-10-25 15:05:46]

Dry run: This issue passed the CQ dry run.

[Leszek Swirski, 2016-10-25 15:07:48]

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

[commit-bot: I haz the power, 2016-10-25 15:07:54]

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, 2016-10-25 15:09:49]

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

[commit-bot: I haz the power, 2016-10-25 15:09:50]

Dry run: This issue passed the CQ dry run.

[Leszek Swirski, 2016-10-25 15:25:37]

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

[commit-bot: I haz the power, 2016-10-25 15:25:44]

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

[Leszek Swirski, 2016-10-25 15:26:08]

Description was changed from

==========
[compiler] Make SFI "optimize" flag a "tier up" flag

Reuses (and renames) the SFI "mark for optimization" flag to also permit
marking for baseline recompilation. The flag now represents a "tier up"
request, and CompileLazy can get baseline code as well as optimized
code.

BUG=v8:5512
==========

to

==========
[compiler] Make SFI "optimize" flag a "tier up" flag

Reuses (and renames) the SFI "mark for optimization" flag to also permit
marking for baseline recompilation. The flag now represents a "tier up"
request, and CompileLazy can get baseline code as well as optimized
code.

BUG=v8:5512
==========

[Leszek Swirski, 2016-10-25 15:26:08]

leszeks@chromium.org changed reviewers:
+ rmcilroy@chromium.org
- machenbach@chromium.org

[commit-bot: I haz the power, 2016-10-25 15:58:30]

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

[commit-bot: I haz the power, 2016-10-25 15:58:31]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2016-10-26 15:07:58]

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

[rmcilroy, 2016-10-26 15:07:59]

Overall looks reasonable, but adding Michi since he know's this code better.

https://codereview.chromium.org/2448933002/diff/100001/src/builtins/arm/built...
File src/builtins/arm/builtins-arm.cc (right):

https://codereview.chromium.org/2448933002/diff/100001/src/builtins/arm/built...
src/builtins/arm/builtins-arm.cc:1485: // Is the shared function marked for
optimization?
update comment (throughtout)

https://codereview.chromium.org/2448933002/diff/100001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/2448933002/diff/100001/src/flag-definitions.h...
src/flag-definitions.h:279: "mark shared functions for optimization")
Update description. Also how about mark_shared_functions_for_tier_up

https://codereview.chromium.org/2448933002/diff/100001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2448933002/diff/100001/src/objects.cc#newcode...
src/objects.cc:12242: shared()->set_was_marked_for_tier_up(true);
Could you add the TODO here to mention this doesn't do the optimization
concurrently if we hit the SFI.

https://codereview.chromium.org/2448933002/diff/100001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2448933002/diff/100001/src/objects.h#newcode7672
src/objects.h:7672: DECL_BOOLEAN_ACCESSORS(was_marked_for_tier_up)
nit - marked_for_tier_up (drop the "was_")

https://codereview.chromium.org/2448933002/diff/100001/src/objects.h#newcode7923
src/objects.h:7923: kWasMarkedForTierUp,
ditto and below

[rmcilroy, 2016-10-26 15:08:03]

Overall looks reasonable, but adding Michi since he know's this code better.

[Michael Starzinger, 2016-10-27 09:29:59]

LGTM from my end with one comment.

https://codereview.chromium.org/2448933002/diff/100001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/2448933002/diff/100001/src/compiler.cc#newcod...
src/compiler.cc:988: DCHECK(function->shared()->is_compiled());
Can you elaborate why this DCHECK was added? I can see how it currently holds,
but I am not sure why it is actually needed or what it is intended to be
guarding. Both {GetBaselineCode} as well as {GetOptimizedCode} should be
resilient towards the SFI no being compiled yet. Unless I am missing something I
would vote for dropping the DCHECK.

[Michael Starzinger, 2016-10-27 09:32:22]

https://codereview.chromium.org/2448933002/diff/100001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/2448933002/diff/100001/src/compiler.cc#newcod...
src/compiler.cc:988: DCHECK(function->shared()->is_compiled());
On 2016/10/27 09:29:59, Michael Starzinger wrote:
> Can you elaborate why this DCHECK was added? I can see how it currently holds,
> but I am not sure why it is actually needed or what it is intended to be
> guarding. Both {GetBaselineCode} as well as {GetOptimizedCode} should be
> resilient towards the SFI no being compiled yet. Unless I am missing something
I
> would vote for dropping the DCHECK.

Actually, I think with code flushing this is not guaranteed to hold. There might
very well be an SFI that was makred for eager tier-up but the GC flushed the
shared code in the meantime. Pretty sure I can write a repro for this. Let me
know if you want/need a repro for this.

[Leszek Swirski, 2016-10-27 09:40:21]

Thanks both, comments addressed.

https://codereview.chromium.org/2448933002/diff/100001/src/builtins/arm/built...
File src/builtins/arm/builtins-arm.cc (right):

https://codereview.chromium.org/2448933002/diff/100001/src/builtins/arm/built...
src/builtins/arm/builtins-arm.cc:1485: // Is the shared function marked for
optimization?
On 2016/10/26 15:07:59, rmcilroy wrote:
> update comment (throughtout)

Done.

https://codereview.chromium.org/2448933002/diff/100001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/2448933002/diff/100001/src/compiler.cc#newcod...
src/compiler.cc:988: DCHECK(function->shared()->is_compiled());
On 2016/10/27 09:29:59, Michael Starzinger wrote:
> Can you elaborate why this DCHECK was added? I can see how it currently holds,
> but I am not sure why it is actually needed or what it is intended to be
> guarding. Both {GetBaselineCode} as well as {GetOptimizedCode} should be
> resilient towards the SFI no being compiled yet. Unless I am missing something
I
> would vote for dropping the DCHECK.

It's here because I put the default case as unreachable, my reasoning being that
"tiering up" to interpreter is a logic error somewhere. But, I see your point
about resilience, and we may have some eager marking in the future (e.g. for
always-opt or some parse-time heuristics), so I'll remove it.

Oh, and good point with the code flushing. No repro needed, but thanks!

https://codereview.chromium.org/2448933002/diff/100001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/2448933002/diff/100001/src/flag-definitions.h...
src/flag-definitions.h:279: "mark shared functions for optimization")
On 2016/10/26 15:07:59, rmcilroy wrote:
> Update description. Also how about mark_shared_functions_for_tier_up

Done.

https://codereview.chromium.org/2448933002/diff/100001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2448933002/diff/100001/src/objects.cc#newcode...
src/objects.cc:12242: shared()->set_was_marked_for_tier_up(true);
On 2016/10/26 15:07:59, rmcilroy wrote:
> Could you add the TODO here to mention this doesn't do the optimization
> concurrently if we hit the SFI.

Done.

https://codereview.chromium.org/2448933002/diff/100001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2448933002/diff/100001/src/objects.h#newcode7672
src/objects.h:7672: DECL_BOOLEAN_ACCESSORS(was_marked_for_tier_up)
On 2016/10/26 15:07:59, rmcilroy wrote:
> nit - marked_for_tier_up (drop the "was_")

Done. I'm not sure about the "marked_for_" either, maybe it'd be simpler to just
call it tier_up? WDYT?

https://codereview.chromium.org/2448933002/diff/100001/src/objects.h#newcode7923
src/objects.h:7923: kWasMarkedForTierUp,
On 2016/10/26 15:07:59, rmcilroy wrote:
> ditto and below

Done.

[Leszek Swirski, 2016-10-27 09:40:33]

The CQ bit was checked by leszeks@chromium.org

[Leszek Swirski, 2016-10-27 09:40:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/2448933002/#ps120001
(title: "Do some renaming and remove a DCHECK")

[commit-bot: I haz the power, 2016-10-27 09:40:45]

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, 2016-10-27 10:08:02]

Description was changed from

==========
[compiler] Make SFI "optimize" flag a "tier up" flag

Reuses (and renames) the SFI "mark for optimization" flag to also permit
marking for baseline recompilation. The flag now represents a "tier up"
request, and CompileLazy can get baseline code as well as optimized
code.

BUG=v8:5512
==========

to

==========
[compiler] Make SFI "optimize" flag a "tier up" flag

Reuses (and renames) the SFI "mark for optimization" flag to also permit
marking for baseline recompilation. The flag now represents a "tier up"
request, and CompileLazy can get baseline code as well as optimized
code.

BUG=v8:5512
==========

[commit-bot: I haz the power, 2016-10-27 10:08:03]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-11-17 22:15:05]

Description was changed from

==========
[compiler] Make SFI "optimize" flag a "tier up" flag

Reuses (and renames) the SFI "mark for optimization" flag to also permit
marking for baseline recompilation. The flag now represents a "tier up"
request, and CompileLazy can get baseline code as well as optimized
code.

BUG=v8:5512
==========

to

==========
[compiler] Make SFI "optimize" flag a "tier up" flag

Reuses (and renames) the SFI "mark for optimization" flag to also permit
marking for baseline recompilation. The flag now represents a "tier up"
request, and CompileLazy can get baseline code as well as optimized
code.

BUG=v8:5512

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

[commit-bot: I haz the power, 2016-11-17 22:15:08]

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