Track whether a node or variable are likely to be a Smi value. Propagate that...

Track whether a node or variable are likely to be a Smi value. Propagate that
knowledge in the AST and inline the Smi check into the generated code if it
is deemed high value (e.g. in loops).


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

[iposva, 2008-10-28 15:18:46]

Opening up for a first set of comments.
I know already that:
- the loop_nesting_ in the CodeGenerator needs accessors
- ARM code generation needs to be updated

Thanks,
-Ivan

[Kasper Lund, 2008-10-28 15:25:38]

LGTM. Seems easy to extend and understand.

http://codereview.chromium.org/8835/diff/1/12
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/8835/diff/1/12#newcode817
Line 817: flags = ((loop_nesting_ > 0) && type->IsLikelySmi()) ?
SMI_CODE_INLINED :
Nit: I would move ? onto a new line and : onto yet another new line, like

x = <complicated boolean expression>
    ? <blah>
    : <argh>;

http://codereview.chromium.org/8835/diff/1/5
File src/variables.h (right):

http://codereview.chromium.org/8835/diff/1/5#newcode83
Line 83: static char* Type2String(StaticType* type) {
I would move this to a .cc file.

http://codereview.chromium.org/8835/diff/1/5#newcode95
Line 95: // LIKELY_SMI accessors
How about MarkAsLikelySmi? Set sounds like something that takes an argument...

[iposva, 2008-10-28 18:30:32]

Thanks for the feedback. Applied the changes, made it lint and now I am running
more tests...

-Ivan

http://codereview.chromium.org/8835/diff/1/12
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/8835/diff/1/12#newcode817
Line 817: flags = ((loop_nesting_ > 0) && type->IsLikelySmi()) ?
SMI_CODE_INLINED :
Done.

On 2008/10/28 15:25:39, Kasper Lund wrote:
> Nit: I would move ? onto a new line and : onto yet another new line, like
> 
> x = <complicated boolean expression>
>     ? <blah>
>     : <argh>;

http://codereview.chromium.org/8835/diff/1/5
File src/variables.h (right):

http://codereview.chromium.org/8835/diff/1/5#newcode83
Line 83: static char* Type2String(StaticType* type) {
Done.

On 2008/10/28 15:25:39, Kasper Lund wrote:
> I would move this to a .cc file.

http://codereview.chromium.org/8835/diff/1/5#newcode95
Line 95: // LIKELY_SMI accessors
Changed to SetAsLikelySmi which to me implies that it is a setter while not
necessarily expecting an argument (the argument is really a part of the function
name).

On 2008/10/28 15:25:39, Kasper Lund wrote:
> How about MarkAsLikelySmi? Set sounds like something that takes an argument...

[Kevin Millikin (Chromium), 2008-10-28 19:10:41]

Overall it looks like a good start.

The flow of values down and up (and back down) the AST is subtle and needs some
overview comments.

Revisiting already analyzed nodes looks like it could be quadratic in the size
of the expression.

Minor comments below.

http://codereview.chromium.org/8835/diff/229/241
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/8835/diff/229/241#newcode814
Line 814: // generated the inline Smi check code if the operation is part of a
loop.
generate

http://codereview.chromium.org/8835/diff/229/239
File src/rewriter.cc (right):

http://codereview.chromium.org/8835/diff/229/239#newcode58
Line 58: void AstOptimizer::Optimize(ZoneList<Statement*>* statements) {
The Visitor base class should have VisitStatements and VisitExpressions that do
just this by default.

http://codereview.chromium.org/8835/diff/229/239#newcode95
Line 95: if (node->init()) {
I prefer explicit "!= NULL" here.  It seems to fit the rest of our code base.

http://codereview.chromium.org/8835/diff/229/233
File src/variables.h (right):

http://codereview.chromium.org/8835/diff/229/233#newcode86
Line 86: static char* Type2String(StaticType* type);
ToString is a better name.

[iposva, 2008-10-28 22:29:51]

http://codereview.chromium.org/8835/diff/229/239
File src/rewriter.cc (right):

http://codereview.chromium.org/8835/diff/229/239#newcode58
Line 58: void AstOptimizer::Optimize(ZoneList<Statement*>* statements) {
Agreed, but not in this change list.

On 2008/10/28 19:10:41, Kevin Millikin wrote:
> The Visitor base class should have VisitStatements and VisitExpressions that
do
> just this by default.

http://codereview.chromium.org/8835/diff/229/239#newcode95
Line 95: if (node->init()) {
Agreed and fixed. I was just mimicking other style from similar methods.

On 2008/10/28 19:10:41, Kevin Millikin wrote:
> I prefer explicit "!= NULL" here.  It seems to fit the rest of our code base.

http://codereview.chromium.org/8835/diff/229/233
File src/variables.h (right):

http://codereview.chromium.org/8835/diff/229/233#newcode86
Line 86: static char* Type2String(StaticType* type);
Again, I agree with your general suggestion, but I will leave this alone until
we change the other code that is in this style such as Variable::Mode2String().
Preferably we could use type()->ToString() instead of
StaticType::Type2String(type()).

On 2008/10/28 19:10:41, Kevin Millikin wrote:
> ToString is a better name.