Add initial code-stub version of Object.prototype.hasOwnProperty

Add initial code-stub version of Object.prototype.hasOwnProperty

It for now only deals with fast-mode smi and object arrays with smi
keys and internalized strings; and fast-mode named properties with an internalized key or symbol.

BUG=v8:2472
LOG=n

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

[Toon Verwaest, 2016-03-30 09:19:39]

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

[Toon Verwaest, 2016-03-30 09:19:40]

ptal

[Toon Verwaest, 2016-03-30 09:20:12]

Description was changed from

==========
Add initial code-stub version of Object.prototype.hasOwnProperty

It for now only deals with fast-mode smi and object arrays with smi
keys; and fast-mode named properties with an internalized key or symbol.

BUG=v8:2472
LOG=n
==========

to

==========
Add initial code-stub version of Object.prototype.hasOwnProperty

It for now only deals with fast-mode smi and object arrays with smi
keys and internalized strings; and fast-mode named properties with an
internalized key or symbol.

BUG=v8:2472
LOG=n
==========

[Benedikt Meurer, 2016-03-30 09:28:31]

LGTM with comment.

https://codereview.chromium.org/1843613002/diff/100001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1843613002/diff/100001/src/builtins.cc#newcod...
src/builtins.cc:348: RUNTIME_FUNCTION(Runtime_ObjectHasOwnProperty) {
Please move this to runtime-something.cc

[Toon Verwaest, 2016-03-30 09:50:06]

Addressed comment.

[Toon Verwaest, 2016-03-30 09:50:32]

The CQ bit was checked by verwaest@chromium.org

[Toon Verwaest, 2016-03-30 09:50:32]

The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org
Link to the patchset: https://codereview.chromium.org/1843613002/#ps120001
(title: "Addressed comment")

[commit-bot: I haz the power, 2016-03-30 09:50:40]

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

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

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

[commit-bot: I haz the power, 2016-03-30 10:25:13]

Try jobs failed on following builders:
  v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1...)

[Toon Verwaest, 2016-03-30 11:31:25]

verwaest@chromium.org changed reviewers:
+ jacob.bramley@arm.com, rodolph.perfetta@arm.com

[Toon Verwaest, 2016-03-30 11:31:26]

@jacob.bramley: It seems like the ARM64 intruction selector has a bug. See the
failing tests. What's happening is the following:

The following code in builtins.cc:

 506     Node* length = assembler->LoadFixedArrayBaseLength(elements);
...
 509     assembler->Branch(
 510         assembler->Int32LessThan(index, assembler->SmiUntag(length)),
 511         &if_iskeyinrange, &return_false);

Gets translated to 

// Load elements from the object
0x5ce35ab0   272  ldur x0, [x0, #15]
// Load the length from the elements (smi)
0x5ce35ab4   276  ldur x1, [x0, #7]
// Load the index (int32)
0x5ce35ab8   280  ldr x2, [jssp, #8]
// Compare the lower word of the smi (always 0) with the index.
0x5ce35abc   284  cmp w2, w1
// Jump to return false.
0x5ce35ac0   288  b.ge #+0x60 (addr 0x5ce35b20)

[Rodolph Perfetta, 2016-03-30 16:31:15]

@Toon: I am not sure it is an instruction selector issue, in the
code-stub-assembler the method SmiUntag returns a Word so on arm64 it is a
word64 and the Int32LessThan doesn't expect that.

>  509     assembler->Branch(
>  510         assembler->Int32LessThan(index, assembler->SmiUntag(length)),
>  511         &if_iskeyinrange, &return_false);

If I replace SmiUntag by SmiToWord32, all the tests pass on my machine. What
puzzles me is that I would expect this to be an issue on x64 too so I must be
missing something and looking at the code I couldn't find what reducer/selector
would lead to the code you have given.

[Toon Verwaest, 2016-03-30 17:14:28]

Even if it returns a full word, shouldn't the untag operation have made the
bit-pattern 0x00000001 rather than 0x00010000? That's what it does on x64, and
why a following 32-bit comparison works...

[Rodolph Perfetta, 2016-03-30 17:38:31]

On 2016/03/30 17:14:28, Toon Verwaest wrote:
> Even if it returns a full word, shouldn't the untag operation have made the
> bit-pattern 0x00000001 rather than 0x00010000? That's what it does on x64, and
> why a following 32-bit comparison works...

on arm64 we fold shift into arithmetic operations (including cmp). so: y = a asr
32; cmp x, y will become cmp x, a asr 32. Since the cmp here is 32-bit the shift
will become 32-bit (and will ignore the top 32-bit), which is why giving 64-bit
input to a 32-bit cmp result in erroneous code on arm64. What puzzle me is that
the code you give doesn't show any shift (merge or otherwise).

[Benedikt Meurer, 2016-03-30 17:40:21]

Ah indeed. SmiUntag yields a Word, but you need a Word32. So Rodolph is right,
the fix is SmiToWord32.

[Toon Verwaest, 2016-03-30 20:16:27]

Understood, thanks for the explanation; I had no clue ;-)

[Toon Verwaest, 2016-03-31 07:24:13]

The CQ bit was checked by verwaest@chromium.org

[Toon Verwaest, 2016-03-31 07:24:13]

The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org
Link to the patchset: https://codereview.chromium.org/1843613002/#ps140001
(title: "Use SmiToWord32 instead of SmiUntag")

[commit-bot: I haz the power, 2016-03-31 07:24:22]

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

[commit-bot: I haz the power, 2016-03-31 07:25:38]

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

[commit-bot: I haz the power, 2016-03-31 07:25:39]

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/...)
  v8_linux64_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/3601)
  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/16022)
  v8_linux_gcc_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...)
  v8_linux_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/3614)
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/13029)

[Toon Verwaest, 2016-03-31 07:27:59]

Yes indeed, the absence of any shift is odd; and is what I was referring to. I
presumed that SmiUntag would always yield a word32. But I'm fine with using
SmiToWord32.

[Toon Verwaest, 2016-03-31 07:29:29]

The CQ bit was checked by verwaest@chromium.org

[Toon Verwaest, 2016-03-31 07:29:29]

The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org
Link to the patchset: https://codereview.chromium.org/1843613002/#ps160001
(title: "Rebase")

[commit-bot: I haz the power, 2016-03-31 07:29:32]

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

[commit-bot: I haz the power, 2016-03-31 07:53:16]

Description was changed from

==========
Add initial code-stub version of Object.prototype.hasOwnProperty

It for now only deals with fast-mode smi and object arrays with smi
keys and internalized strings; and fast-mode named properties with an
internalized key or symbol.

BUG=v8:2472
LOG=n
==========

to

==========
Add initial code-stub version of Object.prototype.hasOwnProperty

It for now only deals with fast-mode smi and object arrays with smi
keys and internalized strings; and fast-mode named properties with an
internalized key or symbol.

BUG=v8:2472
LOG=n
==========

[commit-bot: I haz the power, 2016-03-31 07:53:17]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-03-31 07:55:40]

Description was changed from

==========
Add initial code-stub version of Object.prototype.hasOwnProperty

It for now only deals with fast-mode smi and object arrays with smi
keys and internalized strings; and fast-mode named properties with an
internalized key or symbol.

BUG=v8:2472
LOG=n
==========

to

==========
Add initial code-stub version of Object.prototype.hasOwnProperty

It for now only deals with fast-mode smi and object arrays with smi
keys and internalized strings; and fast-mode named properties with an
internalized key or symbol.

BUG=v8:2472
LOG=n

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

[commit-bot: I haz the power, 2016-03-31 07:55:41]

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

[Rodolph Perfetta (ARM), 2016-03-31 08:06:23]

https://codereview.chromium.org/1843613002/diff/160001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1843613002/diff/160001/src/builtins.cc#newcod...
src/builtins.cc:382: var_index.Bind(assembler->SmiUntag(key));
This should be a SmiToWord32 too as var_index is declared as a Word32.