Comments addressed.
GenerateKnownObjects being static was a leftover from just creating the
code and
returning it without using the cache, changed to non-static and simplified
by
using the normal GenerateMiss.
http://codereview.chromium.org/8520006/diff/17001/src/code-stubs.cc
File src/code-stubs.cc (right):
http://codereview.chromium.org/8520006/diff/17001/src/code-stubs.cc#newcode105
src/code-stubs.cc:105: if (use_special_cache || !FindCodeInCache(&code))
{
On 2011/11/10 19:08:42, Kevin Millikin wrote:
I think this is a little clearer if it is something like:
if (use_special_cache) {
if (FindCodeInSpecialCache(&code)) return Handle<Code>(code);
} else {
if (FindCodeInCache(&code)) return Handle<Code>(code);
}
or if you wanted to be a bit twisted:
if (use_special_cache
? FindCodeInSpecialCache(&code)
: FindCodeInCache(&code)) {
return Handle<Code>(code);
}
Done and moved the pregenerated check up here
http://codereview.chromium.org/8520006/diff/17001/src/code-stubs.cc#newcode176
src/code-stubs.cc:176: Isolate* isolate = Isolate::Current();
On 2011/11/10 19:08:42, Kevin Millikin wrote:
If you want to avoid TLS to fetch the isolate, you could pass in the
one you
have from GetCode or else use new_object->isolate().
Done.
http://codereview.chromium.org/8520006/diff/17001/src/code-stubs.h
File src/code-stubs.h (right):
http://codereview.chromium.org/8520006/diff/17001/src/code-stubs.h#newcode447
src/code-stubs.h:447: static void GenerateKnownObjects(MacroAssembler*
masm, Map* map);
On 2011/11/10 19:08:42, Kevin Millikin wrote:
Put this down in the private section with the other Generate...
functions. Make
it non-static. Do not pass map (== known_map_) as an argument, but
instead use
known_map_ in the generate function.
Done.
http://codereview.chromium.org/8520006/diff/17001/src/code-stubs.h#newcode449
src/code-stubs.h:449: void set_known_map(Map* map) { known_map_ = map; }
On 2011/11/10 19:08:42, Kevin Millikin wrote:
Make map a Handle<Map>. There's a risk that a GC occurs between
set_known_map(map) and Generate().
OK, will do, but maps don't move right, and we already have handles to
the objects with the maps, so I think this should be safe even without
handles
http://codereview.chromium.org/8520006/diff/17001/src/code-stubs.h#newcode476
src/code-stubs.h:476: Map* known_map_;
On 2011/11/10 19:08:42, Kevin Millikin wrote:
Handle<Map>.
Done.
http://codereview.chromium.org/8520006/diff/17001/src/hydrogen.cc
File src/hydrogen.cc (right):
http://codereview.chromium.org/8520006/diff/17001/src/hydrogen.cc#newcode6000
src/hydrogen.cc:6000: case Token::EQ_STRICT: {
On 2011/11/10 19:08:42, Kevin Millikin wrote:
I guess if EQ_STRICT, then we'll never have a compare map (because we
don't go
to KNOWN_OBJECTS in that case). I wonder if we should?
Of course we should, nice catch.
http://codereview.chromium.org/8520006/diff/17001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):
http://codereview.chromium.org/8520006/diff/17001/src/ia32/code-stubs-ia32.cc#newcode6521
src/ia32/code-stubs-ia32.cc:6521: __ bind(&miss);
On 2011/11/10 19:08:42, Kevin Millikin wrote:
All the code from here and below looks like it could be generated by a
call to
GenerateMiss(masm) (if this function were non-static).
Done.
http://codereview.chromium.org/8520006/diff/17001/src/ia32/ic-ia32.cc
File src/ia32/ic-ia32.cc (right):
http://codereview.chromium.org/8520006/diff/17001/src/ia32/ic-ia32.cc#newcode1630
src/ia32/ic-ia32.cc:1630: rewritten = stub.GetCode(true);
On 2011/11/10 19:08:42, Kevin Millikin wrote:
Nicer to have a virtual "UseSpecialCache" function you can override
for just
this stub, instead of the parameter (with a default, and a boolean).
Done.
http://codereview.chromium.org/8520006/diff/17001/src/ic.cc
File src/ic.cc (right):
http://codereview.chromium.org/8520006/diff/17001/src/ic.cc#newcode2336
src/ic.cc:2336: if (state == UNINITIALIZED && x->IsSmi() && y->IsSmi())
{
On 2011/11/10 19:08:42, Kevin Millikin wrote:
I don't like this function. It's really hard to figure out what
transitions
will happen.
It seems to be programmed after listing the possible output types with
some
exceptions up front, but that's not the way that most people think
about state
transitions. What about rewriting it to something more structured
like:
switch (state) {
case UNINITIALIZED:
if (x->IsSmi() && y->IsSmi()) return SMIS;
if (x->IsNumber() && y->IsNumber()) return HEAP_NUMBERS;
if (!Token::IsEqualityOp(op_)) return GENERIC;
if (x->IsSymbol() && y->IsSymbol()) return SYMBOLS;
if (x->IsString() && y->IsString()) return STRINGS;
if (x->IsJSObject() && y->IsJSObject()) {
// Your new stuff here.
}
return GENERIC;
case SMIS:
return has_inlined_smi_code && x->IsNumber() && y->IsNumber()
? HEAP_NUMBERS
: GENERIC;
case SYMBOLS:
ASSERT(Token::IsEqualityOp(op_));
return x->IsString() && y->IsString() ? STRINGS : GENERIC;
case HEAP_NUMBERS:
case STRINGS:
case OBJECTS:
case KNOWN_OBJECTS:
case GENERIC:
return GENERIC;
}
Done.
http://codereview.chromium.org/8520006/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
