[compiler] Fix flipped boolean checks in marked tier-up

[compiler] Fix flipped boolean checks in marked tier-up

Fixes incorrect checks for handle validity when checking the compiled
code, as well as incorrect uses of tst in arm and ppc flag checking
code. Also adds a test that the tier-up works correctly.

Committed: https://crrev.com/712a46cc3f6a37fc23f9bacea21a94b13d270f1b
Cr-Commit-Position: refs/heads/master@{#40915}

[Leszek Swirski, 2016-11-04 17:36:12]

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

[commit-bot: I haz the power, 2016-11-04 17:36:26]

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-11-04 17:41:28]

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

[commit-bot: I haz the power, 2016-11-04 17:41:32]

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-11-04 17:42:23]

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

[Leszek Swirski, 2016-11-04 17:42:23]

I promise that I'm normally a good programmer :/

Feel free to set the commit bit on this if you lgtm, since I'll be away start of
next week.

[commit-bot: I haz the power, 2016-11-04 17:55:02]

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

[commit-bot: I haz the power, 2016-11-04 17:55:03]

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_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[rmcilroy, 2016-11-07 10:17:47]

Opps, that might explain things :).

LGTM on code, but you will need to fix the tests so they are green on the bots
before landing. Looks like you need to deal with --always-opt as well (maybe
just add --no-always-opt to the flags?)

https://codereview.chromium.org/2478323002/diff/20001/test/mjsunit/shared-fun...
File test/mjsunit/shared-function-tier-up-default.js (right):

https://codereview.chromium.org/2478323002/diff/20001/test/mjsunit/shared-fun...
test/mjsunit/shared-function-tier-up-default.js:5: // Flags:
--mark-shared-functions-for-tier-up --allow-natives-syntax --no-ignition-staging
--no-turbo
This is not really default since it's --no-ignition-staging and --no-turbo.
Could you just have a single test and just use %BaselineFunctionOnNextCall to
control whether we are optimizing or baselining (with suitable checks that we
either remain in an appropriate OptimizationStatus (e.g., don't do anything or
tier up to baseline depending on the flags)?

[Leszek Swirski, 2016-11-09 14:43:25]

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

[commit-bot: I haz the power, 2016-11-09 14:43:32]

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-11-09 14:57:00]

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

[commit-bot: I haz the power, 2016-11-09 14:57:01]

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_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[Leszek Swirski, 2016-11-09 16:20:38]

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

[commit-bot: I haz the power, 2016-11-09 16:20:50]

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-11-09 16:37:03]

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

[commit-bot: I haz the power, 2016-11-09 16:37:04]

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_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[Leszek Swirski, 2016-11-10 16:16:56]

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

[commit-bot: I haz the power, 2016-11-10 16:16:58]

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-11-10 16:34:32]

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

[commit-bot: I haz the power, 2016-11-10 16:34:33]

Dry run: Try jobs failed on following builders:
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/12290)
  v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)

[Leszek Swirski, 2016-11-10 16:36:41]

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

[commit-bot: I haz the power, 2016-11-10 16:36:52]

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-11-10 17:06:41]

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

[commit-bot: I haz the power, 2016-11-10 17:06:42]

Dry run: Try jobs failed on following builders:
  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_arm_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng_trigger...)

[Leszek Swirski, 2016-11-11 10:13:30]

Strangely enough, this is failing on ARM now, and ARM alone. Looks like that's
an actual bug picked up by the test (yay tests), so I'll see if I can fix that.

https://codereview.chromium.org/2478323002/diff/20001/test/mjsunit/shared-fun...
File test/mjsunit/shared-function-tier-up-default.js (right):

https://codereview.chromium.org/2478323002/diff/20001/test/mjsunit/shared-fun...
test/mjsunit/shared-function-tier-up-default.js:5: // Flags:
--mark-shared-functions-for-tier-up --allow-natives-syntax --no-ignition-staging
--no-turbo
On 2016/11/07 10:17:46, rmcilroy wrote:
> This is not really default since it's --no-ignition-staging and --no-turbo.
> Could you just have a single test and just use %BaselineFunctionOnNextCall to
> control whether we are optimizing or baselining (with suitable checks that we
> either remain in an appropriate OptimizationStatus (e.g., don't do anything or
> tier up to baseline depending on the flags)?

I tried putting this in a single test, but it ended up being very messy (having
to effectively hack in checks for these various flags because e.g. we don't
baseline when tiering up ignition->turbo). Three tests feels cleaner, but I did
add extra guards against always opt and invalid optimization statuses, as well
as a new optimization status for interpreted code.

[Leszek Swirski, 2016-11-11 11:24:29]

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

[commit-bot: I haz the power, 2016-11-11 11:24: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-11-11 11:49:09]

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

[commit-bot: I haz the power, 2016-11-11 11:49:10]

Dry run: This issue passed the CQ dry run.

[Leszek Swirski, 2016-11-11 11:49:45]

Description was changed from

==========
[compiler] Fix flipped handle validity check in marked tier-up
==========

to

==========
[compiler] Fix flipped boolean checks in marked tier-up

Fixes incorrect checks for handle validity when checking the compiled
code, as well as incorrect uses of tst in arm and ppc flag checking
code. Also adds a test that the tier-up works correctly.
==========

[Leszek Swirski, 2016-11-11 11:50:58]

Finally green, PTAL

[rmcilroy, 2016-11-11 11:54:55]

LGTM, thanks.

[Leszek Swirski, 2016-11-11 11:55:19]

The CQ bit was checked by leszeks@chromium.org

[commit-bot: I haz the power, 2016-11-11 11:55:33]

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-11-11 11:57:06]

Description was changed from

==========
[compiler] Fix flipped boolean checks in marked tier-up

Fixes incorrect checks for handle validity when checking the compiled
code, as well as incorrect uses of tst in arm and ppc flag checking
code. Also adds a test that the tier-up works correctly.
==========

to

==========
[compiler] Fix flipped boolean checks in marked tier-up

Fixes incorrect checks for handle validity when checking the compiled
code, as well as incorrect uses of tst in arm and ppc flag checking
code. Also adds a test that the tier-up works correctly.
==========

[commit-bot: I haz the power, 2016-11-11 11:57:07]

Committed patchset #7 (id:120001)

[Benedikt Meurer, 2016-11-11 12:38:58]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2492523007/ by bmeurer@chromium.org.

The reason for reverting is: Breaks deopt fuzzer:
https://build.chromium.org/p/client.v8/builders/V8%20Deopt%20Fuzzer/builds/14....

[Michael Achenbach, 2016-11-11 12:59:39]

On 2016/11/11 12:38:58, Benedikt Meurer wrote:
> A revert of this CL (patchset #7 id:120001) has been created in
> https://codereview.chromium.org/2492523007/ by mailto:bmeurer@chromium.org.
> 
> The reason for reverting is: Breaks deopt fuzzer:
>
https://build.chromium.org/p/client.v8/builders/V8%20Deopt%20Fuzzer/builds/14....

Hmm - I don't think a revert is really necessary. The test
shared-function-tier-up-turbo runs out of stack in the analysis phase. It
doesn't cause a deopt problem. If the test can't be made working with the
deoptfuzzer just blacklist it in the status file on reland. There's already a
section:
https://cs.chromium.org/chromium/src/v8/test/mjsunit/mjsunit.status?q=mjsunit...

[Leszek Swirski, 2016-11-11 13:04:35]

Not quite a stack failure, looks like it's just a failure from forced deopts
breaking how the test works. Blacklisting sounds like a good idea.

[commit-bot: I haz the power, 2016-11-17 22:30:16]

Description was changed from

==========
[compiler] Fix flipped boolean checks in marked tier-up

Fixes incorrect checks for handle validity when checking the compiled
code, as well as incorrect uses of tst in arm and ppc flag checking
code. Also adds a test that the tier-up works correctly.
==========

to

==========
[compiler] Fix flipped boolean checks in marked tier-up

Fixes incorrect checks for handle validity when checking the compiled
code, as well as incorrect uses of tst in arm and ppc flag checking
code. Also adds a test that the tier-up works correctly.

Committed: https://crrev.com/712a46cc3f6a37fc23f9bacea21a94b13d270f1b
Cr-Commit-Position: refs/heads/master@{#40915}
==========

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

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