Change arm64 simulator register backing store.

src/arm64/simulator-arm64.h

Index: src/arm64/simulator-arm64.h
diff --git a/src/arm64/simulator-arm64.h b/src/arm64/simulator-arm64.h
index 6a7353b4612315200eb40d05d671ae47bd45e3ca..895550d43df0f190aef7ed4fe2dfaecb0a006938 100644
--- a/src/arm64/simulator-arm64.h
+++ b/src/arm64/simulator-arm64.h
@@ -159,35 +159,58 @@ class SimSystemRegister {
 
 
 // Represent a register (r0-r31, v0-v31).
-template<int kSizeInBytes>
 class SimRegisterBase {
  public:
-  template<typename T>
-  void Set(T new_value, unsigned size = sizeof(T)) {
-    ASSERT(size <= kSizeInBytes);
-    ASSERT(size <= sizeof(new_value));
+  void Set(int64_t new_value, unsigned size) {
+    ASSERT(size/8 <= kXRegSize);

[jbramley, 2014/03/27 17:50:42]

There are actually only two supported sizes at the moment; T in the original may
have had other sizes of course, but the _access_ is always 32- or 64-bits.
Perhaps this should assert that size is either kXRegSizeInBits or
kWRegSizeInBits (or kDRegSizeInBits or kSRegSizeInBits).

The use of kSizeInBytes in the original was to allow for NEON Q registers in the
future; this would require a 128-bit backing store and the ability to store 128
bits at a time.

[Fritz, 2014/03/27 19:58:33]

Is there a plan to use Q registers?  I think this method can safely extend to
128-bit registers.  uint128_t is available in gcc 4.8.  Not sure what compilers
are required to be supported for the compiler.

     // All AArch64 registers are zero-extending; Writing a W register clears the
     // top bits of the corresponding X register.
-    memset(value_, 0, kSizeInBytes);
-    memcpy(value_, &new_value, size);
+    value_ = new_value;
+    if (size == kSRegSizeInBits) {

[jbramley, 2014/03/27 17:50:42]

Why kSRegSize here? These can be any register type (currently X, W, D, S). I
think we would use kWRegSize by default, perhaps with a static assertion as you
used to check that kSRegSize == kWRegSize.

[Fritz, 2014/03/27 19:58:33]

I choose kSRegSize because I was following the logic in fpreg.  I now see this
was incorrect.

Done. 

+      value_ &= kSRegMask;
+    }
+  }
+
+  template<typename T>
+  void Set(T new_value) {
+    union SetUnion {
+      T new_value;
+      int64_t value;
+    } reg = { new_value };

[jbramley, 2014/03/27 17:50:42]

This isn't safe for type-aliasing rules. In another recent patch, we decided not
to worry about that, but it's worth noting it here.

On the other hand, a memcpy here should be free if the size is known at compile
time. Perhaps we could keep this type-aliasing-safe and still have the speed
advantage:

void Set(T new_value) {
  value_ = 0;
  memcpy(&value_, &new_value, sizeof(new_value));
}

void Set(T new_value, unsigned size) {
  if (size == sizeof(new_value)) {
    Set(new_value);
  } else {
    // Slower case.
  }
}

[Fritz, 2014/03/27 19:58:33]

I tried this method earlier.
https://codereview.chromium.org/203263017/
and it had to be reverted due to compiler bugs.  I think this method is closer
to what would be expected from the hardware, and faster.

I'm specifically trying to type-alias here in a safe way.  This is defining a
union that takes both a 32 or a 64 bit number and stores it at the same location
that is 64 bits in size.

[Sven Panne, 2014/03/28 11:46:04]

I don't think that such a thing is possible: IIRC, the only defined way of using
a union is accessing its fields consistently, i.e. read only out of the field
you have last written into, everything else is totally undefined (at least this
was the definition for C). Or in other words: The only safe way to use a union
is to save space, not to do some funny casting/conversion tricks.

Has this changed in C++? If yes, could somebody point me to the right section of
the spec? It would be good to know, because I'm quite sure we already had
trouble in this area.

[jbramley, 2014/03/28 11:59:58]

It seems to be allowed in C99 TC3 (dated the September 2007-09-07). See section
6.5.2.3, page 73: "If the member used to access the contents of a union object
is not the same as the member last used to store a value in the object, the
appropriate part of the object representation of the value is reinterpreted as
an object representation in the new type [...]"

I don't think it was allowed in TC2, but I don't have it to hand.
I don't think it's allowed in C++98 but I can't find a clear bit of the
specification that says that. However, it does have a rather surprising note on
reinterpret_cast suggesting that reinterpret_cast<T&>(x) can be used to safely
do type-punning. I'd not seen this before, and every resource I've seen until
now has said that this is _not_ safe. This (draft) standard is dated 2005-10-19
and I'm not sure where I got it from.

I still think that the only _portable_ way to do it is to use memcpy; that's
safe in every version of the standards, but the rules are rather more relaxed
than I previously thought.

+    STATIC_ASSERT(sizeof(union SetUnion) == sizeof(value_));

[jbramley, 2014/03/27 17:50:42]

That assertion isn't sufficient; if T is smaller than int64_t, sizeof(reg) will
still match sizeof(value_).

[Fritz, 2014/03/27 19:58:33]

Which is what I want.  From a conceptual standpoint I think that a union fits
what is going on with the register.  It has 64 bits, but in 32 bit mode only
half is accessible.  But they have the same address.  Just like a union.

I'll agree the initialization was incorrect.  C89 only initializes the first
element.  I can't find documentation on what happens if only the smaller element
is initialized.  You are probably right that the top half is then undefined.

+
+    value_ = reg.value;

[jbramley, 2014/03/27 17:50:42]

Again, if T is smaller than value_, this won't correctly zero-extend it.

   }
 
   // Copy 'size' bytes of the register to the result, and zero-extend to fill
   // the result.
-  template<typename T>
-  T Get(unsigned size = sizeof(T)) const {
-    ASSERT(size <= kSizeInBytes);
-    T result;
-    memset(&result, 0, sizeof(result));
-    memcpy(&result, value_, size);
+  int64_t Get(unsigned size) const {
+    ASSERT(size/8 <= kXRegSize);
+    int64_t result = value_;
+    if (size == kSRegSizeInBits) {
+      result &= kSRegMask;
+    }
     return result;
   }
 
+  template<typename T>
+  T Get() const {
+    union GetUnion {
+      int64_t value;
+      T result;
+    } reg = { value_ };
+    STATIC_ASSERT(sizeof(union GetUnion) == sizeof(value_));
+
+    return reg.result;
+  }
+
  protected:
-  uint8_t value_[kSizeInBytes];
+  int64_t value_;
 };
-typedef SimRegisterBase<kXRegSize> SimRegister;      // r0-r31
-typedef SimRegisterBase<kDRegSize> SimFPRegister;    // v0-v31
+
+STATIC_ASSERT(kXRegSize == kDRegSize);
+typedef SimRegisterBase SimRegister;      // r0-r31
+typedef SimRegisterBase SimFPRegister;    // v0-v31
 
 
 class Simulator : public DecoderVisitor {
@@ -354,33 +377,35 @@ class Simulator : public DecoderVisitor {
   VISITOR_LIST(DECLARE)
   #undef DECLARE
 
-  // Register accessors.
+  bool Reg31ZeroMode(unsigned code, Reg31Mode r31mode) const {
+    return ((code == 31) && (r31mode == Reg31IsZeroRegister));
+  }
 
+  // Register accessors.
   // Return 'size' bits of the value of an integer register, as the specified
   // type. The value is zero-extended to fill the result.
   //
   // The only supported values of 'size' are kXRegSizeInBits and
   // kWRegSizeInBits.
-  template<typename T>
-  T reg(unsigned size, unsigned code,
+  int64_t reg(unsigned size, unsigned code,
         Reg31Mode r31mode = Reg31IsZeroRegister) const {
-    unsigned size_in_bytes = size / 8;
-    ASSERT(size_in_bytes <= sizeof(T));
     ASSERT((size == kXRegSizeInBits) || (size == kWRegSizeInBits));
     ASSERT(code < kNumberOfRegisters);
 
-    if ((code == 31) && (r31mode == Reg31IsZeroRegister)) {
-      T result;
-      memset(&result, 0, sizeof(result));
-      return result;
+    if (Reg31ZeroMode(code, r31mode)) {
+      return 0;
     }
-    return registers_[code].Get<T>(size_in_bytes);
+    return registers_[code].Get(size);
   }
 
   // Like reg(), but infer the access size from the template type.
   template<typename T>
   T reg(unsigned code, Reg31Mode r31mode = Reg31IsZeroRegister) const {
-    return reg<T>(sizeof(T) * 8, code, r31mode);
+    ASSERT(code < kNumberOfRegisters);
+    if (Reg31ZeroMode(code, r31mode)) {
+      return 0;
+    }
+    return registers_[code].Get<T>();
   }
 
   // Common specialized accessors for the reg() template.
@@ -394,11 +419,6 @@ class Simulator : public DecoderVisitor {
     return reg<int64_t>(code, r31mode);
   }
 
-  int64_t reg(unsigned size, unsigned code,
-              Reg31Mode r31mode = Reg31IsZeroRegister) const {
-    return reg<int64_t>(size, code, r31mode);
-  }
-
   // Write 'size' bits of 'value' into an integer register. The value is
   // zero-extended. This behaviour matches AArch64 register writes.
   //
@@ -407,33 +427,31 @@ class Simulator : public DecoderVisitor {
   template<typename T>
   void set_reg(unsigned size, unsigned code, T value,
                Reg31Mode r31mode = Reg31IsZeroRegister) {
-    unsigned size_in_bytes = size / 8;
-    ASSERT(size_in_bytes <= sizeof(T));
     ASSERT((size == kXRegSizeInBits) || (size == kWRegSizeInBits));
     ASSERT(code < kNumberOfRegisters);
 
-    if ((code == 31) && (r31mode == Reg31IsZeroRegister)) {
-      return;
-    }
-    return registers_[code].Set(value, size_in_bytes);
+    if (!Reg31ZeroMode(code, r31mode))
+      registers_[code].Set(value, size);
   }
 
   // Like set_reg(), but infer the access size from the template type.
   template<typename T>
   void set_reg(unsigned code, T value,
                Reg31Mode r31mode = Reg31IsZeroRegister) {
-    set_reg(sizeof(value) * 8, code, value, r31mode);
+    ASSERT(code < kNumberOfRegisters);
+    if (!Reg31ZeroMode(code, r31mode))
+      registers_[code].Set(value);
   }
 
   // Common specialized accessors for the set_reg() template.
   void set_wreg(unsigned code, int32_t value,
                 Reg31Mode r31mode = Reg31IsZeroRegister) {
-    set_reg(kWRegSizeInBits, code, value, r31mode);
+    set_reg(code, value, r31mode);
   }
 
   void set_xreg(unsigned code, int64_t value,
                 Reg31Mode r31mode = Reg31IsZeroRegister) {
-    set_reg(kXRegSizeInBits, code, value, r31mode);
+    set_reg(code, value, r31mode);
   }
 
   // Commonly-used special cases.
@@ -458,24 +476,10 @@ class Simulator : public DecoderVisitor {
 
   Address get_sp() { return reg<Address>(31, Reg31IsStackPointer); }
 
-  // Return 'size' bits of the value of a floating-point register, as the
-  // specified type. The value is zero-extended to fill the result.
-  //
-  // The only supported values of 'size' are kDRegSizeInBits and
-  // kSRegSizeInBits.
-  template<typename T>
-  T fpreg(unsigned size, unsigned code) const {
-    unsigned size_in_bytes = size / 8;
-    ASSERT(size_in_bytes <= sizeof(T));
-    ASSERT((size == kDRegSizeInBits) || (size == kSRegSizeInBits));
-    ASSERT(code < kNumberOfFPRegisters);
-    return fpregisters_[code].Get<T>(size_in_bytes);
-  }
-
-  // Like fpreg(), but infer the access size from the template type.
   template<typename T>
   T fpreg(unsigned code) const {
-    return fpreg<T>(sizeof(T) * 8, code);
+    ASSERT(code < kNumberOfRegisters);
+    return fpregisters_[code].Get<T>();
   }
 
   // Common specialized accessors for the fpreg() template.
@@ -511,7 +515,7 @@ class Simulator : public DecoderVisitor {
   void set_fpreg(unsigned code, T value) {
     ASSERT((sizeof(value) == kDRegSize) || (sizeof(value) == kSRegSize));
     ASSERT(code < kNumberOfFPRegisters);
-    fpregisters_[code].Set(value, sizeof(value));
+    fpregisters_[code].Set(value);
   }
 
   // Common specialized accessors for the set_fpreg() template.