Embedded constant pools.

src/assembler.h

Index: src/assembler.h
diff --git a/src/assembler.h b/src/assembler.h
index fd66e0bfdb111b22c74f562cae1640c191481c2f..258547ffde36343c260693cf9861a92d0bcff97c 100644
--- a/src/assembler.h
+++ b/src/assembler.h
@@ -79,11 +79,11 @@ class AssemblerBase: public Malloced {
     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;
     }
@@ -108,11 +108,11 @@ class AssemblerBase: public Malloced {
   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();
     }
   }
@@ -130,7 +130,7 @@ class AssemblerBase: public Malloced {
 
   // 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;
@@ -413,9 +413,6 @@ class 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 &&
@@ -487,22 +484,11 @@ class RelocInfo {
   }
   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; }
 
@@ -645,10 +631,7 @@ class RelocInfo {
   // 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
@@ -1167,6 +1150,83 @@ class NullCallWrapper : public CallWrapper {
 };
 
 
+// -----------------------------------------------------------------------------
+// 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_