[stubs] Implement fast TF Builtin for Object.create

[stubs] Implement TF builtin for Object.create fast paths

BUG=

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

[Camillo Bruni, 2016-10-05 12:38:59]

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

[commit-bot: I haz the power, 2016-10-05 12:39:05]

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-05 12:54:35]

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

[commit-bot: I haz the power, 2016-10-05 12:54:36]

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

[Camillo Bruni, 2016-10-07 13:51:32]

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

[commit-bot: I haz the power, 2016-10-07 13:51:46]

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-07 14:19:21]

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

[commit-bot: I haz the power, 2016-10-07 14:19:22]

Dry run: This issue passed the CQ dry run.

[Camillo Bruni, 2016-10-17 08:39:25]

Description was changed from

==========
[runtime] Implement fast TF Builtin for Object.create

BUG=
==========

to

==========
[runtime] Implement TF builtin for Object.create fast paths 

BUG=
==========

[Camillo Bruni, 2016-10-18 09:07:02]

cbruni@chromium.org changed reviewers:
+ ishell@chromium.org

[Camillo Bruni, 2016-10-18 09:07:03]

PTAL

[Camillo Bruni, 2016-10-18 09:07:51]

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

[commit-bot: I haz the power, 2016-10-18 09:07: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, 2016-10-18 09:12:44]

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

[commit-bot: I haz the power, 2016-10-18 09:12:46]

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

[Igor Sheludko, 2016-10-18 12:33:22]

I think it's rather [stubs] but not [runtime].

https://codereview.chromium.org/2385423005/diff/120001/src/code-stub-assemble...
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2385423005/diff/120001/src/code-stub-assemble...
src/code-stub-assembler.cc:1059: Assert(Word32Equal(Word32And(bit_field, mask),
Int32Constant(0)));
Please ensure that bit_field is not loaded when asserts are disabled. CSA does
not have dead code elimination yet.

https://codereview.chromium.org/2385423005/diff/120001/src/code-stub-assembler.h
File src/code-stub-assembler.h (right):

https://codereview.chromium.org/2385423005/diff/120001/src/code-stub-assemble...
src/code-stub-assembler.h:191: void BranchIfJSReceiver(compiler::Node* object,
Label* if_true,
I think if_receiver/if_not_receiver are better names.

https://codereview.chromium.org/2385423005/diff/120001/src/code-stub-assemble...
src/code-stub-assembler.h:193: void BranchIfJSObject(compiler::Node* object,
Label* if_true,
Same here.

https://codereview.chromium.org/2385423005/diff/140001/src/builtins/builtins-...
File src/builtins/builtins-object.cc (right):

https://codereview.chromium.org/2385423005/diff/140001/src/builtins/builtins-...
src/builtins/builtins-object.cc:493: Node* is_fast_map = a->Word32Equal(
a->IsFastMap(properties_map); ?

https://codereview.chromium.org/2385423005/diff/140001/src/builtins/builtins-...
src/builtins/builtins-object.cc:497:
a->BranchIfWordEqual(a->LoadMapNumberOfOwnDescriptors(properties_map),
Here you load the bit_field3 again.

https://codereview.chromium.org/2385423005/diff/140001/src/builtins/builtins-...
src/builtins/builtins-object.cc:533: a->GotoUnless(a->HasInstanceType(weak_cell,
WEAK_CELL_TYPE),
Probably it should be an Assert instead. And map comparison will be faster.

https://codereview.chromium.org/2385423005/diff/140001/src/code-stub-assemble...
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2385423005/diff/140001/src/code-stub-assemble...
src/code-stub-assembler.cc:986: Node*
CodeStubAssembler::LoadMapNumberOfOwnDescriptors(Node* map) {
Semi-useful. Usually we load this number after a fast-map bit check which is
also a part of bit_field3. So here we are going to load the bit field second
time.

https://codereview.chromium.org/2385423005/diff/140001/src/code-stub-assemble...
src/code-stub-assembler.cc:1000: GotoUnless(HasInstanceType(prototype_info,
PROTOTYPE_INFO_TYPE),
Map check is better here.

https://codereview.chromium.org/2385423005/diff/140001/src/code-stub-assemble...
src/code-stub-assembler.cc:1050: Node* CodeStubAssembler::IsFastMap(Node* map) {
Semi-useful. Same a above.

https://codereview.chromium.org/2385423005/diff/140001/src/code-stub-assemble...
src/code-stub-assembler.cc:1514: StoreObjectFieldNoWriteBarrier(object,
JSObject::kPropertiesOffset,
Please add a comment about why is it possible to skip write barriers in this
function. Maybe assert that the object is in new space.

https://codereview.chromium.org/2385423005/diff/140001/src/code-stub-assemble...
src/code-stub-assembler.cc:1539: Comment("StoreFieldsNoWriteBarrier");
Consider using BuildFastLoop().

https://codereview.chromium.org/2385423005/diff/140001/src/code-stub-assemble...
src/code-stub-assembler.cc:1540: Variable current(this,
MachineRepresentation::kTagged);
Why is it kTagged? I think it should be PointerRepresentation() instead.

https://codereview.chromium.org/2385423005/diff/140001/src/code-stub-assemble...
src/code-stub-assembler.cc:1543: start_address = IntPtrSub(start_address,
IntPtrConstant(1));
I think it would be better to "untag" it in the caller instead of here.

https://codereview.chromium.org/2385423005/diff/140001/src/code-stub-assemble...
src/code-stub-assembler.cc:1544: end_address = IntPtrSub(end_address,
IntPtrConstant(1));
And thus this would not be necessary.

https://codereview.chromium.org/2385423005/diff/140001/src/compiler/code-asse...
File src/compiler/code-assembler.cc (right):

https://codereview.chromium.org/2385423005/diff/140001/src/compiler/code-asse...
src/compiler/code-assembler.cc:224: return WordEqual(value,
(value & ((1 << kPointerSizeLog2) - 1)) != 0

https://codereview.chromium.org/2385423005/diff/140001/test/cctest/test-code-...
File test/cctest/test-code-stub-assembler.cc (right):

https://codereview.chromium.org/2385423005/diff/140001/test/cctest/test-code-...
test/cctest/test-code-stub-assembler.cc:1540: #define VERIFY(result, map_handle,
properties_handle, elements_handle) \
s/_handle/_value/

[Camillo Bruni, 2016-10-18 12:33:53]

Description was changed from

==========
[runtime] Implement TF builtin for Object.create fast paths 

BUG=
==========

to

==========
[stubs] Implement TF builtin for Object.create fast paths 

BUG=
==========

[Camillo Bruni, 2016-10-18 14:06:00]

https://codereview.chromium.org/2385423005/diff/140001/src/builtins/builtins-...
File src/builtins/builtins-object.cc (right):

https://codereview.chromium.org/2385423005/diff/140001/src/builtins/builtins-...
src/builtins/builtins-object.cc:497:
a->BranchIfWordEqual(a->LoadMapNumberOfOwnDescriptors(properties_map),
On 2016/10/18 at 12:33:21, Igor Sheludko wrote:
> Here you load the bit_field3 again.

Ah, that was the reason I had the bitfield raw up there.

https://codereview.chromium.org/2385423005/diff/140001/src/builtins/builtins-...
src/builtins/builtins-object.cc:532: a->GotoIf(a->TaggedIsSmi(weak_cell),
&call_runtime);
I removed the tagged check here as it is most likely not needed.

https://codereview.chromium.org/2385423005/diff/140001/src/builtins/builtins-...
src/builtins/builtins-object.cc:533: a->GotoUnless(a->HasInstanceType(weak_cell,
WEAK_CELL_TYPE),
On 2016/10/18 at 12:33:21, Igor Sheludko wrote:
> Probably it should be an Assert instead. And map comparison will be faster.

Added a test to cover the uninitlized ObjectCreateMap case on the PrototypeInfo.

https://codereview.chromium.org/2385423005/diff/140001/src/code-stub-assemble...
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2385423005/diff/140001/src/code-stub-assemble...
src/code-stub-assembler.cc:986: Node*
CodeStubAssembler::LoadMapNumberOfOwnDescriptors(Node* map) {
On 2016/10/18 at 12:33:21, Igor Sheludko wrote:
> Semi-useful. Usually we load this number after a fast-map bit check which is
also a part of bit_field3. So here we are going to load the bit field second
time.

removing it.

https://codereview.chromium.org/2385423005/diff/140001/src/code-stub-assemble...
src/code-stub-assembler.cc:1000: GotoUnless(HasInstanceType(prototype_info,
PROTOTYPE_INFO_TYPE),
On 2016/10/18 at 12:33:21, Igor Sheludko wrote:
> Map check is better here.

done.

https://codereview.chromium.org/2385423005/diff/140001/src/code-stub-assemble...
src/code-stub-assembler.cc:1050: Node* CodeStubAssembler::IsFastMap(Node* map) {
On 2016/10/18 at 12:33:21, Igor Sheludko wrote:
> Semi-useful. Same a above.

agreed, removed it.

https://codereview.chromium.org/2385423005/diff/140001/src/code-stub-assemble...
src/code-stub-assembler.cc:1514: StoreObjectFieldNoWriteBarrier(object,
JSObject::kPropertiesOffset,
On 2016/10/18 at 12:33:21, Igor Sheludko wrote:
> Please add a comment about why is it possible to skip write barriers in this
function. Maybe assert that the object is in new space.

done, added semi-assertion in AllocateJSObjectFromMap

https://codereview.chromium.org/2385423005/diff/140001/src/code-stub-assemble...
src/code-stub-assembler.cc:1539: Comment("StoreFieldsNoWriteBarrier");
On 2016/10/18 at 12:33:21, Igor Sheludko wrote:
> Consider using BuildFastLoop().

done.

https://codereview.chromium.org/2385423005/diff/140001/src/code-stub-assemble...
src/code-stub-assembler.cc:1540: Variable current(this,
MachineRepresentation::kTagged);
On 2016/10/18 at 12:33:22, Igor Sheludko wrote:
> Why is it kTagged? I think it should be PointerRepresentation() instead.

done.

https://codereview.chromium.org/2385423005/diff/140001/src/code-stub-assemble...
src/code-stub-assembler.cc:1544: end_address = IntPtrSub(end_address,
IntPtrConstant(1));
On 2016/10/18 at 12:33:21, Igor Sheludko wrote:
> And thus this would not be necessary.

done.

[Camillo Bruni, 2016-10-18 14:12:03]

PTAL again

[Igor Sheludko, 2016-10-18 14:22:19]

lgtm with nits:

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

https://codereview.chromium.org/2385423005/diff/160001/src/builtins/builtins-...
src/builtins/builtins-object.cc:492: //
Spurious change.

https://codereview.chromium.org/2385423005/diff/160001/src/compiler/code-asse...
File src/compiler/code-assembler.cc (right):

https://codereview.chromium.org/2385423005/diff/160001/src/compiler/code-asse...
src/compiler/code-assembler.cc:224: /* return WordNotEqual(IntPtrConstant(0), */
?

https://codereview.chromium.org/2385423005/diff/180001/src/code-stub-assemble...
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2385423005/diff/180001/src/code-stub-assemble...
src/code-stub-assembler.cc:767: return IntPtrLessThanOrEqual(size,
IntPtrConstant(kMaxRegularHeapObjectSize));
UintPtrLessThanOrEqual

[Camillo Bruni, 2016-10-18 14:49:24]

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

[commit-bot: I haz the power, 2016-10-18 14:49:29]

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

[Camillo Bruni, 2016-10-18 14:49:44]

cbruni@chromium.org changed reviewers:
+ jarin@chromium.org

[Camillo Bruni, 2016-10-18 14:49:45]

jarin@ PTAL src/compiler/*

[Igor Sheludko, 2016-10-19 06:50:56]

More nits:

https://codereview.chromium.org/2385423005/diff/240001/src/code-stub-assemble...
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2385423005/diff/240001/src/code-stub-assemble...
src/code-stub-assembler.cc:1696: Node* start_address = IntPtrAdd(object,
IntPtrConstant(start_offset - 1));
s/1/kHeapObjectTag/

https://codereview.chromium.org/2385423005/diff/240001/src/code-stub-assemble...
src/code-stub-assembler.cc:1697: Node* end_address = IntPtrSub(IntPtrAdd(object,
size), IntPtrConstant(1));
Same here.

[Jarin, 2016-10-19 07:24:16]

https://codereview.chromium.org/2385423005/diff/240001/src/compiler/code-asse...
File src/compiler/code-assembler.h (right):

https://codereview.chromium.org/2385423005/diff/240001/src/compiler/code-asse...
src/compiler/code-assembler.h:290: Node* IsWordAligned(Node* value);
I think this should not be here, code-assembler is meant to be a lean assembler
without macro-operations.

Can't you move it to code-stub-assembler?

[Camillo Bruni, 2016-10-19 07:38:40]

On 2016/10/19 at 07:24:16, jarin wrote:
>
https://codereview.chromium.org/2385423005/diff/240001/src/compiler/code-asse...
> File src/compiler/code-assembler.h (right):
> 
>
https://codereview.chromium.org/2385423005/diff/240001/src/compiler/code-asse...
> src/compiler/code-assembler.h:290: Node* IsWordAligned(Node* value);
> I think this should not be here, code-assembler is meant to be a lean
assembler without macro-operations.
> 
> Can't you move it to code-stub-assembler?

No problem, will move it up.

[Camillo Bruni, 2016-10-19 07:50:02]

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

[Camillo Bruni, 2016-10-19 08:44:52]

The CQ bit was unchecked by cbruni@chromium.org

[Jarin, 2016-10-19 09:20:44]

src/compiler/* lgtm. Thanks!

[Camillo Bruni, 2016-10-19 09:24:28]

The CQ bit was checked by cbruni@chromium.org

[Camillo Bruni, 2016-10-19 09:24:28]

The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org
Link to the patchset: https://codereview.chromium.org/2385423005/#ps280001
(title: "moving IsWordAligned to CodeStubAssembler and removing BranchIf")

[commit-bot: I haz the power, 2016-10-19 09:24:32]

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

[Camillo Bruni, 2016-10-19 10:26:23]

The CQ bit was checked by cbruni@chromium.org

[Camillo Bruni, 2016-10-19 10:26:24]

The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org,
jarin@chromium.org
Link to the patchset: https://codereview.chromium.org/2385423005/#ps300001
(title: "removing unused symbol")

[commit-bot: I haz the power, 2016-10-19 10:26:31]

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-19 11:33:37]

Committed patchset #16 (id:300001)

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

Description was changed from

==========
[stubs] Implement TF builtin for Object.create fast paths 

BUG=
==========

to

==========
[stubs] Implement TF builtin for Object.create fast paths

BUG=

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

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

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