[Interpreter] Unify global and unallocated variable access.

[Interpreter] Unify global and unallocated variable access.

Unifies the global and unallocated variable type accesses given that
--global_var_shortcuts is going away. Lda/StaGlobal is modified to use
Load/StoreICs on the global object. The named LoadIC and StoreIC bytecodes
are also modified so that they take a constant pool entry index for the
name rather than a register, avoiding unecessary LdaConstant bytecodes to
be emitted.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/9a594e783add26ecabc74f611d7e31b9a549ee65
Cr-Commit-Position: refs/heads/master@{#31482}

[rmcilroy, 2015-10-22 09:17:28]

rmcilroy@chromium.org changed reviewers:
+ bmeurer@chromium.org, oth@chromium.org

[rmcilroy, 2015-10-22 09:17:28]

Orion / Benedikt, please take a look. This change is due to the change in Global
variables in https://codereview.chromium.org/1422463004/ and
https://codereview.chromium.org/1419823003/.

Igor / Mythri FYI.

[Igor Sheludko, 2015-10-22 09:58:35]

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

[Igor Sheludko, 2015-10-22 09:58:36]

https://codereview.chromium.org/1419003002/diff/1/src/interpreter/bytecode-ar...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1419003002/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-builder.cc:926: Bytecode
BytecodeArrayBuilder::BytecodeForLoadGlobal(
It would be necessary to also propagate a TypeofMode to the IC.

[rmcilroy, 2015-10-22 10:13:20]

https://codereview.chromium.org/1419003002/diff/1/src/interpreter/bytecode-ar...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1419003002/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-builder.cc:926: Bytecode
BytecodeArrayBuilder::BytecodeForLoadGlobal(
On 2015/10/22 09:58:36, Igor Sheludko wrote:
> It would be necessary to also propagate a TypeofMode to the IC.

What does TypeofMode get used for, I can't see it used in the LoadICs? It's not
easy to propagate this information (we would need another 2 bytecodes for
LdaGlobalTypeOfSloppy / LdaGlobalTypeOfStrict if we need to call different ICs
based on this info.

[oth, 2015-10-22 10:54:30]

lgtm with minor comments.

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

https://codereview.chromium.org/1419003002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:1197: AccumulatorResultScope
accumulator_execution_result(this);
A comment here to say VisitProperyLoad here will not allocate temporaries or
call SetAccumulatorResult().

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

https://codereview.chromium.org/1419003002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1116:
execution_result()->SetResultInAccumulator();
I find this function scary. It is setting the result in the accumulator, but no
longer signalling that and relying on the layer above to do so. Probably need a
comment here too or leave as-is so there's a consistent pattern.

https://codereview.chromium.org/1419003002/diff/20001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1419003002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:398: // the in constant pool entry <name> with
the value in the accumulator.
For clarity, might change <name> to <name_index> or similar.

https://codereview.chromium.org/1419003002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:409: // the name in constant pool entry <name>
with the value in the accumulator.
Ditto.

[Benedikt Meurer, 2015-10-22 11:11:15]

lgtm

[rmcilroy, 2015-10-22 13:27:48]

https://codereview.chromium.org/1419003002/diff/1/src/interpreter/bytecode-ar...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1419003002/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-builder.cc:926: Bytecode
BytecodeArrayBuilder::BytecodeForLoadGlobal(
On 2015/10/22 10:13:20, rmcilroy wrote:
> On 2015/10/22 09:58:36, Igor Sheludko wrote:
> > It would be necessary to also propagate a TypeofMode to the IC.
> 
> What does TypeofMode get used for, I can't see it used in the LoadICs? It's
not
> easy to propagate this information (we would need another 2 bytecodes for
> LdaGlobalTypeOfSloppy / LdaGlobalTypeOfStrict if we need to call different ICs
> based on this info.

Added a TODO in BytecodeGenerator VisitTypeOf for now.

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

https://codereview.chromium.org/1419003002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:1197: AccumulatorResultScope
accumulator_execution_result(this);
On 2015/10/22 10:54:30, oth wrote:
> A comment here to say VisitProperyLoad here will not allocate temporaries or
> call SetAccumulatorResult().

N/A now.

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

https://codereview.chromium.org/1419003002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1116:
execution_result()->SetResultInAccumulator();
On 2015/10/22 10:54:30, oth wrote:
> I find this function scary. It is setting the result in the accumulator, but
no
> longer signalling that and relying on the layer above to do so. Probably need
a
> comment here too or leave as-is so there's a consistent pattern.

Done - added VisitPropertyLoadForAccumulator for this and added some
AccumulatorResultScopes (for documentation more than anything) to some of the
other visitors we have which store the result in the accumulator.

https://codereview.chromium.org/1419003002/diff/20001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1419003002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:398: // the in constant pool entry <name> with
the value in the accumulator.
On 2015/10/22 10:54:30, oth wrote:
> For clarity, might change <name> to <name_index> or similar.

Done. Also for Lda/StaGlobals above.

https://codereview.chromium.org/1419003002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:409: // the name in constant pool entry <name>
with the value in the accumulator.
On 2015/10/22 10:54:30, oth wrote:
> Ditto.

Done.

[rmcilroy, 2015-10-22 13:28:50]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2015-10-22 13:28:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, oth@chromium.org
Link to the patchset: https://codereview.chromium.org/1419003002/#ps40001
(title: "Address review comments.")

[commit-bot: I haz the power, 2015-10-22 13:29:02]

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

[commit-bot: I haz the power, 2015-10-22 13:30:37]

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

[commit-bot: I haz the power, 2015-10-22 13:30:37]

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

[rmcilroy, 2015-10-22 13:57:37]

The CQ bit was checked by rmcilroy@chromium.org

[commit-bot: I haz the power, 2015-10-22 13:57:45]

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

[commit-bot: I haz the power, 2015-10-22 14:10:03]

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

[commit-bot: I haz the power, 2015-10-22 14:10:03]

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

[rmcilroy, 2015-10-22 14:14:29]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2015-10-22 14:14:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, oth@chromium.org
Link to the patchset: https://codereview.chromium.org/1419003002/#ps60001
(title: "Fix unallocated variable error")

[commit-bot: I haz the power, 2015-10-22 14:14:46]

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

[commit-bot: I haz the power, 2015-10-22 14:55:37]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-10-22 14:56:25]

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

[Igor Sheludko, 2015-11-03 17:52:56]

Forgot to publish my drafts.

https://codereview.chromium.org/1419003002/diff/1/src/interpreter/bytecode-ar...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1419003002/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-builder.cc:926: Bytecode
BytecodeArrayBuilder::BytecodeForLoadGlobal(
On 2015/10/22 10:13:20, rmcilroy wrote:
> On 2015/10/22 09:58:36, Igor Sheludko wrote:
> > It would be necessary to also propagate a TypeofMode to the IC.
> 
> What does TypeofMode get used for, I can't see it used in the LoadICs? It's
not
> easy to propagate this information (we would need another 2 bytecodes for
> LdaGlobalTypeOfSloppy / LdaGlobalTypeOfStrict if we need to call different ICs
> based on this info.

See LoadIC::ShoudlThrowReferenceError(). Referencing of non-existing variable
should not throw if it is mentioned inside typeof. The LoadGlobalViaContext
implementation also had a bug related to typeof use case.

[rmcilroy, 2015-11-03 18:26:12]

https://codereview.chromium.org/1419003002/diff/1/src/interpreter/bytecode-ar...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1419003002/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-builder.cc:926: Bytecode
BytecodeArrayBuilder::BytecodeForLoadGlobal(
On 2015/11/03 17:52:56, Igor Sheludko wrote:
> On 2015/10/22 10:13:20, rmcilroy wrote:
> > On 2015/10/22 09:58:36, Igor Sheludko wrote:
> > > It would be necessary to also propagate a TypeofMode to the IC.
> > 
> > What does TypeofMode get used for, I can't see it used in the LoadICs? It's
> not
> > easy to propagate this information (we would need another 2 bytecodes for
> > LdaGlobalTypeOfSloppy / LdaGlobalTypeOfStrict if we need to call different
ICs
> > based on this info.
> 
> See LoadIC::ShoudlThrowReferenceError(). Referencing of non-existing variable
> should not throw if it is mentioned inside typeof. The LoadGlobalViaContext
> implementation also had a bug related to typeof use case.

Yeah, I figured this out and landed the fix in
http://codereview.chromium.org/1422443006/