Encapsulating Type information in the CompareICStub

src/code-stubs.cc

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 389 matching lines @@
     case CompareIC::KNOWN_OBJECT:
       ASSERT(*known_map_ != NULL);
       GenerateKnownObjects(masm);
       break;
     case CompareIC::GENERIC:
       GenerateGeneric(masm);
       break;
   }
 }
 
-
-CompareNilICStub::Types CompareNilICStub::GetPatchedICFlags(
-    Code::ExtraICState extra_ic_state,
-    Handle<Object> object,
-    bool* already_monomorphic) {
-  Types types = TypesField::decode(extra_ic_state);
-  NilValue nil = NilValueField::decode(extra_ic_state);
-  EqualityKind kind = EqualityKindField::decode(extra_ic_state);
-  ASSERT(types != CompareNilICStub::kFullCompare);
-  *already_monomorphic =
-      (types & CompareNilICStub::kCompareAgainstMonomorphicMap) != 0;
-  if (kind == kStrictEquality) {
-    if (nil == kNullValue) {
-      return CompareNilICStub::kCompareAgainstNull;
+void CompareNilICStub::Record(Handle<Object> object) {
+  ASSERT(!types_.IsFullCompare());
+  if (equality_kind_ == kStrictEquality) {
+    if (nil_value_ == kNullValue) {

[Sven Panne, 2013/05/14 09:18:42]

Use a ternary. More importantly: Why do we *set* types_ here and *Add* below? It
is unclear to me why we have to handle this differently. If it is correct,
please add a comment. (Yes, it has benn there  before in a different form, but
nevertheless...)

+      types_.SetTo(NULL_TYPE);
     } else {
-      return CompareNilICStub::kCompareAgainstUndefined;
+      types_.SetTo(UNDEFINED);
     }
   } else {
     if (object->IsNull()) {
-      types = static_cast<CompareNilICStub::Types>(
-          types | CompareNilICStub::kCompareAgainstNull);
+      types_.Add(NULL_TYPE);
     } else if (object->IsUndefined()) {
-      types = static_cast<CompareNilICStub::Types>(
-          types | CompareNilICStub::kCompareAgainstUndefined);
+      types_.Add(UNDEFINED);
     } else if (object->IsUndetectableObject() ||
                object->IsOddball() ||
                !object->IsHeapObject()) {
-        types = CompareNilICStub::kFullCompare;
-    } else if ((types & CompareNilICStub::kCompareAgainstMonomorphicMap) != 0) {
-      types = CompareNilICStub::kFullCompare;
+      types_ = Types::FullCompare();
+    } else if (IsMonomorphic()) {
+      types_ = Types::FullCompare();
     } else {
-      types = static_cast<CompareNilICStub::Types>(
-          types | CompareNilICStub::kCompareAgainstMonomorphicMap);
+      types_.Add(MONOMORPHIC_MAP);
     }
   }
-  return types;
 }
 
+void CompareNilICStub::Types::SetTo(Type type) {
+  RemoveAll();
+  Add(type);
+}
+
+void CompareNilICStub::PrintName(StringStream* stream) {
+  stream->Add("CompareNilICStub_");
+  types_.Print(stream);
+  if (nil_value_ == kNullValue) {

[Sven Panne, 2013/05/14 09:18:42]

Use a ternary ?: here...

+    stream->Add("(NullValue|");
+  } else {
+    stream->Add("(UndefinedValue|");
+  }
+  if (equality_kind_ == kStrictEquality) {

[Sven Panne, 2013/05/14 09:18:42]

... and here, and merge the stream->Add() calls. More concise and easier to
read.

+    stream->Add("StrictEquality)");
+  } else {
+    stream->Add("NonStrictEquality)");
+  }
+}
+
+void CompareNilICStub::Types::Print(StringStream* stream) const {
+  stream->Add("(");
+  if (IsEmpty()) stream->Add("None,");
+  if (Contains(UNDEFINED)) stream->Add("Undefined,");
+  if (Contains(NULL_TYPE)) stream->Add("Null,");
+  if (Contains(MONOMORPHIC_MAP)) stream->Add("MonomorphicMap,");
+  if (Contains(UNDETECTABLE)) stream->Add("Undetectable,");
+  stream->Remove(1);

[Sven Panne, 2013/05/14 09:18:42]

Although it looks like a cunning idea at first, removing things from a stream is
a bad idea conceptually. I want to refactor things to clearly distinguish
between the conversion of an object to a sequence of characters and the
character sink itself. This kind of hack would be counter-productive. Just keep
track of the fact if you need to emit a separator by hand. C++ sucks:
http://hackage.haskell.org/packages/archive/base/latest/doc/html/Data-List.ht...
:-/

"What happens in a stream, stays in a stream!". ;-)

[oliv, 2013/05/14 13:28:11]

ok, ic
i know:
http://ruby-doc.org/core-2.0/Array.html#method-i-join
;)

+  stream->Add(")");
+}
 
 void InstanceofStub::PrintName(StringStream* stream) {
   const char* args = "";
   if (HasArgsInRegisters()) {
     args = "_REGS";
   }
 
   const char* inline_check = "";
   if (HasCallSiteInlineCheck()) {
     inline_check = "_INLINE";
@@ skipping 87 matching lines @@
 }
 
 
 void ToBooleanStub::PrintName(StringStream* stream) {
   stream->Add("ToBooleanStub_");
   types_.Print(stream);
 }
 
 
 void ToBooleanStub::Types::Print(StringStream* stream) const {
-  if (IsEmpty()) stream->Add("None");
-  if (Contains(UNDEFINED)) stream->Add("Undefined");
-  if (Contains(BOOLEAN)) stream->Add("Bool");
-  if (Contains(NULL_TYPE)) stream->Add("Null");
-  if (Contains(SMI)) stream->Add("Smi");
-  if (Contains(SPEC_OBJECT)) stream->Add("SpecObject");
-  if (Contains(STRING)) stream->Add("String");
-  if (Contains(SYMBOL)) stream->Add("Symbol");
-  if (Contains(HEAP_NUMBER)) stream->Add("HeapNumber");
+  stream->Add("(");
+  if (IsEmpty()) stream->Add("None,");
+  if (Contains(UNDEFINED)) stream->Add("Undefined,");
+  if (Contains(BOOLEAN)) stream->Add("Bool,");
+  if (Contains(NULL_TYPE)) stream->Add("Null,");
+  if (Contains(SMI)) stream->Add("Smi,");
+  if (Contains(SPEC_OBJECT)) stream->Add("SpecObject,");
+  if (Contains(STRING)) stream->Add("String,");
+  if (Contains(SYMBOL)) stream->Add("Symbol,");
+  if (Contains(HEAP_NUMBER)) stream->Add("HeapNumber,");
+  stream->Remove(1);

[Sven Panne, 2013/05/14 09:18:42]

Same comment as in the other Print() function.

+  stream->Add(")");
 }
 
 
 void ToBooleanStub::Types::TraceTransition(Types to) const {
   if (!FLAG_trace_ic) return;
   char buffer[100];
   NoAllocationStringAllocator allocator(buffer,
                                         static_cast<unsigned>(sizeof(buffer)));
   StringStream stream(&allocator);
   stream.Add("[ToBooleanIC (");
@@ skipping 157 matching lines @@
   } else if (argument_count >= 2) {
     argument_count_ = MORE_THAN_ONE;
   } else {
     UNREACHABLE();
   }
   ArrayConstructorStubBase::GenerateStubsAheadOfTime(isolate);
 }
 
 
 } }  // namespace v8::internal