[compiler] Drop the CompareNilIC.

[compiler] Drop the CompareNilIC.

Since both null and undefined are also marked as undetectable now, we
can just test that bit instead of having the CompareNilIC try to collect
feedback to speed up the general case (without the undetectable bit
being used).

Drive-by-fix: Update the type system to match the new handling of
undetectable in the runtime.

R=danno@chromium.org

Committed: https://crrev.com/666aec0348c8793e61c8633dee7ad29a514239ba
Cr-Commit-Position: refs/heads/master@{#34237}

Committed: https://crrev.com/fb59ea3334bef0f4d2a1dd4b579bf5eec3bc1f35
Cr-Commit-Position: refs/heads/master@{#34344}

[Benedikt Meurer, 2016-02-23 13:55:07]

Hey Danno,

Dropping your baby now that we have the "undetectable" bit on undefined and
null. :-)
Please take a look.

Thanks,
Benedikt

[Toon Verwaest, 2016-02-23 20:03:18]

verwaest@chromium.org changed reviewers:
+ verwaest@chromium.org

[Toon Verwaest, 2016-02-23 20:03:19]

DBCs inline
Nice!

https://codereview.chromium.org/1722193002/diff/1/src/crankshaft/hydrogen-typ...
File src/crankshaft/hydrogen-types.cc (right):

https://codereview.chromium.org/1722193002/diff/1/src/crankshaft/hydrogen-typ...
src/crankshaft/hydrogen-types.cc:46: if (value->IsJSArray() &&
!value->IsUndetectableObject()) {
JSArrays cannot be undetectable I think. It's only settable on ObjectTemplates
which cannot result in JSArray objects.

https://codereview.chromium.org/1722193002/diff/1/src/crankshaft/hydrogen.cc
File src/crankshaft/hydrogen.cc (right):

https://codereview.chromium.org/1722193002/diff/1/src/crankshaft/hydrogen.cc#...
src/crankshaft/hydrogen.cc:11683: NilValue nil) {
As a next step I guess we should just drop this entire function, and just check
whether it's an abstract equality check with either side evaluated to an
undetectable HConstant. In that case we do an HIsUndetectable on the other side.
That removes some hacky code from the ast.

[Benedikt Meurer, 2016-02-24 05:39:19]

https://codereview.chromium.org/1722193002/diff/1/src/crankshaft/hydrogen-typ...
File src/crankshaft/hydrogen-types.cc (right):

https://codereview.chromium.org/1722193002/diff/1/src/crankshaft/hydrogen-typ...
src/crankshaft/hydrogen-types.cc:46: if (value->IsJSArray() &&
!value->IsUndetectableObject()) {
On 2016/02/23 20:03:19, Toon Verwaest wrote:
> JSArrays cannot be undetectable I think. It's only settable on ObjectTemplates
> which cannot result in JSArray objects.

Done.

https://codereview.chromium.org/1722193002/diff/1/src/crankshaft/hydrogen.cc
File src/crankshaft/hydrogen.cc (right):

https://codereview.chromium.org/1722193002/diff/1/src/crankshaft/hydrogen.cc#...
src/crankshaft/hydrogen.cc:11683: NilValue nil) {
Fullcodegen still uses the Ast functionality, and we need to make sure that we
get the environment right, i.e. FCG doesn't push the nil value onto the
expression stack (although since we don't eager deopt from the nil comparison
anymore after this CL, this shouldn't matter).

[danno, 2016-02-24 08:25:40]

almost, but see my comment.

https://codereview.chromium.org/1722193002/diff/40001/src/crankshaft/hydrogen...
File src/crankshaft/hydrogen-instructions.h (right):

https://codereview.chromium.org/1722193002/diff/40001/src/crankshaft/hydrogen...
src/crankshaft/hydrogen-instructions.h:4447: int known_successor_index_;
Maybe I'm missing something, but it doesn't look like known_successor_index_ is
ever set to anything other than  kNoKnownSuccessorIndex, and is
set_known_successor_index isn't called in your patch nor is it a virtual method
that could be called somewhere else. It looks like your logic to make the
successor decision is completely in KnownSuccessorBlock based on the Types. Is
all that known_successor_index stuff then even necessary?

https://codereview.chromium.org/1722193002/diff/40001/src/types.cc
File src/types.cc (right):

https://codereview.chromium.org/1722193002/diff/40001/src/types.cc#newcode204
src/types.cc:204: if (map->is_undetectable()) return kOtherUndetectable;
nit: ternary operator?

[Benedikt Meurer, 2016-02-24 08:41:18]

https://codereview.chromium.org/1722193002/diff/40001/src/crankshaft/hydrogen...
File src/crankshaft/hydrogen-instructions.h (right):

https://codereview.chromium.org/1722193002/diff/40001/src/crankshaft/hydrogen...
src/crankshaft/hydrogen-instructions.h:4447: int known_successor_index_;
Right, I tried to add support for HIsUndetectableAndBranch to check elimination,
but undid that change because there was no benefit; forgot to remove the
known_successor_index_ later.

https://codereview.chromium.org/1722193002/diff/40001/src/types.cc
File src/types.cc (right):

https://codereview.chromium.org/1722193002/diff/40001/src/types.cc#newcode204
src/types.cc:204: if (map->is_undetectable()) return kOtherUndetectable;
Prefer to match surrounding style.

[danno, 2016-02-24 09:02:47]

lgtm

[Benedikt Meurer, 2016-02-24 09:05:48]

The CQ bit was checked by bmeurer@chromium.org

[commit-bot: I haz the power, 2016-02-24 09:06:00]

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

[commit-bot: I haz the power, 2016-02-24 09:09:39]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-02-24 09:10:20]

Description was changed from

==========
[compiler] Drop the CompareNilIC.

Since both null and undefined are also marked as undetectable now, we
can just test that bit instead of having the CompareNilIC try to collect
feedback to speed up the general case (without the undetectable bit
being used).

Drive-by-fix: Update the type system to match the new handling of
undetectable in the runtime.

R=danno@chromium.org
==========

to

==========
[compiler] Drop the CompareNilIC.

Since both null and undefined are also marked as undetectable now, we
can just test that bit instead of having the CompareNilIC try to collect
feedback to speed up the general case (without the undetectable bit
being used).

Drive-by-fix: Update the type system to match the new handling of
undetectable in the runtime.

R=danno@chromium.org

Committed: https://crrev.com/666aec0348c8793e61c8633dee7ad29a514239ba
Cr-Commit-Position: refs/heads/master@{#34237}
==========

[commit-bot: I haz the power, 2016-02-24 09:10:21]

Patchset 4 (id:??) landed as
https://crrev.com/666aec0348c8793e61c8633dee7ad29a514239ba
Cr-Commit-Position: refs/heads/master@{#34237}

[adamk, 2016-02-25 23:08:24]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1743433002/ by adamk@chromium.org.

The reason for reverting is: Speculative revert in attempt to fix #2 crasher on
canary..

[mvstanton, 2016-02-25 23:08:56]

mvstanton@chromium.org changed reviewers:
+ mvstanton@chromium.org

[mvstanton, 2016-02-25 23:08:57]

https://codereview.chromium.org/1722193002/diff/60001/src/full-codegen/x64/fu...
File src/full-codegen/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/1722193002/diff/60001/src/full-codegen/x64/fu...
src/full-codegen/x64/full-codegen-x64.cc:3925: Immediate(1 <<
Map::kIsUndetectable));
Adam and I were speculating that this allows a Map (internal object) to fall out
into a later ToBooleanStub input through rax, per today's Canary. Could it be
so?

[adamk, 2016-02-25 23:38:37]

adamk@chromium.org changed reviewers:
+ adamk@chromium.org

[adamk, 2016-02-25 23:38:38]

https://codereview.chromium.org/1722193002/diff/60001/src/full-codegen/x64/fu...
File src/full-codegen/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/1722193002/diff/60001/src/full-codegen/x64/fu...
src/full-codegen/x64/full-codegen-x64.cc:3925: Immediate(1 <<
Map::kIsUndetectable));
On 2016/02/25 23:08:57, mvstanton wrote:
> Adam and I were speculating that this allows a Map (internal object) to fall
out
> into a later ToBooleanStub input through rax, per today's Canary. Could it be
> so?

If there's something wrong with this patch, I don't think this is it. I can't
figure a way that rax leaks into anything else past this block (certainly
haven't been able to generate a test case where we do this compare and then try
to use the ToBoolean stub).

[Benedikt Meurer, 2016-02-27 18:34:22]

Description was changed from

==========
[compiler] Drop the CompareNilIC.

Since both null and undefined are also marked as undetectable now, we
can just test that bit instead of having the CompareNilIC try to collect
feedback to speed up the general case (without the undetectable bit
being used).

Drive-by-fix: Update the type system to match the new handling of
undetectable in the runtime.

R=danno@chromium.org

Committed: https://crrev.com/666aec0348c8793e61c8633dee7ad29a514239ba
Cr-Commit-Position: refs/heads/master@{#34237}
==========

to

==========
[compiler] Drop the CompareNilIC.

Since both null and undefined are also marked as undetectable now, we
can just test that bit instead of having the CompareNilIC try to collect
feedback to speed up the general case (without the undetectable bit
being used).

Drive-by-fix: Update the type system to match the new handling of
undetectable in the runtime.

R=danno@chromium.org

Committed: https://crrev.com/666aec0348c8793e61c8633dee7ad29a514239ba
Cr-Commit-Position: refs/heads/master@{#34237}
==========

[Benedikt Meurer, 2016-02-27 18:35:07]

The CQ bit was checked by bmeurer@chromium.org

[commit-bot: I haz the power, 2016-02-27 18:35:15]

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

[commit-bot: I haz the power, 2016-02-27 18:36:43]

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

[commit-bot: I haz the power, 2016-02-27 18:36:44]

Try jobs failed on following builders:
  v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)
  v8_linux64_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/2046)
  v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1...)
  v8_linux_arm_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel/builds/14444)
  v8_linux_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/2043)

[Benedikt Meurer, 2016-02-27 18:51:58]

The CQ bit was checked by bmeurer@chromium.org

[Benedikt Meurer, 2016-02-27 18:51:59]

The patchset sent to the CQ was uploaded after l-g-t-m from danno@chromium.org
Link to the patchset: https://codereview.chromium.org/1722193002/#ps80001
(title: "REBASE")

[commit-bot: I haz the power, 2016-02-27 18:52:10]

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

[commit-bot: I haz the power, 2016-02-27 19:13:28]

Description was changed from

==========
[compiler] Drop the CompareNilIC.

Since both null and undefined are also marked as undetectable now, we
can just test that bit instead of having the CompareNilIC try to collect
feedback to speed up the general case (without the undetectable bit
being used).

Drive-by-fix: Update the type system to match the new handling of
undetectable in the runtime.

R=danno@chromium.org

Committed: https://crrev.com/666aec0348c8793e61c8633dee7ad29a514239ba
Cr-Commit-Position: refs/heads/master@{#34237}
==========

to

==========
[compiler] Drop the CompareNilIC.

Since both null and undefined are also marked as undetectable now, we
can just test that bit instead of having the CompareNilIC try to collect
feedback to speed up the general case (without the undetectable bit
being used).

Drive-by-fix: Update the type system to match the new handling of
undetectable in the runtime.

R=danno@chromium.org

Committed: https://crrev.com/666aec0348c8793e61c8633dee7ad29a514239ba
Cr-Commit-Position: refs/heads/master@{#34237}
==========

[commit-bot: I haz the power, 2016-02-27 19:13:29]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-02-27 19:13:58]

Description was changed from

==========
[compiler] Drop the CompareNilIC.

Since both null and undefined are also marked as undetectable now, we
can just test that bit instead of having the CompareNilIC try to collect
feedback to speed up the general case (without the undetectable bit
being used).

Drive-by-fix: Update the type system to match the new handling of
undetectable in the runtime.

R=danno@chromium.org

Committed: https://crrev.com/666aec0348c8793e61c8633dee7ad29a514239ba
Cr-Commit-Position: refs/heads/master@{#34237}
==========

to

==========
[compiler] Drop the CompareNilIC.

Since both null and undefined are also marked as undetectable now, we
can just test that bit instead of having the CompareNilIC try to collect
feedback to speed up the general case (without the undetectable bit
being used).

Drive-by-fix: Update the type system to match the new handling of
undetectable in the runtime.

R=danno@chromium.org

Committed: https://crrev.com/666aec0348c8793e61c8633dee7ad29a514239ba
Cr-Commit-Position: refs/heads/master@{#34237}

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

[commit-bot: I haz the power, 2016-02-27 19:13:59]

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