Fix typeof optimization for undetectable

Fix typeof optimization for undetectable

Currently, typeof o, where o is an undetectable
callable object (such as document.all), returns 'function' if
optimised. It should, however, return 'undefined'.

This CL excludes undetectable objects from the optimization
resulting in type 'function' and renames the related code to
reflect that.

BUG=v8:5972
R=bmeurer@chromium.org

Review-Url: https://codereview.chromium.org/2697063002
Cr-Commit-Position: refs/heads/master@{#43298}
Committed: https://chromium.googlesource.com/v8/v8/+/6302753e2fb224156d0ca407fd4922653e98652b

[vabr (Chromium), 2017-02-15 19:02:03]

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

[commit-bot: I haz the power, 2017-02-15 19:02:07]

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

[vabr (Chromium), 2017-02-15 19:05:17]

Hi Benedikt,

Could you please check whether this test looks like doing what it should do?

On an unrelated note: test-api.cc got so large that git cl upload no longer
wants to upload it. As a result, Rietveld cannot show the side-by-side diff for
it any more. Would you like me to split it (in a different CL)?

Cheers,
Vaclav

[vabr (Chromium), 2017-02-15 19:07:32]

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

[commit-bot: I haz the power, 2017-02-15 19:07:36]

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

[Benedikt Meurer, 2017-02-15 19:17:14]

On 2017/02/15 19:05:17, vabr (OOO until late Feb) wrote:
> Hi Benedikt,
> 
> Could you please check whether this test looks like doing what it should do?
> 
> On an unrelated note: test-api.cc got so large that git cl upload no longer
> wants to upload it. As a result, Rietveld cannot show the side-by-side diff
for
> it any more. Would you like me to split it (in a different CL)?
> 
> Cheers,
> Vaclav

See my comment on the bug report. Also there's %GetUndetectable, which gives you
an undetectable object in mjsunit tests, which is easier to deal with than
test-api.cc (which really needs some love, you're right).

[commit-bot: I haz the power, 2017-02-15 19:33:57]

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

[commit-bot: I haz the power, 2017-02-15 19:33:58]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2017-02-15 22:41:44]

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

[commit-bot: I haz the power, 2017-02-15 22:41:48]

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-02-15 22:57:36]

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

[commit-bot: I haz the power, 2017-02-15 22:57:37]

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...)

[vabr (Chromium), 2017-02-15 23:33:33]

Description was changed from

==========
Add a test for optimised typeof document.all

This CL adds a test for checking that typeof o, where o is an undetectable
callable objects (such as document.all), returns 'undefined' even if
optimised.

BUG=v8:5972
R=bmeurer@chromium.org
==========

to

==========
Fix typeof optimization for non-functions

Currently, typeof o, where o is an undetectable
callable objects (such as document.all), returns 'function' even if
optimised.

This CL attempts to improve the check for whether something is a function inside
effect-control-linearizer.

BUG=v8:5972
R=bmeurer@chromium.org
==========

[vabr (Chromium), 2017-02-15 23:36:41]

Thanks, Benedikt!

Your trick for producing a value suspected from both being callable and
undetectable is really interesting!

The current CL is still a WIP, because I need to figure out handling proxies
(see below), but I would appreciate you taking a look and stopping me early if I
already do something silly.

I also have some questions, below in the comments and one here:

What is Phi in this context? I gather it is something like an expression being
reduced by the compiler, but could neither figure it out nor google it.

Cheers,
Vaclav

https://codereview.chromium.org/2697063002/diff/60001/src/compiler/effect-con...
File src/compiler/effect-control-linearizer.cc (right):

https://codereview.chromium.org/2697063002/diff/60001/src/compiler/effect-con...
src/compiler/effect-control-linearizer.cc:1870: __ GotoUnless(check2,
&return_false);
This ignores proxies (which was helpfully pointed out by proxy-related tests). I
need to figure out how to include function proxies in this check.

https://codereview.chromium.org/2697063002/diff/60001/test/mjsunit/regress/re...
File test/mjsunit/regress/regress-5972.js (right):

https://codereview.chromium.org/2697063002/diff/60001/test/mjsunit/regress/re...
test/mjsunit/regress/regress-5972.js:14: assertFalse(foo(false));
Just out of curiosity: why is the call to foo() repeated twice before
instructing the compiler to optimize it?

[Benedikt Meurer, 2017-02-16 04:01:19]

Thanks for digging into this Vaclav. As described in the comments, the bug is
really just in the typing rule (and the name is misleading). I should have
probably said it this way in the bug report. 

A Phi is a special kind of instruction in SSA form to describe a value produced
by different branches at a control flow join. See
https://en.m.wikipedia.org/wiki/Static_single_assignment_form for an
explanation.

https://codereview.chromium.org/2697063002/diff/60001/src/compiler/effect-con...
File src/compiler/effect-control-linearizer.cc (right):

https://codereview.chromium.org/2697063002/diff/60001/src/compiler/effect-con...
src/compiler/effect-control-linearizer.cc:1870: __ GotoUnless(check2,
&return_false);
On 2017/02/15 23:36:41, vabr (OOO until late Feb) wrote:
> This ignores proxies (which was helpfully pointed out by proxy-related tests).
I
> need to figure out how to include function proxies in this check.

The code of ObjectIsCallable is the correct code for typeof x==='function', it
just has a misleading name and the typing rule is wrong. So my idea to fix this
was to rename ObjectIsCallabe and fix the typing rule in the Typer.cc.

https://codereview.chromium.org/2697063002/diff/60001/test/mjsunit/regress/re...
File test/mjsunit/regress/regress-5972.js (right):

https://codereview.chromium.org/2697063002/diff/60001/test/mjsunit/regress/re...
test/mjsunit/regress/regress-5972.js:14: assertFalse(foo(false));
On 2017/02/15 23:36:41, vabr (OOO until late Feb) wrote:
> Just out of curiosity: why is the call to foo() repeated twice before
> instructing the compiler to optimize it?

This is kind of a habit. In this particular case we wouldn't need any warmup at
all actually. But in general if you have property accesses like o.x for example,
then the IC associated with it first transitions to a premonomorphic state
before going monomorphic. So you only get consumable feedback after the second
run in baseline.

[vabr (Chromium), 2017-02-16 20:15:43]

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

[commit-bot: I haz the power, 2017-02-16 20:15: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, 2017-02-16 20:31:21]

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

[commit-bot: I haz the power, 2017-02-16 20:31:22]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)

[vabr (Chromium), 2017-02-16 21:59:34]

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

[commit-bot: I haz the power, 2017-02-16 21:59:38]

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-02-16 22:13:30]

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

[commit-bot: I haz the power, 2017-02-16 22:13:31]

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...)

[vabr (Chromium), 2017-02-17 13:33:45]

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

[commit-bot: I haz the power, 2017-02-17 13:33:55]

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-02-17 13:58:06]

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

[commit-bot: I haz the power, 2017-02-17 13:58:06]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2017-02-17 14:00:09]

Thanks, Benedikt,
For all your online and off-line support, and for delviering everything for this
CL :).
Your solution from this afternoon made the bots green, I will just do the
renaming soon and then ask you for a review.

Cheers,
Vaclav

[Benedikt Meurer, 2017-02-17 14:08:42]

Thanks Vaclav. Just a bit and the operator renaming, but otherwise LGTM! :-)

https://codereview.chromium.org/2697063002/diff/120001/src/compiler/types.h
File src/compiler/types.h (right):

https://codereview.chromium.org/2697063002/diff/120001/src/compiler/types.h#n...
src/compiler/types.h:169: V(Callable,                     kDetectableCallable |
\
Can you fit this into one line?

[vabr (Chromium), 2017-02-17 23:36:10]

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

[commit-bot: I haz the power, 2017-02-17 23:36:18]

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

[vabr (Chromium), 2017-02-17 23:36:24]

Thanks, Benedikt!

I added "Detectable" in front of "Callable" as we discussed offline. I renamed
these pieces:
  * The method in EffectControlLinearizer, because of
    the computation it makes in
    src/compiler/effect-control-linearizer.cc:1703-1707
  * The opcode because of its processing in
    src/compiler/simplified-lowering.cc:2525
  * The method in typer.cc, because that reflects how it
    changed.
  * The operator in src/compiler/simplified-operator.*,
    because of how it links to the opcode in the PURE
    macro in the .cc file.

I appreciate your early approval, but I'll wait with landing until you confirm
that the current patch looks good.

Thanks!
Vaclav

https://codereview.chromium.org/2697063002/diff/120001/src/compiler/types.h
File src/compiler/types.h (right):

https://codereview.chromium.org/2697063002/diff/120001/src/compiler/types.h#n...
src/compiler/types.h:169: V(Callable,                     kDetectableCallable |
\
On 2017/02/17 14:08:42, Benedikt Meurer wrote:
> Can you fit this into one line?

Done.

[vabr (Chromium), 2017-02-17 23:59:14]

Description was changed from

==========
Fix typeof optimization for non-functions

Currently, typeof o, where o is an undetectable
callable objects (such as document.all), returns 'function' even if
optimised.

This CL attempts to improve the check for whether something is a function inside
effect-control-linearizer.

BUG=v8:5972
R=bmeurer@chromium.org
==========

to

==========
Fix typeof optimization for undetectable

Currently, typeof o, where o is an undetectable
callable object (such as document.all), returns 'function' if
optimised. It should, however, return 'undefined'.

This CL excludes undetectable objects from the optimization
resulting in type 'function' and renames the related code to
reflect that.

BUG=v8:5972
R=bmeurer@chromium.org
==========

[commit-bot: I haz the power, 2017-02-18 00:19:23]

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

[commit-bot: I haz the power, 2017-02-18 00:19:24]

Dry run: This issue passed the CQ dry run.

[Benedikt Meurer, 2017-02-18 08:04:28]

Thanks a lot. LGTM!

[vabr (Chromium), 2017-02-18 12:41:53]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2017-02-18 12:41:59]

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-02-18 12:43:38]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1487421713819920,
"parent_rev": "6a797d7edbfe858524958af98af25eead4e00b3c", "commit_rev":
"6302753e2fb224156d0ca407fd4922653e98652b"}

[commit-bot: I haz the power, 2017-02-18 12:43:45]

Description was changed from

==========
Fix typeof optimization for undetectable

Currently, typeof o, where o is an undetectable
callable object (such as document.all), returns 'function' if
optimised. It should, however, return 'undefined'.

This CL excludes undetectable objects from the optimization
resulting in type 'function' and renames the related code to
reflect that.

BUG=v8:5972
R=bmeurer@chromium.org
==========

to

==========
Fix typeof optimization for undetectable

Currently, typeof o, where o is an undetectable
callable object (such as document.all), returns 'function' if
optimised. It should, however, return 'undefined'.

This CL excludes undetectable objects from the optimization
resulting in type 'function' and renames the related code to
reflect that.

BUG=v8:5972
R=bmeurer@chromium.org

Review-Url: https://codereview.chromium.org/2697063002
Cr-Commit-Position: refs/heads/master@{#43298}
Committed:
https://chromium.googlesource.com/v8/v8/+/6302753e2fb224156d0ca407fd4922653e9...
==========

[commit-bot: I haz the power, 2017-02-18 12:43:47]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/v8/v8/+/6302753e2fb224156d0ca407fd4922653e9...