Embedded constant pools.

src/assembler.h

 // Copyright (c) 1994-2006 Sun Microsystems Inc.
 // 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.
 //
@@ skipping 61 matching lines @@
   void set_predictable_code_size(bool value) { predictable_code_size_ = value; }
 
   uint64_t enabled_cpu_features() const { return enabled_cpu_features_; }
   void set_enabled_cpu_features(uint64_t features) {
     enabled_cpu_features_ = features;
   }
   bool IsEnabled(CpuFeature f) {
     return (enabled_cpu_features_ & (static_cast<uint64_t>(1) << f)) != 0;
   }
 
-  bool is_ool_constant_pool_available() const {
-    if (FLAG_enable_ool_constant_pool) {
-      return ool_constant_pool_available_;
+  bool is_constant_pool_available() const {
+    if (FLAG_enable_embedded_constant_pool) {
+      return constant_pool_available_;
     } else {
-      // Out-of-line constant pool not supported on this architecture.
+      // Constant pool not supported on this architecture.

[rmcilroy, 2015/05/20 14:32:11]

Embedded constant... (and below)

[MTBrandyberry, 2015/05/20 22:28:22]

Done.

       UNREACHABLE();
       return false;
     }
   }
 
   // Overwrite a host NaN with a quiet target NaN.  Used by mksnapshot for
   // cross-snapshotting.
   static void QuietNaN(HeapObject* nan) { }
 
   int pc_offset() const { return static_cast<int>(pc_ - buffer_); }
 
   // This function is called when code generation is aborted, so that
   // the assembler could clean up internal data structures.
   virtual void AbortedCodeGeneration() { }
 
   static const int kMinimalBufferSize = 4*KB;
 
  protected:
   // The buffer into which code and relocation info are generated. It could
   // either be owned by the assembler or be provided externally.
   byte* buffer_;
   int buffer_size_;
   bool own_buffer_;
 
-  void set_ool_constant_pool_available(bool available) {
-    if (FLAG_enable_ool_constant_pool) {
-      ool_constant_pool_available_ = available;
+  void set_constant_pool_available(bool available) {
+    if (FLAG_enable_embedded_constant_pool) {
+      constant_pool_available_ = available;
     } else {
-      // Out-of-line constant pool not supported on this architecture.
+      // Constant pool not supported on this architecture.
       UNREACHABLE();
     }
   }
 
   // The program counter, which points into the buffer above and moves forward.
   byte* pc_;
 
  private:
   Isolate* isolate_;
   int jit_cookie_;
   uint64_t enabled_cpu_features_;
   bool emit_debug_code_;
   bool predictable_code_size_;
   bool serializer_enabled_;
 
   // Indicates whether the constant pool can be accessed, which is only possible
   // if the pp register points to the current code object's constant pool.
-  bool ool_constant_pool_available_;
+  bool constant_pool_available_;
 
   // Constant pool.
   friend class FrameAndConstantPoolScope;
   friend class ConstantPoolUnavailableScope;
 };
 
 
 // Avoids emitting debug code during the lifetime of this scope object.
 class DontEmitDebugCodeScope BASE_EMBEDDED {
  public:
@@ skipping 262 matching lines @@
     // Modes <= LAST_COMPACT_ENUM are guaranteed to have compact encoding.
     LAST_COMPACT_ENUM = CODE_TARGET_WITH_ID,
     LAST_STANDARD_NONCOMPACT_ENUM = INTERNAL_REFERENCE_ENCODED
   };
 
   RelocInfo() {}
 
   RelocInfo(byte* pc, Mode rmode, intptr_t data, Code* host)
       : pc_(pc), rmode_(rmode), data_(data), host_(host) {
   }
-  RelocInfo(byte* pc, double data64)
-      : pc_(pc), rmode_(NONE64), data64_(data64), host_(NULL) {
-  }
 
   static inline bool IsRealRelocMode(Mode mode) {
     return mode >= FIRST_REAL_RELOC_MODE &&
         mode <= LAST_REAL_RELOC_MODE;
   }
   static inline bool IsPseudoRelocMode(Mode mode) {
     DCHECK(!IsRealRelocMode(mode));
     return mode >= FIRST_PSEUDO_RELOC_MODE &&
         mode <= LAST_PSEUDO_RELOC_MODE;
   }
@@ skipping 51 matching lines @@
     return mode == DEBUG_BREAK;
   }
   static inline bool IsNone(Mode mode) {
     return mode == NONE32 || mode == NONE64;
   }
   static inline bool IsCodeAgeSequence(Mode mode) {
     return mode == CODE_AGE_SEQUENCE;
   }
   static inline int ModeMask(Mode mode) { return 1 << mode; }
 
-  // Returns true if the first RelocInfo has the same mode and raw data as the
-  // second one.
-  static inline bool IsEqual(RelocInfo first, RelocInfo second) {
-    return first.rmode() == second.rmode() &&
-           (first.rmode() == RelocInfo::NONE64 ?
-              first.raw_data64() == second.raw_data64() :
-              first.data() == second.data());
-  }
-
   // Accessors
   byte* pc() const { return pc_; }
   void set_pc(byte* pc) { pc_ = pc; }
   Mode rmode() const {  return rmode_; }
   intptr_t data() const { return data_; }
-  double data64() const { return data64_; }
-  uint64_t raw_data64() { return bit_cast<uint64_t>(data64_); }
   Code* host() const { return host_; }
   void set_host(Code* host) { host_ = host; }
 
   // Apply a relocation by delta bytes
   INLINE(void apply(intptr_t delta,
                     ICacheFlushMode icache_flush_mode =
                         FLUSH_ICACHE_IF_NEEDED));
 
   // Is the pointer this relocation info refers to coded like a plain pointer
   // or is it strange in some way (e.g. relative or patched into a series of
@@ skipping 122 matching lines @@
       (1 << CODE_TARGET_WITH_ID) | kPositionMask | (1 << COMMENT);
   static const int kApplyMask;  // Modes affected by apply. Depends on arch.
 
  private:
   // On ARM, note that pc_ is the address of the constant pool entry
   // to be relocated and not the address of the instruction
   // referencing the constant pool entry (except when rmode_ ==
   // comment).
   byte* pc_;
   Mode rmode_;
-  union {
-    intptr_t data_;
-    double data64_;
-  };
+  intptr_t data_;
   Code* host_;
   // External-reference pointers are also split across instruction-pairs
   // on some platforms, but are accessed via indirect pointers. This location
   // provides a place for that pointer to exist naturally. Its address
   // is returned by RelocInfo::target_reference_address().
   Address reconstructed_adr_ptr_;
   friend class RelocIterator;
 };
 
 
@@ skipping 498 matching lines @@
 
 class NullCallWrapper : public CallWrapper {
  public:
   NullCallWrapper() { }
   virtual ~NullCallWrapper() { }
   virtual void BeforeCall(int call_size) const { }
   virtual void AfterCall() const { }
 };
 
 
+// -----------------------------------------------------------------------------
+// Embedded constant pool support
+
+class ConstantPoolEntry {
+ public:
+  ConstantPoolEntry() {}
+  ConstantPoolEntry(int position, intptr_t value, bool sharing_ok)
+      : position_(position),
+        merged_index_(sharing_ok ? -1 : -2),

[rmcilroy, 2015/05/20 14:32:11]

Define and use constants for '-1' and '-2' to clarify their meaning

[MTBrandyberry, 2015/05/20 22:28:22]

Done.

+        value_(value) {}
+  ConstantPoolEntry(int position, double value)
+      : position_(position), merged_index_(-1), value64_(value) {}
+
+  int position() const { return position_; }
+  bool sharing_ok() const { return merged_index_ != -2; }
+  intptr_t value() const { return value_; }
+  uint64_t value64() const { return bit_cast<uint64_t>(value64_); }
+
+  enum Type { INTPTR, DOUBLE, NUMBER_OF_TYPES };
+
+  static int size(Type type) {
+    return ((kPointerSize == kDoubleSize || type == INTPTR) ? kPointerSize

[rmcilroy, 2015/05/20 14:32:11]

I think all you need here is:
  return (type == INTPTR) ? kPointerSize : kDoubleSize

[MTBrandyberry, 2015/05/20 22:28:22]

My thinking was that the compiler could eliminate the condition all together on
64-bit architectures.

[rmcilroy, 2015/05/22 12:21:22]

If the compiler is smart enough to do that, then it is also probably smart
enough to do constant propigation and work out kPointerSize and kDoubleSize are
the same and also eliminate the condition. In anycase, I don't think this
microoptimisation is worth the reduced readability unless you have specific
numbers to back up it being worthwhile.

[MTBrandyberry, 2015/05/26 19:19:05]

Acknowledged.

+                                                            : kDoubleSize);
+  }
+
+  enum Access { REGULAR, OVERFLOWED };
+
+ private:
+  int position_;
+  int merged_index_;
+  union {
+    intptr_t value_;
+    double value64_;
+  };
+
+  friend class ConstantPoolBuilder;
+};
+
+
+class ConstantPoolBuilder BASE_EMBEDDED {

[rmcilroy, 2015/05/20 14:32:11]

Thanks for factoring this out into common code.  Could you please add some
unittests which test this class and the various corner cases (e.g., overflowing
constant pools, checking the emitted layout etc.).

[MTBrandyberry, 2015/05/20 22:28:22]

I will resolve the other comments here in the next iteration and tackle the
unittests in parallel with the 2nd review.

+ public:
+  ConstantPoolBuilder(int ptr_reach, int double_reach);

[rmcilroy, 2015/05/20 14:32:11]

ptr_reach_bits and double_reach_bits

[MTBrandyberry, 2015/05/20 22:28:22]

Done.

+  ConstantPoolEntry::Access AddEntry(int position, intptr_t value,
+                                     bool sharing_ok) {
+    ConstantPoolEntry entry(position, value, sharing_ok);
+    return AddEntry(entry, ConstantPoolEntry::INTPTR);
+  }
+  ConstantPoolEntry::Access AddEntry(int position, double value) {
+    ConstantPoolEntry entry(position, value);
+    return AddEntry(entry, ConstantPoolEntry::DOUBLE);
+  }
+  ConstantPoolEntry::Access NextAccess(ConstantPoolEntry::Type type) const;
+  int Emit(Assembler* assm);
+  inline Label* Position() { return &label_; }

[rmcilroy, 2015/05/20 14:32:11]

/s/Position/EmittedPosition.  Also some comments for each of the public methods
would be nice.

[MTBrandyberry, 2015/05/20 22:28:22]

Done.

+
+ private:
+  ConstantPoolEntry::Access AddEntry(ConstantPoolEntry& entry,
+                                     ConstantPoolEntry::Type type);
+  void EmitGroup(Assembler* assm, ConstantPoolEntry::Access access,
+                 ConstantPoolEntry::Type type);
+
+  struct TypeInfo {

[rmcilroy, 2015/05/20 14:32:11]

nit - TypeInfo seems a bit confusingly named. How about PerTypeEntryDetails or
similar?

[MTBrandyberry, 2015/05/20 22:28:22]

Done.  PerTypeEntryInfo?

[rmcilroy, 2015/05/22 12:21:22]

SGTM

+    TypeInfo() : regular_count(0), overflow_start(-1) {}
+    bool overflow() const {
+      return (overflow_start >= 0 &&
+              overflow_start < static_cast<int>(entries.size()));
+    }
+    int regular_reach;
+    int regular_count;
+    int overflow_start;
+    std::vector<ConstantPoolEntry> entries;
+    std::vector<ConstantPoolEntry> shared_entries;
+  };
+
+  Label label_;

[rmcilroy, 2015/05/20 14:32:11]

/s/label_/emitted_label_ (and a comment describing what this is would be nice).

[MTBrandyberry, 2015/05/20 22:28:22]

Done.

+  TypeInfo info_[ConstantPoolEntry::NUMBER_OF_TYPES];
+};

[rmcilroy, 2015/05/20 14:32:11]

two newlines here

[MTBrandyberry, 2015/05/20 22:28:22]

Done.

 } }  // namespace v8::internal
 
 #endif  // V8_ASSEMBLER_H_