Introduce a hydrogen value for contexts, support context slot assignment.

Introduce a hydrogen value for contexts, support context slot assignment.

Each context in the context chain has a corresponding hydrogen value.
The context values are used for global object lookup and context slot
lookup.  Add simple (non-compound) assignment to context slots.

Committed: http://code.google.com/p/v8/source/detail?r=6615

[Kevin Millikin (Chromium), 2011-01-27 13:36:25]

Request for comment.  The code needs some refactoring and needs to be ported.

[antonm, 2011-01-27 14:00:07]

Nice and thanks.

http://codereview.chromium.org/6390003/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/6390003/diff/1/src/hydrogen.cc#newcode3524
src/hydrogen.cc:3524: int context_chain_length =
should we refactor the code that builds context values chain into a helper
method?

http://codereview.chromium.org/6390003/diff/1/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/6390003/diff/1/src/ia32/lithium-codegen-ia32.c...
src/ia32/lithium-codegen-ia32.cc:1934: Operand(context,
Context::SlotOffset(Context::FCONTEXT_INDEX)));
maybe introduce additional instruction for this load not to repeat on each slot
load/store?

http://codereview.chromium.org/6390003/diff/1/src/ia32/lithium-codegen-ia32.c...
src/ia32/lithium-codegen-ia32.cc:2184: __ mov(result, esi);
Can we make it noop by requesting the result register to be esi?

[Vitaly Repeshko, 2011-01-27 14:05:21]

Drive by suggestion.

http://codereview.chromium.org/6390003/diff/1/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

http://codereview.chromium.org/6390003/diff/1/src/ia32/lithium-ia32.cc#newcod...
src/ia32/lithium-ia32.cc:1682: bool skip_write_barrier =
stored_value->type().IsSmi() ||
Add NeedsWriteBarrier predicate to HStoreContextSlot instruction like it's done
for HStore{Named,Keyed}.

[fschneider, 2011-01-28 11:54:50]

LGTM.

http://codereview.chromium.org/6390003/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/6390003/diff/1/src/hydrogen-instructions.h#new...
src/hydrogen-instructions.h:2645: virtual Representation
RequiredInputRepresentation(int index) const {
I think we are not consistent with when to override this function. Until now we
left the default implementation (Representation::None) for cases where we don't
ever expect a conversion of the inputs.

http://codereview.chromium.org/6390003/diff/1/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/6390003/diff/1/src/ia32/lithium-codegen-ia32.c...
src/ia32/lithium-codegen-ia32.cc:2184: __ mov(result, esi);
On 2011/01/27 14:00:07, antonm wrote:
> Can we make it noop by requesting the result register to be esi?

esi will be replaced by [fixed-stack-slot] - so i'd leave it as is for now.

[Kevin Millikin (Chromium), 2011-02-03 08:37:23]

http://codereview.chromium.org/6390003/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/6390003/diff/1/src/hydrogen-instructions.h#new...
src/hydrogen-instructions.h:2645: virtual Representation
RequiredInputRepresentation(int index) const {
On 2011/01/28 11:54:50, fschneider wrote:
> I think we are not consistent with when to override this function. Until now
we
> left the default implementation (Representation::None) for cases where we
don't
> ever expect a conversion of the inputs. 

Do you think that's a good idea?  It seems like another unsafe default.

http://codereview.chromium.org/6390003/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/6390003/diff/1/src/hydrogen.cc#newcode3524
src/hydrogen.cc:3524: int context_chain_length =
On 2011/01/27 14:00:07, antonm wrote:
> should we refactor the code that builds context values chain into a helper
> method?

Yes, because we'll need it for compound assignments and count operations too.

http://codereview.chromium.org/6390003/diff/1/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

http://codereview.chromium.org/6390003/diff/1/src/ia32/lithium-ia32.cc#newcod...
src/ia32/lithium-ia32.cc:1682: bool skip_write_barrier =
stored_value->type().IsSmi() ||
On 2011/01/27 14:05:21, Vitaly wrote:
> Add NeedsWriteBarrier predicate to HStoreContextSlot instruction like it's
done
> for HStore{Named,Keyed}.

Yes, done.

[Kevin Millikin (Chromium), 2011-02-03 11:25:40]

Comments addressed and ported to the other architectures (implemented on ARM,
unimplemented on x64).

[fschneider, 2011-02-03 13:08:02]

LGTM.

http://codereview.chromium.org/6390003/diff/14001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

http://codereview.chromium.org/6390003/diff/14001/src/ia32/lithium-ia32.cc#ne...
src/ia32/lithium-ia32.cc:1696: context = UseRegisterAtStart(instr->context());
UseTempRegister if it is overwritten.

[fschneider, 2011-02-03 13:08:02]

LGTM.

http://codereview.chromium.org/6390003/diff/14001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

http://codereview.chromium.org/6390003/diff/14001/src/ia32/lithium-ia32.cc#ne...
src/ia32/lithium-ia32.cc:1696: context = UseRegisterAtStart(instr->context());
UseTempRegister if it is overwritten.

[antonm, 2011-02-03 13:14:29]

LGTM

http://codereview.chromium.org/6390003/diff/14001/src/hydrogen.cc
File src/hydrogen.cc (left):

http://codereview.chromium.org/6390003/diff/14001/src/hydrogen.cc#oldcode2976
src/hydrogen.cc:2976: // TODO(antonm): if slot's value is not modified by
closures, instead
may we keep this TODO?

[Kevin Millikin (Chromium), 2011-02-04 11:49:09]

http://codereview.chromium.org/6390003/diff/14001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

http://codereview.chromium.org/6390003/diff/14001/src/ia32/lithium-ia32.cc#ne...
src/ia32/lithium-ia32.cc:1696: context = UseRegisterAtStart(instr->context());
On 2011/02/03 13:08:02, fschneider wrote:
> UseTempRegister if it is overwritten.

Thanks, very good catch.