Cleanup of contexts in the full code generator.

Cleanup of contexts in the full code generator.

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

[Erik Corry, 2010-09-15 08:07:52]

This is work in progress, but I don't want to waste any more time if the
approach is not right.  The main file I want you to look at is
full-codegen-ia32.cc.  The other files still have stuff that needs cleaning up,
but I want to be careful about the order I do it in so as not to forego the
possibility of getting good error messages when I do the other architectures. 
Obviously the name new_context is pretty poor and will be replaced by context. 
The expression contexts will hopefully disappear completely.

I split the value context into two because there was almost no code shared
between the accumulator and stack states of that context.  The net result is
cleaner, I feel.

There are numerous long lines and minor formatting things which will be gone
through mechanically later.

[Kevin Millikin (Chromium), 2010-09-15 09:04:08]

This approach seems good.  Comments below.

http://codereview.chromium.org/3449004/diff/1/2
File src/ast.h (right):

http://codereview.chromium.org/3449004/diff/1/2#newcode181
src/ast.h:181: // Evaluated for its value (and side effects).  Result on stack.
The distinction between Stack/Accumulator is specific to a particular AST
visitor (the FullCodeGenerator), so maybe this whole enum should be moved out of
Expression and into FullCodeGenerator::Context.

http://codereview.chromium.org/3449004/diff/1/2#newcode184
src/ast.h:184: kValueAccumulator,
The names should match the classes in the full code generator: kStackValue,
kAccumulatorValue.

http://codereview.chromium.org/3449004/diff/1/4
File src/full-codegen.h (right):

http://codereview.chromium.org/3449004/diff/1/4#newcode74
src/full-codegen.h:74: new_context_(NULL),
It's confusing to have both context_ and new_context_ here.  Don't they change
in lockstep?

Can context_ be a member of new_context_ (and rename context_ ==> kind_,
new_context_ ==> context_)?

http://codereview.chromium.org/3449004/diff/1/4#newcode255
src/full-codegen.h:255: void Apply(Register reg) { new_context_->Apply(reg); }
To me, these Apply helpers that don't mention the context are weird to read at
the call site.  It doesn't hurt to much to just get rid of them and write

context()->Apply(eax);

or whatever at all the call sites, does it?

Maybe the "Apply" name is too cute.  "Plug" seems strictly better.

http://codereview.chromium.org/3449004/diff/1/4#newcode283
src/full-codegen.h:283: bool ContextIsEffect() {
This could be replaced by virtual type testers on the context classes.

http://codereview.chromium.org/3449004/diff/1/4#newcode337
src/full-codegen.h:337: Label* saved_true = true_label_;
Aren't these labels subsumed by the ones in the expression context?

http://codereview.chromium.org/3449004/diff/1/4#newcode491
src/full-codegen.h:491: ExpressionContext* new_context() { return new_context_;
}
Can this be const?

http://codereview.chromium.org/3449004/diff/1/4#newcode546
src/full-codegen.h:546: // Convert constant control flow (true or false) to the
result expected for
Rewrite all these comments so they don't mention "a given" context, because the
receiver is not so given.

http://codereview.chromium.org/3449004/diff/1/5
File src/ia32/full-codegen-ia32.cc (right):

http://codereview.chromium.org/3449004/diff/1/5#newcode1392
src/ia32/full-codegen-ia32.cc:1392: EmitNamedPropertyLoad(property);
You also want to visit the named (and keyed below) properties in an
AccumulatorValue context, not in whatever the outer context of the assignment
happens to be.

Even if they don't rely on the specific context (or assume AccumulatorValue), I
think it's safer to widen the scope of AccumulatorValueContext to include these
calls.

http://codereview.chromium.org/3449004/diff/1/5#newcode3428
src/ia32/full-codegen-ia32.cc:3428: if (context == Expression::kValueStack) {
This reads a bit better if you have something like:

bool should_push = (context == Expression::kValueStack);
ASSERT(should_push || context == Expression::kValueAccumulator);

up front and then:

if (should_push) __ push(eax);

At all push sites in this function.

http://codereview.chromium.org/3449004/diff/1/5#newcode3456
src/ia32/full-codegen-ia32.cc:3456: if (context ==
Expression::kValueAccumulator) {
It seems legitimate to change the calls to VisitForTypeofValue to have an
explicit ExpressionContext pushed instead of passing a token as argument:

{ StackValueContext ctxt(this);
  VisitForTypeofValue(expr->expression());
}

Then here in this case you can have a "tail call" that forwards to Visit in the
current context with just Visit(expr).

[Erik Corry, 2010-09-20 14:25:57]

http://codereview.chromium.org/3449004/diff/1/2
File src/ast.h (right):

http://codereview.chromium.org/3449004/diff/1/2#newcode181
src/ast.h:181: // Evaluated for its value (and side effects).  Result on stack.
On 2010/09/15 09:04:08, Kevin Millikin wrote:
> The distinction between Stack/Accumulator is specific to a particular AST
> visitor (the FullCodeGenerator), so maybe this whole enum should be moved out
of
> Expression and into FullCodeGenerator::Context.

This code is now gone.

http://codereview.chromium.org/3449004/diff/1/2#newcode184
src/ast.h:184: kValueAccumulator,
On 2010/09/15 09:04:08, Kevin Millikin wrote:
> The names should match the classes in the full code generator: kStackValue,
> kAccumulatorValue.

This code is now gone.

http://codereview.chromium.org/3449004/diff/1/4
File src/full-codegen.h (right):

http://codereview.chromium.org/3449004/diff/1/4#newcode74
src/full-codegen.h:74: new_context_(NULL),
On 2010/09/15 09:04:08, Kevin Millikin wrote:
> It's confusing to have both context_ and new_context_ here.  Don't they change
> in lockstep?
> 
> Can context_ be a member of new_context_ (and rename context_ ==> kind_,
> new_context_ ==> context_)?

That was an intermediate stage.  We now have only new_context and it is called
context

http://codereview.chromium.org/3449004/diff/1/4#newcode255
src/full-codegen.h:255: void Apply(Register reg) { new_context_->Apply(reg); }
On 2010/09/15 09:04:08, Kevin Millikin wrote:
> To me, these Apply helpers that don't mention the context are weird to read at
> the call site.  It doesn't hurt to much to just get rid of them and write
> 
> context()->Apply(eax);
> 
> or whatever at all the call sites, does it?
> 
> Maybe the "Apply" name is too cute.  "Plug" seems strictly better.

Done.

http://codereview.chromium.org/3449004/diff/1/4#newcode283
src/full-codegen.h:283: bool ContextIsEffect() {
On 2010/09/15 09:04:08, Kevin Millikin wrote:
> This could be replaced by virtual type testers on the context classes.

Done.

http://codereview.chromium.org/3449004/diff/1/4#newcode337
src/full-codegen.h:337: Label* saved_true = true_label_;
On 2010/09/15 09:04:08, Kevin Millikin wrote:
> Aren't these labels subsumed by the ones in the expression context?

Done.

http://codereview.chromium.org/3449004/diff/1/4#newcode491
src/full-codegen.h:491: ExpressionContext* new_context() { return new_context_;
}
On 2010/09/15 09:04:08, Kevin Millikin wrote:
> Can this be const?

Done.

http://codereview.chromium.org/3449004/diff/1/4#newcode546
src/full-codegen.h:546: // Convert constant control flow (true or false) to the
result expected for
On 2010/09/15 09:04:08, Kevin Millikin wrote:
> Rewrite all these comments so they don't mention "a given" context, because
the
> receiver is not so given.

Done.

http://codereview.chromium.org/3449004/diff/1/5
File src/ia32/full-codegen-ia32.cc (right):

http://codereview.chromium.org/3449004/diff/1/5#newcode1392
src/ia32/full-codegen-ia32.cc:1392: EmitNamedPropertyLoad(property);
On 2010/09/15 09:04:08, Kevin Millikin wrote:
> You also want to visit the named (and keyed below) properties in an
> AccumulatorValue context, not in whatever the outer context of the assignment
> happens to be.
> 
> Even if they don't rely on the specific context (or assume AccumulatorValue),
I
> think it's safer to widen the scope of AccumulatorValueContext to include
these
> calls.

Done.

http://codereview.chromium.org/3449004/diff/1/5#newcode3428
src/ia32/full-codegen-ia32.cc:3428: if (context == Expression::kValueStack) {
On 2010/09/15 09:04:08, Kevin Millikin wrote:
> This reads a bit better if you have something like:
> 
> bool should_push = (context == Expression::kValueStack);
> ASSERT(should_push || context == Expression::kValueAccumulator);
> 
> up front and then:
> 
> if (should_push) __ push(eax);
> 
> At all push sites in this function.

Actually after the change below this just turns into context()->Apply(eax) and I
can get rid of the if around the push.

http://codereview.chromium.org/3449004/diff/1/5#newcode3456
src/ia32/full-codegen-ia32.cc:3456: if (context ==
Expression::kValueAccumulator) {
On 2010/09/15 09:04:08, Kevin Millikin wrote:
> It seems legitimate to change the calls to VisitForTypeofValue to have an
> explicit ExpressionContext pushed instead of passing a token as argument:
> 
> { StackValueContext ctxt(this);
>   VisitForTypeofValue(expr->expression());
> }
> 
> Then here in this case you can have a "tail call" that forwards to Visit in
the
> current context with just Visit(expr).

Done.

[Erik Corry, 2010-09-20 14:28:26]

New version uploaded.  Changes from last time:

* The enums in ast.h are gone since they are no longer used.
* Move(reg, reg) introduced in ia32 and x64
* Virtual methods from some contexts moved to arch independent code.
* Ported to ARM and x64.
* Handling of && and || now uses virtual dispatch instead of switches.

[Kevin Millikin (Google), 2010-09-21 08:19:28]

This looks really good.  A have some small comments you should address, which
mostly apply to all three platforms.

http://codereview.chromium.org/3449004/diff/10001/11001
File src/arm/full-codegen-arm.cc (right):

http://codereview.chromium.org/3449004/diff/10001/11001#newcode257
src/arm/full-codegen-arm.cc:257: // Nothing to do.
It's not obvious why some of these are in the platform-specific and some in the
platform-independent implementation file.

My personal preference would be to have them all in the platform-specific files.
 If not, then all the platform-independent implementations should move to the
platform-independent file.

http://codereview.chromium.org/3449004/diff/10001/11001#newcode280
src/arm/full-codegen-arm.cc:280: }
// Nothing to do.

here or else consistently leave them out.

http://codereview.chromium.org/3449004/diff/10001/11001#newcode398
src/arm/full-codegen-arm.cc:398: Label* materialize_false) const {
I think the intent is that materialize_true == true_label_ && materialize_false
== false_label_.  Can we assert that?

http://codereview.chromium.org/3449004/diff/10001/11001#newcode1629
src/arm/full-codegen-arm.cc:1629: context()->Plug(r0);
I'd probably move the two calls to context()->Plug(r0) out if the if...else so
it's obvious they're in tail position where they belong.

http://codereview.chromium.org/3449004/diff/10001/11001#newcode2976
src/arm/full-codegen-arm.cc:2976: {
The other times this pattern appeared you used

{ EffectContext context(this);
  ...

http://codereview.chromium.org/3449004/diff/10001/11001#newcode3024
src/arm/full-codegen-arm.cc:3024: ASSERT(!context()->IsEffect());
I'm not sure which is more brittle to catch future changes: blacklisting effect
and test, or whitelisting the two value contexts.  I'd probably prefer the
latter.

http://codereview.chromium.org/3449004/diff/10001/11003
File src/full-codegen.cc (right):

http://codereview.chromium.org/3449004/diff/10001/11003#newcode691
src/full-codegen.cc:691: // Set up the appropriate context for the left
subexpression based
I think this comment is confusing now.  Delete it?

http://codereview.chromium.org/3449004/diff/10001/11003#newcode711
src/full-codegen.cc:711: codegen()->Visit(expr->left());
Probably a comment here that we want the value in the accumulator for the test
and on the stack in case we need it.

http://codereview.chromium.org/3449004/diff/10001/11003#newcode732
src/full-codegen.cc:732: codegen()->VisitForAccumulatorValue(expr->left());
Same comment about why we need two copies of the value.

http://codereview.chromium.org/3449004/diff/10001/11004
File src/full-codegen.h (right):

http://codereview.chromium.org/3449004/diff/10001/11004#newcode492
src/full-codegen.h:492: // Emit code to convert pure control flow to a pair of
labels into the
Probably this should say "pair of unbound labels" because the implementation may
decide to bind either of them.

http://codereview.chromium.org/3449004/diff/10001/11004#newcode497
src/full-codegen.h:497: virtual void Plug(Slot* slot) const = 0;
These four functions should move up to right under Plug(Register) so they can
share the comment there.

http://codereview.chromium.org/3449004/diff/10001/11004#newcode512
src/full-codegen.h:512: // Set up branch labels for a test expression.
Maybe the comment should say if_true, if_false, and fall_through are output
parameters?

http://codereview.chromium.org/3449004/diff/10001/11004#newcode553
src/full-codegen.h:553: virtual void PrepareTest(Label* t,
Might as well spell the parameters out.

[Erik Corry, 2010-09-23 10:09:14]

http://codereview.chromium.org/3449004/diff/10001/11001
File src/arm/full-codegen-arm.cc (right):

http://codereview.chromium.org/3449004/diff/10001/11001#newcode257
src/arm/full-codegen-arm.cc:257: // Nothing to do.
On 2010/09/21 08:19:28, kmillikin wrote:
> It's not obvious why some of these are in the platform-specific and some in
the
> platform-independent implementation file.
> 
> My personal preference would be to have them all in the platform-specific
files.
>  If not, then all the platform-independent implementations should move to the
> platform-independent file.

They are now grouped so that the 4 methods for each context class are always
together.  This means more code duplication, but hopefully more clarity.

http://codereview.chromium.org/3449004/diff/10001/11001#newcode280
src/arm/full-codegen-arm.cc:280: }
On 2010/09/21 08:19:28, kmillikin wrote:
> // Nothing to do.
> 
> here or else consistently leave them out.

I'm going to leave them out.  It's more obvious in a function than it was in the
swich statements.

http://codereview.chromium.org/3449004/diff/10001/11001#newcode398
src/arm/full-codegen-arm.cc:398: Label* materialize_false) const {
On 2010/09/21 08:19:28, kmillikin wrote:
> I think the intent is that materialize_true == true_label_ &&
materialize_false
> == false_label_.  Can we assert that?

Done.

http://codereview.chromium.org/3449004/diff/10001/11001#newcode1629
src/arm/full-codegen-arm.cc:1629: context()->Plug(r0);
On 2010/09/21 08:19:28, kmillikin wrote:
> I'd probably move the two calls to context()->Plug(r0) out if the if...else so
> it's obvious they're in tail position where they belong.

Done.

http://codereview.chromium.org/3449004/diff/10001/11001#newcode2976
src/arm/full-codegen-arm.cc:2976: {
On 2010/09/21 08:19:28, kmillikin wrote:
> The other times this pattern appeared you used
> 
> { EffectContext context(this);
>   ...

Done.

http://codereview.chromium.org/3449004/diff/10001/11001#newcode3024
src/arm/full-codegen-arm.cc:3024: ASSERT(!context()->IsEffect());
On 2010/09/21 08:19:28, kmillikin wrote:
> I'm not sure which is more brittle to catch future changes: blacklisting
effect
> and test, or whitelisting the two value contexts.  I'd probably prefer the
> latter.

This is pretty marginal, but I'm going to leave it as it was, simply because
there isn't an Is... method for the other two states.

http://codereview.chromium.org/3449004/diff/10001/11003
File src/full-codegen.cc (right):

http://codereview.chromium.org/3449004/diff/10001/11003#newcode691
src/full-codegen.cc:691: // Set up the appropriate context for the left
subexpression based
On 2010/09/21 08:19:28, kmillikin wrote:
> I think this comment is confusing now.  Delete it?

Done.

http://codereview.chromium.org/3449004/diff/10001/11003#newcode711
src/full-codegen.cc:711: codegen()->Visit(expr->left());
On 2010/09/21 08:19:28, kmillikin wrote:
> Probably a comment here that we want the value in the accumulator for the test
> and on the stack in case we need it.

Done.

http://codereview.chromium.org/3449004/diff/10001/11003#newcode732
src/full-codegen.cc:732: codegen()->VisitForAccumulatorValue(expr->left());
On 2010/09/21 08:19:28, kmillikin wrote:
> Same comment about why we need two copies of the value.

Done.

http://codereview.chromium.org/3449004/diff/10001/11004
File src/full-codegen.h (right):

http://codereview.chromium.org/3449004/diff/10001/11004#newcode497
src/full-codegen.h:497: virtual void Plug(Slot* slot) const = 0;
On 2010/09/21 08:19:28, kmillikin wrote:
> These four functions should move up to right under Plug(Register) so they can
> share the comment there.

Done.

http://codereview.chromium.org/3449004/diff/10001/11004#newcode512
src/full-codegen.h:512: // Set up branch labels for a test expression.
On 2010/09/21 08:19:28, kmillikin wrote:
> Maybe the comment should say if_true, if_false, and fall_through are output
> parameters?

Done.

http://codereview.chromium.org/3449004/diff/10001/11004#newcode553
src/full-codegen.h:553: virtual void PrepareTest(Label* t,
On 2010/09/21 08:19:28, kmillikin wrote:
> Might as well spell the parameters out.

Done.