Implement simple fast-path code for functions containing this property stores and global variables.

Implement simple fast-path code for functions containing this property stores and global variables.

Code is specialized to the initial receiver.

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

[Kevin Millikin (Chromium), 2010-01-29 17:50:34]

This change is not as big as it looks.

[antonm, 2010-01-29 19:16:11]

minor notes and one question for my education.

http://codereview.chromium.org/553149/diff/2001/2004
File src/arm/fast-codegen-arm.cc (right):

http://codereview.chromium.org/553149/diff/2001/2004#newcode47
src/arm/fast-codegen-arm.cc:47: __ tst(r1, Operand(kSmiTagMask));
maybe branchonsmi?

http://codereview.chromium.org/553149/diff/2001/2004#newcode62
src/arm/fast-codegen-arm.cc:62: // registers are esi (context) and possibly edx
(this).  Both are also
ia32-isms (esi, edx)?

http://codereview.chromium.org/553149/diff/2001/2010
File src/fast-codegen.cc (right):

http://codereview.chromium.org/553149/diff/2001/2010#newcode523
src/fast-codegen.cc:523: // Result is in eax by convention.
eax?

http://codereview.chromium.org/553149/diff/2001/2018
File src/x64/fast-codegen-x64.cc (right):

http://codereview.chromium.org/553149/diff/2001/2018#newcode48
src/x64/fast-codegen-x64.cc:48: __ j(is_smi, bailout());
just for my education: which is preferred, this or JumpIfSmi?

http://codereview.chromium.org/553149/diff/2001/2018#newcode61
src/x64/fast-codegen-x64.cc:61: // saved in the stack and esi is preserved by
the call.
same for esi and edx

[Lasse Reichstein, 2010-01-29 20:05:06]

http://codereview.chromium.org/553149/diff/2001/2018
File src/x64/fast-codegen-x64.cc (right):

http://codereview.chromium.org/553149/diff/2001/2018#newcode48
src/x64/fast-codegen-x64.cc:48: __ j(is_smi, bailout());
JumpIfSmi is preferred. 
It's a convenience function that does the same as these two lines (so there is
no difference in the resulting code), but it's *much* easier to read.

[Kasper Lund, 2010-02-01 09:29:02]

LGTM.

http://codereview.chromium.org/553149/diff/2001/2004
File src/arm/fast-codegen-arm.cc (right):

http://codereview.chromium.org/553149/diff/2001/2004#newcode72
src/arm/fast-codegen-arm.cc:72: int index = 2 +
function()->scope()->num_parameters();
Maybe refactor the code that loads this from the stack? It's used in two places.

http://codereview.chromium.org/553149/diff/2001/2005
File src/arm/full-codegen-arm.cc (right):

http://codereview.chromium.org/553149/diff/2001/2005#newcode84
src/arm/full-codegen-arm.cc:84: __ CallRuntime(Runtime::kNewContext, 1);
Consider using the faster new context stub if possible. At least add a TODO.

http://codereview.chromium.org/553149/diff/2001/2005#newcode99
src/arm/full-codegen-arm.cc:99: __ str(r0, MemOperand(cp,
Context::SlotOffset(slot->index())));
This doesn't use the write barrier and that is safe because? At least we should
have a comment.

http://codereview.chromium.org/553149/diff/2001/2005#newcode116
src/arm/full-codegen-arm.cc:116: fun->num_parameters() * kPointerSize));
Weird indentation.

http://codereview.chromium.org/553149/diff/2001/2007
File src/compiler.h (right):

http://codereview.chromium.org/553149/diff/2001/2007#newcode61
src/compiler.h:61: bool has_globals() { return has_globals_; }
has_this_properties, has_globals could be renamed to accesses_this_properties
and accesses_globals. I don't know if that's better though.

http://codereview.chromium.org/553149/diff/2001/2013
File src/full-codegen.h (right):

http://codereview.chromium.org/553149/diff/2001/2013#newcode66
src/full-codegen.h:66: enum Mode {
So you're falling back to the full code generator (SECONDARY), not to the
old-school optimizing one?

http://codereview.chromium.org/553149/diff/2001/2015
File src/ia32/fast-codegen-ia32.cc (right):

http://codereview.chromium.org/553149/diff/2001/2015#newcode44
src/ia32/fast-codegen-ia32.cc:44: // Offset 2 is due to return address and saved
frame pointer.
Refactor loading of this.

http://codereview.chromium.org/553149/diff/2001/2015#newcode67
src/ia32/fast-codegen-ia32.cc:67: // Restore this.
Refactor loading of this.

http://codereview.chromium.org/553149/diff/2001/2016
File src/ia32/full-codegen-ia32.cc (right):

http://codereview.chromium.org/553149/diff/2001/2016#newcode83
src/ia32/full-codegen-ia32.cc:83: __ CallRuntime(Runtime::kNewContext, 1);
Use fast new context stub.

http://codereview.chromium.org/553149/diff/2001/2016#newcode99
src/ia32/full-codegen-ia32.cc:99: __ mov(Operand(esi,
Context::SlotOffset(slot->index())), eax);
Comment on missing write barrier?

http://codereview.chromium.org/553149/diff/2001/2018
File src/x64/fast-codegen-x64.cc (right):

http://codereview.chromium.org/553149/diff/2001/2018#newcode46
src/x64/fast-codegen-x64.cc:46: __ movq(rdx, Operand(rbp, index *
kPointerSize));
Refactor loading of this.

http://codereview.chromium.org/553149/diff/2001/2018#newcode68
src/x64/fast-codegen-x64.cc:68: // Offset 2 is due to return address and saved
frame pointer.
Refactor loading of this.

http://codereview.chromium.org/553149/diff/2001/2019
File src/x64/full-codegen-x64.cc (right):

http://codereview.chromium.org/553149/diff/2001/2019#newcode83
src/x64/full-codegen-x64.cc:83: __ CallRuntime(Runtime::kNewContext, 1);
Use fast new context stub.

http://codereview.chromium.org/553149/diff/2001/2019#newcode99
src/x64/full-codegen-x64.cc:99: __ movq(Operand(rsi,
Context::SlotOffset(slot->index())), rax);
Comment on missing write barrier.

[fschneider, 2010-02-01 10:04:59]

LGTM!

http://codereview.chromium.org/553149/diff/2001/2010
File src/fast-codegen.cc (right):

http://codereview.chromium.org/553149/diff/2001/2010#newcode362
src/fast-codegen.cc:362: full_cgen.Generate(fun, FullCodeGenerator::SECONDARY);
Should we also pass the CompilationInfo to the full code generator to propagate
loop nesting information, etc.?

http://codereview.chromium.org/553149/diff/2001/2010#newcode368
src/fast-codegen.cc:368: Code::Flags flags = Code::ComputeFlags(Code::FUNCTION,
NOT_IN_LOOP);
(info->loop_nesting() == 0 ? NOT_IN_LOOP : IN_LOOP) instead?

http://codereview.chromium.org/553149/diff/2001/2010#newcode537
src/fast-codegen.cc:537: expr->num(), expr->num(), *name_string,
expr->value()->num());
As a separate change: It would be nice to emit these as codegen comments, too.

[Kevin Millikin (Chromium), 2010-02-01 10:38:40]

Thanks for the comments.  I'll work on incorporating them.

I'm not sure about the missing write barrier when writing context parameters. 
We should look into it because that code is unchanged from bleeding_edge.

I can do that when I get to work, but that will be about 7 hours from now....

http://codereview.chromium.org/553149/diff/2001/2005
File src/arm/full-codegen-arm.cc (right):

http://codereview.chromium.org/553149/diff/2001/2005#newcode84
src/arm/full-codegen-arm.cc:84: __ CallRuntime(Runtime::kNewContext, 1);
On 2010/02/01 09:29:02, Kasper Lund wrote:
> Consider using the faster new context stub if possible. At least add a TODO.

I didn't change this code at all (to be honest, I didn't even look at it).  I
guess the new context stub was never ported to the full code generator.

http://codereview.chromium.org/553149/diff/2001/2005#newcode99
src/arm/full-codegen-arm.cc:99: __ str(r0, MemOperand(cp,
Context::SlotOffset(slot->index())));
On 2010/02/01 09:29:02, Kasper Lund wrote:
> This doesn't use the write barrier and that is safe because? At least we
should
> have a comment.

I'm not sure that it is safe.  We should take a look because this whole code
block inside if (mode == PRIMARY) is on bleeding_edge.

http://codereview.chromium.org/553149/diff/2001/2010
File src/fast-codegen.cc (right):

http://codereview.chromium.org/553149/diff/2001/2010#newcode362
src/fast-codegen.cc:362: full_cgen.Generate(fun, FullCodeGenerator::SECONDARY);
On 2010/02/01 10:04:59, fschneider wrote:
> Should we also pass the CompilationInfo to the full code generator to
propagate
> loop nesting information, etc.?

If we need it, we will pass it.  Right now it would be unused.

http://codereview.chromium.org/553149/diff/2001/2010#newcode368
src/fast-codegen.cc:368: Code::Flags flags = Code::ComputeFlags(Code::FUNCTION,
NOT_IN_LOOP);
On 2010/02/01 10:04:59, fschneider wrote:
> (info->loop_nesting() == 0 ? NOT_IN_LOOP : IN_LOOP) instead?

I'm not sure that's right.  I don't know if we use the IN_LOOP flag for code
objects other than CallICs.  This function is just building the code of a
regular JS Function.

I'll take a look.

http://codereview.chromium.org/553149/diff/2001/2010#newcode537
src/fast-codegen.cc:537: expr->num(), expr->num(), *name_string,
expr->value()->num());
On 2010/02/01 10:04:59, fschneider wrote:
> As a separate change: It would be nice to emit these as codegen comments, too.

As we discussed, that's the intent (probably GC the --print-ir flag too, because
it will be reduntant).

I want a Comment constructor that takes a printf style format string and
varargs.  Until we have that, it's too annoying to use our abstracted sprintf at
every comment site.

http://codereview.chromium.org/553149/diff/2001/2013
File src/full-codegen.h (right):

http://codereview.chromium.org/553149/diff/2001/2013#newcode66
src/full-codegen.h:66: enum Mode {
On 2010/02/01 09:29:02, Kasper Lund wrote:
> So you're falling back to the full code generator (SECONDARY), not to the
> old-school optimizing one?

Yes initially, for simplicity.

The full codegen was built with this use case in mind---it can take an assembler
as input and generate code into the same buffer.  The plumbing on the classic
code generator needs to be rerouted to make that work.