[compiler] Sanitize entry points to LookupSlot access.

[compiler] Sanitize entry points to LookupSlot access.

Add dedicated %LoadLookupSlot, %LoadLookupSlotInsideTypeof,
%LoadLookupSlotForCall, %StoreLookupSlot_Sloppy and
%StoreLookupSlot_Strict runtime entry points and use them
appropriately in the various compilers. This way we can
finally drop the machine operators from the JS graph level
completely in TurboFan.

Also drop the funky JSLoadDynamic operator from TurboFan,
which was by now just a small wrapper around the runtime
call to %LoadLookupSlot.

R=mstarzinger@chromium.org

Committed: https://crrev.com/4ff159bd28be36a39a1f8416cdf8fccafd3c2f95
Cr-Commit-Position: refs/heads/master@{#33880}

[Benedikt Meurer, 2016-02-10 13:25:43]

Hey Michi,

Here's the baby, finally getting rid of the machine operators in the graph
builders. :-)
Please take a look.

Thanks,
Benedikt

[rmcilroy, 2016-02-10 13:39:30]

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

[rmcilroy, 2016-02-10 13:39:31]

Drive-by comment on interpreter changes, otherwise LG.

https://codereview.chromium.org/1683103002/diff/1/src/interpreter/bytecode-ge...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1683103002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:2210: Register name =
register_allocator()->NextConsecutiveRegister();
No need for PrepareForConsecutiveAllocations ... NextConsecutiveRegister... when
there is only a single argument, just:

Register name = register_allocator()->NewRegister();

[Benedikt Meurer, 2016-02-10 13:50:06]

https://codereview.chromium.org/1683103002/diff/1/src/interpreter/bytecode-ge...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1683103002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:2210: Register name =
register_allocator()->NextConsecutiveRegister();
On 2016/02/10 13:39:31, rmcilroy wrote:
> No need for PrepareForConsecutiveAllocations ... NextConsecutiveRegister...
when
> there is only a single argument, just:
> 
> Register name = register_allocator()->NewRegister();

Done.

[rmcilroy, 2016-02-10 13:54:10]

https://codereview.chromium.org/1683103002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1683103002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:2209: Register name =
register_allocator()->NextConsecutiveRegister();
This needs to be NewRegister, not NextConsecutiveRegister (this will break).

[Benedikt Meurer, 2016-02-10 14:05:37]

https://codereview.chromium.org/1683103002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1683103002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:2209: Register name =
register_allocator()->NextConsecutiveRegister();
Thanks.

[Benedikt Meurer, 2016-02-10 17:48:14]

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

[commit-bot: I haz the power, 2016-02-10 17:48:23]

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

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

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

[commit-bot: I haz the power, 2016-02-10 18:03:28]

Dry run: Try jobs failed on following builders:
  v8_linux_arm_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel/builds/13554)

[Benedikt Meurer, 2016-02-10 18:13:54]

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

[commit-bot: I haz the power, 2016-02-10 18:14:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1683103002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1683103002/100001

[commit-bot: I haz the power, 2016-02-10 18:34:31]

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

[commit-bot: I haz the power, 2016-02-10 18:34:32]

Dry run: This issue passed the CQ dry run.

[Michael Starzinger, 2016-02-10 19:48:16]

LGTM. Nice. Just some nits to address.

https://codereview.chromium.org/1683103002/diff/100001/src/compiler/ast-graph...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/1683103002/diff/100001/src/compiler/ast-graph...
src/compiler/ast-graph-builder.cc:3410: Node* value = NewNode(op,
jsgraph()->Constant(name));
nit: Can we wrap this into a helper with the following signature?
Node* BuildDynamicLoad(Handle<Name> name, TypeofMode typeof_mode);

https://codereview.chromium.org/1683103002/diff/100001/src/compiler/ast-graph...
src/compiler/ast-graph-builder.cc:3573: Node* store = NewNode(op, name, value);
nit: Can we wrap this into a helper with the following signature?
Node* BuildDynamicStore(Handle<Name> name, Node* value);

https://codereview.chromium.org/1683103002/diff/100001/src/compiler/ast-graph...
src/compiler/ast-graph-builder.cc:3914: Node* slow = NewNode(op,
jsgraph()->Constant(name));
nit: Use aforementioned helper.

https://codereview.chromium.org/1683103002/diff/100001/src/compiler/ast-graph...
src/compiler/ast-graph-builder.cc:3961: Node* slow = NewNode(op,
jsgraph()->Constant(name));
nit: Use aforementioned helper.

https://codereview.chromium.org/1683103002/diff/100001/src/runtime/runtime-sc...
File src/runtime/runtime-scopes.cc (right):

https://codereview.chromium.org/1683103002/diff/100001/src/runtime/runtime-sc...
src/runtime/runtime-scopes.cc:1075: Handle<String> name = args.at<String>(0);
nit: Please use CONVERT_ARG_HANDLE_CHECKED(String, name, 0) here as well instead
of the DCHECK. Eventually all of them will become hard-crash checks.

[Benedikt Meurer, 2016-02-11 05:25:05]

Great suggestions thanks.

https://codereview.chromium.org/1683103002/diff/100001/src/compiler/ast-graph...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/1683103002/diff/100001/src/compiler/ast-graph...
src/compiler/ast-graph-builder.cc:3410: Node* value = NewNode(op,
jsgraph()->Constant(name));
On 2016/02/10 19:48:16, Michael Starzinger wrote:
> nit: Can we wrap this into a helper with the following signature?
> Node* BuildDynamicLoad(Handle<Name> name, TypeofMode typeof_mode);

Done.

https://codereview.chromium.org/1683103002/diff/100001/src/compiler/ast-graph...
src/compiler/ast-graph-builder.cc:3573: Node* store = NewNode(op, name, value);
On 2016/02/10 19:48:16, Michael Starzinger wrote:
> nit: Can we wrap this into a helper with the following signature?
> Node* BuildDynamicStore(Handle<Name> name, Node* value);

Done.

https://codereview.chromium.org/1683103002/diff/100001/src/compiler/ast-graph...
src/compiler/ast-graph-builder.cc:3914: Node* slow = NewNode(op,
jsgraph()->Constant(name));
On 2016/02/10 19:48:16, Michael Starzinger wrote:
> nit: Use aforementioned helper.

Done.

https://codereview.chromium.org/1683103002/diff/100001/src/compiler/ast-graph...
src/compiler/ast-graph-builder.cc:3961: Node* slow = NewNode(op,
jsgraph()->Constant(name));
On 2016/02/10 19:48:16, Michael Starzinger wrote:
> nit: Use aforementioned helper.

Done.

https://codereview.chromium.org/1683103002/diff/100001/src/runtime/runtime-sc...
File src/runtime/runtime-scopes.cc (right):

https://codereview.chromium.org/1683103002/diff/100001/src/runtime/runtime-sc...
src/runtime/runtime-scopes.cc:1075: Handle<String> name = args.at<String>(0);
Not possible unfortunately until we change CONVERT_ARG_HANDLE_CHECKED to
CHECK/DCHECK instead of throw "Illegal access", since the runtime function
returns a pair.

[Benedikt Meurer, 2016-02-11 05:48:30]

The CQ bit was checked by bmeurer@chromium.org

[Benedikt Meurer, 2016-02-11 05:48:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1683103002/#ps120001
(title: "REBASE. Fixes. Comments.")

[commit-bot: I haz the power, 2016-02-11 05:48:42]

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

[commit-bot: I haz the power, 2016-02-11 06:24:17]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-02-11 10:41:04]

Description was changed from

==========
[compiler] Sanitize entry points to LookupSlot access.

Add dedicated %LoadLookupSlot, %LoadLookupSlotInsideTypeof,
%LoadLookupSlotForCall, %StoreLookupSlot_Sloppy and
%StoreLookupSlot_Strict runtime entry points and use them
appropriately in the various compilers. This way we can
finally drop the machine operators from the JS graph level
completely in TurboFan.

Also drop the funky JSLoadDynamic operator from TurboFan,
which was by now just a small wrapper around the runtime
call to %LoadLookupSlot.

R=mstarzinger@chromium.org
==========

to

==========
[compiler] Sanitize entry points to LookupSlot access.

Add dedicated %LoadLookupSlot, %LoadLookupSlotInsideTypeof,
%LoadLookupSlotForCall, %StoreLookupSlot_Sloppy and
%StoreLookupSlot_Strict runtime entry points and use them
appropriately in the various compilers. This way we can
finally drop the machine operators from the JS graph level
completely in TurboFan.

Also drop the funky JSLoadDynamic operator from TurboFan,
which was by now just a small wrapper around the runtime
call to %LoadLookupSlot.

R=mstarzinger@chromium.org

Committed: https://crrev.com/4ff159bd28be36a39a1f8416cdf8fccafd3c2f95
Cr-Commit-Position: refs/heads/master@{#33880}
==========

[commit-bot: I haz the power, 2016-02-11 10:41:04]

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