Fast codegen: Implement with.

Fast codegen: Implement with.

[Lasse Reichstein, 2009-12-14 14:17:12]

Short review.

[Kevin Millikin (Chromium), 2009-12-14 16:35:47]

LGTM.

You should go back (in another change) and make use of the new abstractions for
frame and context accesses.

http://codereview.chromium.org/491077/diff/1/4
File src/fast-codegen.cc (right):

http://codereview.chromium.org/491077/diff/1/4#newcode341
src/fast-codegen.cc:341: if (FLAG_debug_info) {
SetStatementPosition(stmt)?

http://codereview.chromium.org/491077/diff/1/4#newcode361
src/fast-codegen.cc:361: if (FLAG_debug_info) {
Same.

http://codereview.chromium.org/491077/diff/1/5
File src/fast-codegen.h (right):

http://codereview.chromium.org/491077/diff/1/5#newcode294
src/fast-codegen.h:294: static Register context_register();
This seems like going overboard to avoid platform-specific code in the backend.

But if you want do this, we have lots of explicit references to the register in
the toplevel code generator that should change to use the new abstraction.

http://codereview.chromium.org/491077/diff/1/5#newcode298
src/fast-codegen.h:298: void StoreFrameField(int frame_offset, Register value);
Not all of the constants in frames-{arm,ia32,x64}.h are fp relative.  Probably
should mention that the offset here is fp relative.

We have a lot of uses of this pattern (and loading from a frame field, and
pushing one) in the fast code generator.  They should be rewritten.

For some reason, I like StoreTo... or Set... better as names.

http://codereview.chromium.org/491077/diff/1/5#newcode302
src/fast-codegen.h:302: void LoadContextField(Register dst, int context_index);
We have a lot of uses of this (or a similar) pattern too.  Search for
Context::SlotOffset and CodeGenerator::ContextOperand in the fast code
generator.

[Lasse Reichstein, 2009-12-16 13:40:31]

http://codereview.chromium.org/491077/diff/1/4
File src/fast-codegen.cc (right):

http://codereview.chromium.org/491077/diff/1/4#newcode341
src/fast-codegen.cc:341: if (FLAG_debug_info) {
Should have known it would be in the fast codegen as well.

http://codereview.chromium.org/491077/diff/1/4#newcode361
src/fast-codegen.cc:361: if (FLAG_debug_info) {
Fixed.

http://codereview.chromium.org/491077/diff/1/5
File src/fast-codegen.h (right):

http://codereview.chromium.org/491077/diff/1/5#newcode294
src/fast-codegen.h:294: static Register context_register();
The context_register() function is used in generic code (fast-codegen.cc), but
we could just as well have a SaveContextRegister function, because that's all
it's currently used for. I'll keep it for now and see if there will be more uses
for it.

The uses of this function in the backend specific files, e.g.
fast-codegen-x64.cc, is overdoing it. I'll remove those.

http://codereview.chromium.org/491077/diff/1/5#newcode298
src/fast-codegen.h:298: void StoreFrameField(int frame_offset, Register value);
Will mention exceptions and change name to StoreTo...

The Load/StoreTo functions are meant to be used from generic code
(fast-codegen.cc), but we can only do that when we have access to the register
being loaded/stored (currently only result_register() and context_register()). 

We can use the functions from the backend-specific code as well, but I don't
think it's really worth it. At that level, the proper abstraction would be an
Operand/MemOperand.

http://codereview.chromium.org/491077/diff/1/5#newcode302
src/fast-codegen.h:302: void LoadContextField(Register dst, int context_index);
I actually think ContextOperand is the correct level of abstraction for the
backend specific code.