This is a large one :) First round of comments. I only looked at IA32 code. The things there also apply for other platforms.
In general we should think also about how we deal with expression contexts in our code generator once we introduce an accumulator and slot operands for evaluating expressions. http://codereview.chromium.org/405033/diff/1/5 File src/compiler.cc (right): http://codereview.chromium.org/405033/diff/1/5#newcode803 src/compiler.cc:803: BAILOUT("non-global/non-slot/non-property variable"); I think that should not happen, should it? ASSERT(property != NULL); http://codereview.chromium.org/405033/diff/1/7 File src/ia32/fast-codegen-ia32.cc (right): http://codereview.chromium.org/405033/diff/1/7#newcode88 src/ia32/fast-codegen-ia32.cc:88: // Assert we do not have to copy any parameters into the context. Change this comment => Copy parameters into context. http://codereview.chromium.org/405033/diff/1/7#newcode246 src/ia32/fast-codegen-ia32.cc:246: default: Do you need default at all? We handle all 4 possible enums for Slot. http://codereview.chromium.org/405033/diff/1/7#newcode333 src/ia32/fast-codegen-ia32.cc:333: case Expression::kValue:\ extra '\'? http://codereview.chromium.org/405033/diff/1/7#newcode958 src/ia32/fast-codegen-ia32.cc:958: Move(Expression::kValue, object_slot); Move(...) can destroy eax, maybe we should pass it as a scratch register explicitly. Otherwise the caller may not be aware of this. http://codereview.chromium.org/405033/diff/1/7#newcode965 src/ia32/fast-codegen-ia32.cc:965: __ mov(eax, Operand(esp, 2 * kPointerSize)); This line relies on the fact that the Move(...) before pushes the values on the stack. This may change in the future. Since we are pushing arguments to a call here I think its better to do sth. like: __ pop(eax) __ push(object_slot) __ push(key_literal) (and do the corresponding stack cleanup code after the KeyedStoreIC, also use a different scratch register for evaluating the slots then) General idea for later refactoring: Maybe we should split the Expression::kValue into different subcases: e.g. Stack, Register, or Slot. i.e. state explicitly where to put the value of an expression. http://codereview.chromium.org/405033 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
