This patch much improves our tracking of whether function is...

This patch much improves our tracking of whether function is
called from within a loop or not.  In the past we lost the
information if a call site went megamorphic before a lazily
compiled callee was called for the first time.  Now we track
that correctly (this is an issue that affects richards).
We still don't manage to track the in-loop state through a
constructor call, since constructor calls use LoadICs instead
of CallICs.  This issue affects delta-blue.  So in this patch
we assume that lazy compilations that don't happen through a
CallIC happen from inside a loop.  I have an idea to fix this
but this patch is big enough already.
With our improved tracking of in-loop state I have switched
off the inlining of in-object loads for code that is not in
a loop.  This benefits compile speed.  One issue is that
eagerly compiled code now doesn't get the in-object loads
inlined.  We need to eagerly compile less code to fix this.

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

[Kasper Lund, 2009-05-25 09:45:42]

LGTM, but maybe Kevin should take a look too?

http://codereview.chromium.org/115744/diff/1/17
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/115744/diff/1/17#newcode66
Line 66: Operand(~(Code::kFlagsTypeMask | Code::kFlagsICInLoopMask)));
Why not have a constant mask for the combination of the two masks?

http://codereview.chromium.org/115744/diff/1/9
File src/ic.h (right):

http://codereview.chromium.org/115744/diff/1/9#newcode124
Line 124: const char* updated_from = "");
updated_from -> extra_info?

[Kevin Millikin (Chromium), 2009-05-25 11:00:42]

LGTM with some comments.

http://codereview.chromium.org/115744/diff/1/19
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/115744/diff/1/19#newcode1060
Line 1060: CallFunctionStub(int argc, InlineCacheInLoop in_loop)
I like a name that indicates flaggishness.  InLoopFlag?

http://codereview.chromium.org/115744/diff/1/18
File src/arm/ic-arm.cc (right):

http://codereview.chromium.org/115744/diff/1/18#newcode425
Line 425: Code::Flags flags = Code::ComputeFlags(Code::LOAD_IC, NOT_IN_LOOP,
MONOMORPHIC);
Long line?

http://codereview.chromium.org/115744/diff/1/18#newcode758
Line 758: Code::Flags flags = Code::ComputeFlags(Code::STORE_IC, NOT_IN_LOOP,
MONOMORPHIC);
Long line?

http://codereview.chromium.org/115744/diff/1/17
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/115744/diff/1/17#newcode500
Line 500: Code::Flags flags) {
Since you've generalized the call site to take flags, you might want to assert
here that the passed flags has type FIELD.

http://codereview.chromium.org/115744/diff/1/17#newcode552
Line 552: Code::Flags flags) {
Same as above with type CONSTANT_FUNCTION.

http://codereview.chromium.org/115744/diff/1/13
File src/codegen.cc (right):

http://codereview.chromium.org/115744/diff/1/13#newcode330
Line 330: if (in_loop) {
My opinion is mixed about making these two value, on/off flags  into enums (for
readability at the call site).

But since you didn't make it a bool, please write:

if (in_loop == IN_LOOP) ...

http://codereview.chromium.org/115744/diff/1/5
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/115744/diff/1/5#newcode474
Line 474: Code::Flags flags) {
Same comment about ASSERT as in the ARM file.

http://codereview.chromium.org/115744/diff/1/5#newcode523
Line 523: Code::Flags flags) {
And here.

http://codereview.chromium.org/115744/diff/1/8
File src/ic.cc (right):

http://codereview.chromium.org/115744/diff/1/8#newcode451
Line 451: state == PREMONOMORPHIC || state == MONOMORPHIC ||
One disjunct per line?

http://codereview.chromium.org/115744/diff/1/8#newcode1111
Line 1111: if (in_loop) {
if (in_loop == IN_LOOP) ....

http://codereview.chromium.org/115744/diff/1/16
File src/objects-inl.h (right):

http://codereview.chromium.org/115744/diff/1/16#newcode1957
Line 1957: if (in_loop) bits |= kFlagsICInLoopMask;
if (in_loop == IN_LOOP) ...