Encapsulating Type information in the CompareICStub

src/code-stubs.h

 // 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 1016 matching lines @@
   Token::Value op_;
   CompareIC::State left_;
   CompareIC::State right_;
   CompareIC::State state_;
   Handle<Map> known_map_;
 };
 
 
 class CompareNilICStub : public HydrogenCodeStub  {
  public:
-  enum Types {
-    kCompareAgainstNull = 1 << 0,
-    kCompareAgainstUndefined = 1 << 1,
-    kCompareAgainstMonomorphicMap = 1 << 2,
-    kCompareAgainstUndetectable = 1 << 3,
-    kFullCompare = kCompareAgainstNull | kCompareAgainstUndefined |
-        kCompareAgainstUndetectable
+  enum Type {
+    UNDEFINED,
+    NULL_TYPE,
+    MONOMORPHIC_MAP,
+    UNDETECTABLE,
+    NUMBER_OF_TYPES
   };
 
+  class Types {

[Sven Panne, 2013/05/13 14:24:50]

Instead of inventing tons of ad-hoc helper functions, publicly derive from
EnumSet<Type,byte> and add just Print(StringStream*), perhaps FullCompare().
Nothing else is really needed.

[oliv, 2013/05/13 18:03:58]

Good point -> Done

+   public:
+    Types() : set_(0) { }
+    Types(const Types& state) : set_(state.set_) { }
+    explicit Types(byte bits) : set_(bits) { }
+
+    static Types FullCompare() {
+      Types full(full_compare_types().ToIntegral());
+      return full;
+    }
+
+    bool IsEmpty()           const { return set_.ToIntegral() == 0; }
+    bool IsMonomorphic()     const { return set_.Contains(MONOMORPHIC_MAP); }
+    bool Contains(Type type) const { return set_.Contains(type); }
+    bool IsFullCompare()     const {
+      return set_.ToIntegral() == full_compare_types().ToIntegral();
+    }
+
+    void Add(Type type);
+    void SetTo(Type type);
+    void BeFullCompare() { set_ = full_compare_types(); }
+    void Clear() { set_ = EnumSet<Type, byte>(0); }
+
+    byte ToByte() const { return set_.ToIntegral(); }
+
+    void Print(StringStream* stream) const;
+
+   private:
+    static const EnumSet<Type, byte> full_compare_types() {
+      EnumSet<Type, byte> set;
+      set.Add(UNDEFINED);
+      set.Add(NULL_TYPE);
+      set.Add(UNDETECTABLE);
+      return set;
+    }
+
+    EnumSet<Type, byte> set_;
+  };
+
+  // At most 6 different types can be distinguished, because the Code object
+  // only has room for a single byte to hold a set and there are two more
+  // boolean flags we need to store. :-P
+  STATIC_ASSERT(NUMBER_OF_TYPES <= 6);
+
   CompareNilICStub(EqualityKind kind, NilValue nil, Types types)
-      : HydrogenCodeStub(CODE_STUB_IS_NOT_MISS), bit_field_(0) {
-    bit_field_ = EqualityKindField::encode(kind) |
-        NilValueField::encode(nil) |
-        TypesField::encode(types);
+      : HydrogenCodeStub(CODE_STUB_IS_NOT_MISS), types_(types) {
+    equality_kind_ = kind;
+    nil_value_ = nil;
   }
 
-  virtual InlineCacheState GetICState() {
-    Types types = GetTypes();
-    if (types == kFullCompare) {
-      return MEGAMORPHIC;
-    } else if ((types & kCompareAgainstMonomorphicMap) != 0) {
-      return MONOMORPHIC;
-    } else {
-      return PREMONOMORPHIC;
-    }
+  explicit CompareNilICStub(Code::ExtraICState ic_state)
+      : HydrogenCodeStub(CODE_STUB_IS_NOT_MISS) {
+    equality_kind_ = EqualityKindField::decode(ic_state);
+    nil_value_ = NilValueField::decode(ic_state);
+    types_ = Types(ExtractTypesFromExtraICState(ic_state));
   }
 
-  virtual Code::Kind GetCodeKind() const { return Code::COMPARE_NIL_IC; }
-
-  Handle<Code> GenerateCode();
-
   static Handle<Code> GetUninitialized(Isolate* isolate,
                                        EqualityKind kind,
                                        NilValue nil) {
-    return CompareNilICStub(kind, nil).GetCode(isolate);
+    return CompareNilICStub(kind, nil, CODE_STUB_IS_MISS).GetCode(isolate);
   }
 
   virtual void InitializeInterfaceDescriptor(
       Isolate* isolate,
       CodeStubInterfaceDescriptor* descriptor);
 
   static void InitializeForIsolate(Isolate* isolate) {
-    CompareNilICStub compare_stub(kStrictEquality, kNullValue);
+    CompareNilICStub compare_stub(kStrictEquality, kNullValue,
+                                  CODE_STUB_IS_MISS);
     compare_stub.InitializeInterfaceDescriptor(
         isolate,
         isolate->code_stub_interface_descriptor(CodeStub::CompareNilIC));
   }
 
-  virtual Code::ExtraICState GetExtraICState() {
-    return bit_field_;
+  virtual InlineCacheState GetICState() {
+    if (types_.IsFullCompare()) {
+      return MEGAMORPHIC;
+    } else if (types_.Contains(MONOMORPHIC_MAP)) {
+      return MONOMORPHIC;
+    } else {
+      return PREMONOMORPHIC;
+    }
   }
 
-  EqualityKind GetKind() { return EqualityKindField::decode(bit_field_); }
-  NilValue GetNilValue() { return NilValueField::decode(bit_field_); }
-  Types GetTypes() { return TypesField::decode(bit_field_); }
+  virtual Code::Kind GetCodeKind() const { return Code::COMPARE_NIL_IC; }
 
-  static Types TypesFromExtraICState(
+  Handle<Code> GenerateCode();
+
+  // extra ic state = nil_value | equality_kind | type_n-1 | ... | type_0
+  virtual Code::ExtraICState GetExtraICState() {

[Sven Panne, 2013/05/13 14:24:50]

Use the BitField template instead of doing the bit fiddling by hand.

[oliv, 2013/05/13 18:03:58]

They are BitFields, except for the types, since those are just the lsb, so no
need to encode anything there.
Or maybe i don't fully get the bitField templates?

+    return NilValueField::encode(nil_value_)         |
+           EqualityKindField::encode(equality_kind_) |
+           types_.ToByte();
+  }
+  static byte ExtractTypesFromExtraICState(
       Code::ExtraICState state) {
-    return TypesField::decode(state);
-  }
-  static EqualityKind EqualityKindFromExtraICState(
-      Code::ExtraICState state) {
-    return EqualityKindField::decode(state);
-  }
-  static NilValue NilValueFromExtraICState(Code::ExtraICState state) {
-    return NilValueField::decode(state);
+    return state & ((1<<NUMBER_OF_TYPES)-1);
   }
 
-  static Types GetPatchedICFlags(Code::ExtraICState extra_ic_state,
-                                 Handle<Object> object,
-                                 bool* already_monomorphic);
+  void Record(Handle<Object> object);
+

[Sven Panne, 2013/05/13 14:24:50]

Do we really need all those tiny helper functions below? One reason could be
avoiding "train wrecks" like foo()->bar()->baz(), but I am not sure if this is
the case here.

[oliv, 2013/05/13 18:03:58]

i mostly introduced them to make the callees code more readable, but i agree,
having two different accessors is bad

+  bool IsMonomorphic() const { return types_.Contains(MONOMORPHIC_MAP); }
+  bool IsStrictEquality() const { return equality_kind_ == kStrictEquality; }
+  bool IsNullValue() const { return nil_value_ == kNullValue; }
+  EqualityKind GetKind() const { return equality_kind_; }
+  NilValue GetNilValue() const { return nil_value_; }
+  const Types GetTypes() const { return types_; }

[Sven Panne, 2013/05/13 14:24:50]

Why 2 consts?

[oliv, 2013/05/13 18:03:58]

1. the returned types shall be const
2. the method does not change the stub itself -> const

+  void ClearTypes() { types_.Clear(); }
+  void SetKind(EqualityKind kind) { equality_kind_ = kind; }
+
+  virtual void PrintName(StringStream* stream);
 
  private:
   friend class CompareNilIC;
 
-  class EqualityKindField : public BitField<EqualityKind, 0, 1> {};
-  class NilValueField : public BitField<NilValue, 1, 1> {};
-  class TypesField : public BitField<Types, 3, 4> {};
-
-  CompareNilICStub(EqualityKind kind, NilValue nil)
-      : HydrogenCodeStub(CODE_STUB_IS_MISS), bit_field_(0) {
-    bit_field_ = EqualityKindField::encode(kind) |
-        NilValueField::encode(nil);
+  CompareNilICStub(EqualityKind kind, NilValue nil,
+                   InitializationState init_state)
+      : HydrogenCodeStub(init_state), types_(0) {
+    equality_kind_ = kind;
+    nil_value_ = nil;
   }
 
+  CompareNilICStub(Code::ExtraICState ic_state, InitializationState init_state)
+      : HydrogenCodeStub(init_state) {
+    equality_kind_ = EqualityKindField::decode(ic_state);
+    nil_value_ = NilValueField::decode(ic_state);
+    types_ = Types(ExtractTypesFromExtraICState(ic_state));
+  }
+
+  class EqualityKindField : public BitField<EqualityKind, NUMBER_OF_TYPES, 1> {
+  };
+  class NilValueField : public BitField<NilValue, NUMBER_OF_TYPES+1, 1> {};
+
   virtual CodeStub::Major MajorKey() { return CompareNilIC; }
-  virtual int NotMissMinorKey() { return bit_field_; }
+  virtual int NotMissMinorKey() { return GetExtraICState(); }
 
-  int bit_field_;
+  EqualityKind equality_kind_;
+  NilValue nil_value_;
+  Types types_;
 
   DISALLOW_COPY_AND_ASSIGN(CompareNilICStub);
 };
 
 
 class CEntryStub : public PlatformCodeStub {
  public:
   explicit CEntryStub(int result_size,
                       SaveFPRegsMode save_doubles = kDontSaveFPRegs)
       : result_size_(result_size), save_doubles_(save_doubles) { }
@@ skipping 815 matching lines @@
 
   // The current function entry hook.
   static FunctionEntryHook entry_hook_;
 
   DISALLOW_COPY_AND_ASSIGN(ProfileEntryHookStub);
 };
 
 } }  // namespace v8::internal
 
 #endif  // V8_CODE_STUBS_H_