* String type inference using compiler framework.

Tracks static type of strings in frame elements and results.
Uses static string type to optimize string additions.

[Lasse Reichstein, 2009-03-31 12:31:38]

Not-as-big-as-seems code review, please.
many of the changes comes from renaming the existing class StaticType to
SmiAnalysis, before introducing a new one by the same name.

Please check if there are any way that I have missed that can cause a result or
frame element to have the "String" static type when the value is not a string
(I'm a little wary about copying).

[William Hesse, 2009-03-31 13:19:59]

I don't think this should be a byte, rather than a word, unless you know how you
plan to embed it in a word with some other shared info.

http://codereview.chromium.org/57052/diff/40/49
File src/register-allocator.h (right):

http://codereview.chromium.org/57052/diff/40/49#newcode100
Line 100: STRING_TYPE = 12
Should you represent these values in hex: 0xC?
Should these values be aligned, or have end-of-line comments
instead?

http://codereview.chromium.org/57052/diff/40/49#newcode105
Line 105: byte static_type_;
We haven't really been using bytes in structures except as definite parts of
words.  Do we know what this does to alignment of everything else?

http://codereview.chromium.org/57052/diff/40/49#newcode196
Line 196: 
If this ever did save memory, it would do so by totally messing up the alignment
of everything following it.  I'm suspicious.

[Kevin Millikin (Chromium), 2009-03-31 14:00:03]

Random superficial comments below.  I'll take a closer look tonight or in the
morning.

http://codereview.chromium.org/57052/diff/40/44
File src/codegen-ia32.h (right):

http://codereview.chromium.org/57052/diff/40/44#newcode435
Line 435: void GenericBinaryOperation(Token::Value op,
While you're at it, fix this indentation so Token::Value op is on the next line
and aligned with the other arguments.

http://codereview.chromium.org/57052/diff/40/49
File src/register-allocator.h (right):

http://codereview.chromium.org/57052/diff/40/49#newcode47
Line 47: inline StaticType() : static_type_(UNKNOWN_TYPE) {}
We don't usually have such dense blocks of declarations in classes in the V8
code.  You might add some blank lines between blocks to break them up.

I think it's better (easier to read) to leave inline off when the definition is
in the class.

http://codereview.chromium.org/57052/diff/40/49#newcode56
Line 56: bool is_known() const { return static_type_ != UNKNOWN_TYPE; }
Do we need is_known and (two copies of) is_unknown?  I'd normally prefer just
is_known (which can be negated for is_unknown), but since "unknown" is actually
an element of the order, it might make sense to have only is_unknown.

http://codereview.chromium.org/57052/diff/40/49#newcode58
Line 58: bool is_likely_smi() const { return static_type_ & LIKELY_SMI_TYPE !=
0; }
Isn't this (static_type_ & LIKELY_SMI_TYPE) != 0?

http://codereview.chromium.org/57052/diff/40/49#newcode62
Line 62: return static_type_ & HEAP_OBJECT_TYPE != 0;
Isn't this (static_type_ & HEAP_OBJECT_TYPE) != 0?

[Lasse Reichstein, 2009-03-31 16:41:49]

http://codereview.chromium.org/57052/diff/40/49
File src/register-allocator.h (right):

http://codereview.chromium.org/57052/diff/40/49#newcode56
Line 56: bool is_known() const { return static_type_ != UNKNOWN_TYPE; }
No, we don't. It's a leftover from when I tried merging SmiAnalysis and
StaticType. The only one that is actually use is is_string().

http://codereview.chromium.org/57052/diff/40/49#newcode58
Line 58: bool is_likely_smi() const { return static_type_ & LIKELY_SMI_TYPE !=
0; }
Yes, and it won't compile on Windows without the parentheses, so they will be
added.

http://codereview.chromium.org/57052/diff/40/49#newcode62
Line 62: return static_type_ & HEAP_OBJECT_TYPE != 0;
Yes, too.

http://codereview.chromium.org/57052/diff/40/49#newcode100
Line 100: STRING_TYPE = 12
The comments should be removed completely, they are no longer accurate.
Alignment ... hmm, I'll try it out and see if it improves readability.

http://codereview.chromium.org/57052/diff/40/49#newcode105
Line 105: byte static_type_;
We know that the compiler will make a layout that has room for everything. I
decided to trust it :)
A byte should be sufficient here (and sizeof(StaticType) is actually 1), and
should make it easier to add it to other structures without increasing their
size.

http://codereview.chromium.org/57052/diff/40/49#newcode196
Line 196: 
I *expect* static_type and type_ to be byte aligned, and the following union to
be word aligned, i.e., there is two bytes of padding between them.
Not reducing the size of type_ would make objects require three words (to be
word aligned) with three bytes of padding. 

There is nothing in the C++ spec preventing the compiler from making objects
take up six bytes and be byte aligned, if the platform supports it. With
optimization enabled, I doubt it will happen, but we should check it.

[Lasse Reichstein, 2009-04-03 08:32:45]

Re-review, please.
In particular the frame merging code has changed.

http://codereview.chromium.org/57052/diff/2001/3010
File src/register-allocator.h (right):

http://codereview.chromium.org/57052/diff/2001/3010#newcode45
Line 45: class StaticType  {
Oops, removed the BASE_EMBEDDED while editing (it confuzes my editor). Will add
it back in.

[Kevin Millikin (Chromium), 2009-04-03 09:49:22]

Does it need any changes to make ARM continue to work?

http://codereview.chromium.org/57052/diff/40/49
File src/register-allocator.h (right):

http://codereview.chromium.org/57052/diff/40/49#newcode164
Line 164: StaticType static_type() { return static_type_; }
const, since the other getters are.

http://codereview.chromium.org/57052/diff/2001/3003
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/57052/diff/2001/3003#newcode846
Line 846: bool left_is_string = left.static_type().is_jsstring();
This is a separate change for one of us to make: we really need to be able to
inspect the types of frame elements without popping them.  In this case (as well
as the non-smi case below) we pop then push, which may emit code.

http://codereview.chromium.org/57052/diff/2001/3004
File src/codegen-ia32.h (right):

http://codereview.chromium.org/57052/diff/2001/3004#newcode436
Line 436: SmiAnalysis* type,
Could you line up all the arguments here (which means starting the first one on
the next line).

http://codereview.chromium.org/57052/diff/2001/3005
File src/jump-target.cc (right):

http://codereview.chromium.org/57052/diff/2001/3005#newcode286
Line 286: current.set_static_type(StaticType::unknown());
entry_frame_->elements_[i].set_static_type(StaticType::unknown())

http://codereview.chromium.org/57052/diff/2001/3005#newcode295
Line 295: current.set_static_type(merged_type);
And similarly.

http://codereview.chromium.org/57052/diff/2001/3010
File src/register-allocator.h (right):

http://codereview.chromium.org/57052/diff/2001/3010#newcode58
Line 58: bool is_jsstring() { return static_type_ == STRING_TYPE; }
Is there a distinction between string and JS string?  I much prefer to read
is_string().

http://codereview.chromium.org/57052/diff/2001/3010#newcode63
Line 63: // if (object->IsSmi()) return smi();                 // Unused for
now.
Go ahead and put the commented code in?  In any case, this probably deserves a
comment reminding that the order of comparisons is important (most specific
first).

http://codereview.chromium.org/57052/diff/2001/3010#newcode89
Line 89: SMI_TYPE         = 0x01,
Remove likely smi from the ASCII art?

http://codereview.chromium.org/57052/diff/2001/3010#newcode91
Line 91: STRING_TYPE      = 0x04 | HEAP_OBJECT_TYPE
This is good.

http://codereview.chromium.org/57052/diff/2001/3010#newcode155
Line 155: StaticType static_type() { return static_type_; }
Go ahead and make this const like the other accessors.

http://codereview.chromium.org/57052/diff/2001/3016
File src/virtual-frame-ia32.cc (right):

http://codereview.chromium.org/57052/diff/2001/3016#newcode197
Line 197: // Adjust the stack point downward if necessary.
As long as you're fixing my comment, you could change "stack point" to "stack
pointer".

http://codereview.chromium.org/57052/diff/2001/3018
File src/virtual-frame.cc (right):

http://codereview.chromium.org/57052/diff/2001/3018#newcode389
Line 389: // register element, or the new element at frame_index, must be made
If we keep the merging of copied registers (see comment below) it should happen
here as well.

http://codereview.chromium.org/57052/diff/2001/3018#newcode446
Line 446: StaticType merge_type = static_type.merge(element.static_type());
This is a little weird.  A register on the frame and a register in an off-frame
result should be copies of each other.  Neither should be changed, without first
breaking the copy and moving one to another place.  A failure to have the same
static_type might indicate a bug somewhere else.

So the question is: is there something that can happen to a result to cause us
to lose information about its type without changing its value?

http://codereview.chromium.org/57052/diff/2001/3019
File src/virtual-frame.h (right):

http://codereview.chromium.org/57052/diff/2001/3019#newcode47
Line 47: class FrameElement  {
Does this still want BASE_EMBEDDED?

http://codereview.chromium.org/57052/diff/2001/3019#newcode56
Line 56: : static_type_(), type_(INVALID), copied_(false), synced_(false) {
Won't static_type_() happen without mentioning it?

http://codereview.chromium.org/57052/diff/2001/3019#newcode68
Line 68: FrameElement result(MEMORY, no_reg, true);
Err, the main point of having an enum for SYNCED/NOT_SYNCED is so that we don't
have to read trues and falses at the call sites and remember the sense of the
flag...

http://codereview.chromium.org/57052/diff/2001/3019#newcode76
Line 76: return FrameElement(REGISTER, reg, is_synced != NOT_SYNCED,
static_type);
... so could you move the conversion from enum to bool all the way down into the
private constructor?

http://codereview.chromium.org/57052/diff/2001/3019#newcode175
Line 175: inline FrameElement(Handle<Object> value, bool is_synced)
We don't typically write inline if the def'n is right in the header file.

[William Hesse, 2009-04-03 11:38:14]

LGTM.

[Lasse Reichstein, 2009-04-03 12:43:21]

http://codereview.chromium.org/57052/diff/2001/3005
File src/jump-target.cc (right):

http://codereview.chromium.org/57052/diff/2001/3005#newcode286
Line 286: current.set_static_type(StaticType::unknown());
Changed "current" to be a pointer to the element in the frame, instead of a
copy.

http://codereview.chromium.org/57052/diff/2001/3005#newcode295
Line 295: current.set_static_type(merged_type);
And ditto.

http://codereview.chromium.org/57052/diff/2001/3010
File src/register-allocator.h (right):

http://codereview.chromium.org/57052/diff/2001/3010#newcode58
Line 58: bool is_jsstring() { return static_type_ == STRING_TYPE; }
So do I, but then the static factory method should be called string(), and that
makes the lint check barf because I use "string" without including "string.h".
Maybe I should capitalize the factory methods.

http://codereview.chromium.org/57052/diff/2001/3010#newcode63
Line 63: // if (object->IsSmi()) return smi();                 // Unused for
now.
On 2009/04/03 09:49:22, Kevin Millikin wrote:
> Go ahead and put the commented code in?  In any case, this probably deserves
a
> comment reminding that the order of comparisons is important (most specific
> first).

Done.

http://codereview.chromium.org/57052/diff/2001/3018
File src/virtual-frame.cc (right):

http://codereview.chromium.org/57052/diff/2001/3018#newcode446
Line 446: StaticType merge_type = static_type.merge(element.static_type());
I'll assert that there can't. If it turns out we are wrong, we'll have to figure
out whether the bug is in the assert or in the compiler.

http://codereview.chromium.org/57052/diff/2001/3019
File src/virtual-frame.h (right):

http://codereview.chromium.org/57052/diff/2001/3019#newcode47
Line 47: class FrameElement  {
Indeed it does.

http://codereview.chromium.org/57052/diff/2001/3019#newcode76
Line 76: return FrameElement(REGISTER, reg, is_synced != NOT_SYNCED,
static_type);
On 2009/04/03 09:49:22, Kevin Millikin wrote:
> ... so could you move the conversion from enum to bool all the way down into
the
> private constructor?

Done.