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.

http://codereview.chromium.org/553149

--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev

Reply via email to