[stubs] Optimize LoadGlobalViaContextStub and StoreGlobalViaContextStub.

[stubs] Optimize LoadGlobalViaContextStub and StoreGlobalViaContextStub.

This is the initial round of optimizations for the
LoadGlobalViaContextStub and StoreGlobalViaContextStub, basically
turning them into platform code stubs to avoid the Crankshaft overhead
in the fast case, and making the runtime interface cheaper.

R=ishell@chromium.org
BUG=chromium:510694
LOG=n

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

[Benedikt Meurer, 2015-07-17 09:38:04]

Hey Igor,

This is the x64 port. Please take a look, I'll finish the rest next week.

Thanks,
Benedikt

[Benedikt Meurer, 2015-07-17 09:46:11]

Hey MIPS/ARM people,

Please add appropriate ports to this change.

Thanks,
Benedikt

[martyn.capewell, 2015-07-17 10:57:47]

Ok, we're looking at ARM and ARM64.

[Benedikt Meurer, 2015-07-17 10:58:08]

On 2015/07/17 10:57:47, martyn.capewell wrote:
> Ok, we're looking at ARM and ARM64.

Ok, thanks a lot :-)

[Igor Sheludko, 2015-07-18 20:52:44]

LGTM with nits for patch set 2. Thanks for improving this!
Please land this after https://codereview.chromium.org/1228113008/ which will
require rebasing.

https://codereview.chromium.org/1238143002/diff/20001/src/compiler/js-generic...
File src/compiler/js-generic-lowering.cc (right):

https://codereview.chromium.org/1238143002/diff/20001/src/compiler/js-generic...
src/compiler/js-generic-lowering.cc:343: node->ReplaceInput(2, script_context); 
// Replace old context.
// Set new context ...

https://codereview.chromium.org/1238143002/diff/20001/src/compiler/js-generic...
src/compiler/js-generic-lowering.cc:344: node->RemoveInput(3);
// ... instead of old one.

https://codereview.chromium.org/1238143002/diff/20001/src/compiler/js-generic...
src/compiler/js-generic-lowering.cc:403: node->ReplaceInput(3, script_context); 
// Replace old context.
// Set new context ...

https://codereview.chromium.org/1238143002/diff/20001/src/compiler/js-generic...
src/compiler/js-generic-lowering.cc:404: node->RemoveInput(4);
// ... instead of old one.

https://codereview.chromium.org/1238143002/diff/20001/src/ia32/code-stubs-ia3...
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/1238143002/diff/20001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:5106: Register context_reg = esi;
Same comments as for x64.

https://codereview.chromium.org/1238143002/diff/20001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:5130: __ Pop(result_reg);
// Pop return address.

https://codereview.chromium.org/1238143002/diff/20001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:5240: __ Pop(cell_reg);
// Pop return address.

https://codereview.chromium.org/1238143002/diff/20001/src/interface-descripto...
File src/interface-descriptors.h (right):

https://codereview.chromium.org/1238143002/diff/20001/src/interface-descripto...
src/interface-descriptors.h:446: static const Register DepthRegister();
Remove this too.

https://codereview.chromium.org/1238143002/diff/20001/src/runtime/runtime-obj...
File src/runtime/runtime-object.cc (right):

https://codereview.chromium.org/1238143002/diff/20001/src/runtime/runtime-obj...
src/runtime/runtime-object.cc:444: script_context->set(slot,
*it.GetPropertyCell());
GC mole will complain about this change.

https://codereview.chromium.org/1238143002/diff/20001/src/runtime/runtime-obj...
src/runtime/runtime-object.cc:475: script_context->set(slot,
*it.GetPropertyCell());
GC mole will complain about this change.

https://codereview.chromium.org/1238143002/diff/20001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/1238143002/diff/20001/src/x64/code-stubs-x64....
src/x64/code-stubs-x64.cc:5041: Register context_reg = rsi;
What about using LoadGlobalViaContextDescriptor::SlotRegister() and friends
here?

https://codereview.chromium.org/1238143002/diff/20001/src/x64/code-stubs-x64....
src/x64/code-stubs-x64.cc:5050: context_reg = rdi;
Nice!

https://codereview.chromium.org/1238143002/diff/20001/src/x64/code-stubs-x64....
src/x64/code-stubs-x64.cc:5074: Register context_reg = rsi;
Same here.

https://codereview.chromium.org/1238143002/diff/20001/src/x64/code-stubs-x64....
src/x64/code-stubs-x64.cc:5131: // This can only be true for Constant and
ConstantType cells, because we
We could probably get here for PropertyCellType::kUndefined if we store an
undefined value. I think it's ok just to check that KindField part is kData.

[Igor Sheludko, 2015-07-19 20:29:30]

One more comment.

https://codereview.chromium.org/1238143002/diff/20001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/1238143002/diff/20001/src/x64/code-stubs-x64....
src/x64/code-stubs-x64.cc:5169: __ j(equal, &fast_case);
I think it is better to write field and RecordWriteField(..., OMIT_SMI_CHECK)
here instead of jumping to fast case that does a smi check in RecordWriteField()
again.

[paul.l..., 2015-07-20 06:12:13]

paul.lind@imgtec.com changed reviewers:
+ paul.lind@imgtec.com

[paul.l..., 2015-07-20 06:12:14]

MIPS ports for this CL are in https://codereview.chromium.org/1238253002/

[Igor Sheludko, 2015-07-20 09:01:17]

https://codereview.chromium.org/1238143002/diff/20001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/1238143002/diff/20001/src/x64/code-stubs-x64....
src/x64/code-stubs-x64.cc:5169: __ j(equal, &fast_case);
On 2015/07/19 20:29:29, Igor Sheludko wrote:
> I think it is better to write field and RecordWriteField(..., OMIT_SMI_CHECK)
> here instead of jumping to fast case that does a smi check in
RecordWriteField()
> again.

Even better idea: we could add an explicit smi check before the above
RecordWriteField(), which we can then call as RecordWriteField(...,
OMIT_SMI_CHECK) and jump there from here after storing to the cell.

[jbramley, 2015-07-21 10:25:30]

jacob.bramley@arm.com changed reviewers:
+ jacob.bramley@arm.com

[jbramley, 2015-07-21 10:25:31]

A few questions that came up whilst working on the ARM port. These probably
apply to the other ports too.

https://codereview.chromium.org/1238143002/diff/20001/src/ia32/full-codegen-i...
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/1238143002/diff/20001/src/ia32/full-codegen-i...
src/ia32/full-codegen-ia32.cc:1414: __
Move(LoadGlobalViaContextDescriptor::SlotRegister(), Immediate(slot));
Is it safe to pass raw (non-smi) integers to stubs? Can't CallStub trigger a GC?

https://codereview.chromium.org/1238143002/diff/20001/src/ia32/full-codegen-i...
src/ia32/full-codegen-ia32.cc:2693: int const slot = var->index() + 1;
`slot` shadows a function argument.

https://codereview.chromium.org/1238143002/diff/20001/src/ia32/full-codegen-i...
src/ia32/full-codegen-ia32.cc:2694: int const depth =
scope()->ContextChainLength(var->scope());
Also, isn't the preferred style "const int" in Google V8?

[martyn.capewell, 2015-07-21 17:46:25]

ARM and ARM64 changes are at https://codereview.chromium.org/1245983003

[Benedikt Meurer, 2015-07-24 05:18:29]

Thanks for the comments. Rebasing now.

https://codereview.chromium.org/1238143002/diff/20001/src/compiler/js-generic...
File src/compiler/js-generic-lowering.cc (right):

https://codereview.chromium.org/1238143002/diff/20001/src/compiler/js-generic...
src/compiler/js-generic-lowering.cc:343: node->ReplaceInput(2, script_context); 
// Replace old context.
On 2015/07/18 20:52:43, Igor Sheludko wrote:
> // Set new context ...

Done.

https://codereview.chromium.org/1238143002/diff/20001/src/compiler/js-generic...
src/compiler/js-generic-lowering.cc:344: node->RemoveInput(3);
On 2015/07/18 20:52:43, Igor Sheludko wrote:
> // ... instead of old one.

Done.

https://codereview.chromium.org/1238143002/diff/20001/src/compiler/js-generic...
src/compiler/js-generic-lowering.cc:403: node->ReplaceInput(3, script_context); 
// Replace old context.
On 2015/07/18 20:52:43, Igor Sheludko wrote:
> // Set new context ...

Done.

https://codereview.chromium.org/1238143002/diff/20001/src/compiler/js-generic...
src/compiler/js-generic-lowering.cc:404: node->RemoveInput(4);
On 2015/07/18 20:52:43, Igor Sheludko wrote:
> // ... instead of old one.

Done.

https://codereview.chromium.org/1238143002/diff/20001/src/ia32/code-stubs-ia3...
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/1238143002/diff/20001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:5106: Register context_reg = esi;
On 2015/07/18 20:52:43, Igor Sheludko wrote:
> Same comments as for x64.

Acknowledged.

https://codereview.chromium.org/1238143002/diff/20001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:5130: __ Pop(result_reg);
On 2015/07/18 20:52:43, Igor Sheludko wrote:
> // Pop return address.

Done.

https://codereview.chromium.org/1238143002/diff/20001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:5240: __ Pop(cell_reg);
On 2015/07/18 20:52:43, Igor Sheludko wrote:
> // Pop return address.

Done.

https://codereview.chromium.org/1238143002/diff/20001/src/interface-descripto...
File src/interface-descriptors.h (right):

https://codereview.chromium.org/1238143002/diff/20001/src/interface-descripto...
src/interface-descriptors.h:446: static const Register DepthRegister();
On 2015/07/18 20:52:43, Igor Sheludko wrote:
> Remove this too.

Done.

https://codereview.chromium.org/1238143002/diff/20001/src/runtime/runtime-obj...
File src/runtime/runtime-object.cc (right):

https://codereview.chromium.org/1238143002/diff/20001/src/runtime/runtime-obj...
src/runtime/runtime-object.cc:444: script_context->set(slot,
*it.GetPropertyCell());
On 2015/07/18 20:52:44, Igor Sheludko wrote:
> GC mole will complain about this change.

Done.

https://codereview.chromium.org/1238143002/diff/20001/src/runtime/runtime-obj...
src/runtime/runtime-object.cc:475: script_context->set(slot,
*it.GetPropertyCell());
On 2015/07/18 20:52:44, Igor Sheludko wrote:
> GC mole will complain about this change.

Done.

https://codereview.chromium.org/1238143002/diff/20001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/1238143002/diff/20001/src/x64/code-stubs-x64....
src/x64/code-stubs-x64.cc:5041: Register context_reg = rsi;
I explicitly changed it to not use those functions because that would make the
register allocation less readable. Now everything is explicit by just reading
the function.

https://codereview.chromium.org/1238143002/diff/20001/src/x64/code-stubs-x64....
src/x64/code-stubs-x64.cc:5050: context_reg = rdi;
On 2015/07/18 20:52:44, Igor Sheludko wrote:
> Nice!

Acknowledged.

https://codereview.chromium.org/1238143002/diff/20001/src/x64/code-stubs-x64....
src/x64/code-stubs-x64.cc:5074: Register context_reg = rsi;
See above.

https://codereview.chromium.org/1238143002/diff/20001/src/x64/code-stubs-x64....
src/x64/code-stubs-x64.cc:5131: // This can only be true for Constant and
ConstantType cells, because we
On 2015/07/18 20:52:44, Igor Sheludko wrote:
> We could probably get here for PropertyCellType::kUndefined if we store an
> undefined value. I think it's ok just to check that KindField part is kData.

Acknowledged.

https://codereview.chromium.org/1238143002/diff/20001/src/x64/code-stubs-x64....
src/x64/code-stubs-x64.cc:5169: __ j(equal, &fast_case);
On 2015/07/20 09:01:16, Igor Sheludko wrote:
> On 2015/07/19 20:29:29, Igor Sheludko wrote:
> > I think it is better to write field and RecordWriteField(...,
OMIT_SMI_CHECK)
> > here instead of jumping to fast case that does a smi check in
> RecordWriteField()
> > again.
> 
> Even better idea: we could add an explicit smi check before the above
> RecordWriteField(), which we can then call as RecordWriteField(...,
> OMIT_SMI_CHECK) and jump there from here after storing to the cell.

Done.

https://codereview.chromium.org/1238143002/diff/20001/src/x64/code-stubs-x64....
src/x64/code-stubs-x64.cc:5169: __ j(equal, &fast_case);
On 2015/07/19 20:29:29, Igor Sheludko wrote:
> I think it is better to write field and RecordWriteField(..., OMIT_SMI_CHECK)
> here instead of jumping to fast case that does a smi check in
RecordWriteField()
> again.

Acknowledged.

[Benedikt Meurer, 2015-07-24 06:21:56]

The CQ bit was checked by bmeurer@chromium.org

[Benedikt Meurer, 2015-07-24 06:21:57]

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/1238143002/#ps100001
(title: "Fix ARM typo.")

[commit-bot: I haz the power, 2015-07-24 06:22:06]

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

[commit-bot: I haz the power, 2015-07-24 07:16:54]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-07-24 07:17:14]

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

[Igor Sheludko, 2015-07-24 11:10:01]

DBC about mips ports:

https://codereview.chromium.org/1238143002/diff/100001/src/mips/code-stubs-mi...
File src/mips/code-stubs-mips.cc (right):

https://codereview.chromium.org/1238143002/diff/100001/src/mips/code-stubs-mi...
src/mips/code-stubs-mips.cc:5288: __ Addu(at, at, Operand(cp));
We should load the cell from context_reg here.

https://codereview.chromium.org/1238143002/diff/100001/src/mips/code-stubs-mi...
src/mips/code-stubs-mips.cc:5330: __ Addu(at, at, Operand(cp));
Same here.

https://codereview.chromium.org/1238143002/diff/100001/src/mips64/code-stubs-...
File src/mips64/code-stubs-mips64.cc (right):

https://codereview.chromium.org/1238143002/diff/100001/src/mips64/code-stubs-...
src/mips64/code-stubs-mips64.cc:5319: __ Daddu(at, at, Operand(cp));
We should load the cell from context_reg context.

https://codereview.chromium.org/1238143002/diff/100001/src/mips64/code-stubs-...
src/mips64/code-stubs-mips64.cc:5361: __ Daddu(at, at, Operand(cp));
Same here.

[balazs.kilvady, 2015-07-24 13:42:33]

We are working on fixing the MIPS ports.

[Igor Sheludko, 2015-07-24 14:18:26]

On 2015/07/24 13:42:33, balazs.kilvady wrote:
> We are working on fixing the MIPS ports.

We already fixed them: https://codereview.chromium.org/1257603004/