Change arm64 simulator register backing store.

src/arm64/simulator-arm64.cc

Index: src/arm64/simulator-arm64.cc
diff --git a/src/arm64/simulator-arm64.cc b/src/arm64/simulator-arm64.cc
index c7c9f6cedd9105aef766f45f273dfcf8226d9fc1..3d7cf97dd15d398bd350d4ed0548133200f7dd34 100644
--- a/src/arm64/simulator-arm64.cc
+++ b/src/arm64/simulator-arm64.cc
@@ -878,32 +878,38 @@ int64_t Simulator::AddWithCarry(unsigned reg_size,
 }
 
 
-int64_t Simulator::ShiftOperand(unsigned reg_size,
-                                int64_t value,
-                                Shift shift_type,
-                                unsigned amount) {
+template<> struct make_unsigned<int32_t> {
+  public:
+    typedef uint32_t type;
+};
+
+
+template<> struct make_unsigned<int64_t> {
+  public:

[jbramley, 2014/05/16 14:05:50]

Is 'public' necessary here?

[Fritz, 2014/05/16 18:11:46]

Yes.  Below it is accessed make_unsigned<T>::type

[Sven Panne, 2014/05/19 08:12:26]

Huh? struct = class + public, so this shouldn't be needed, unless there is some
arcane template/namespace/overloading/... resolution rule which I don't know.
:-)

[Fritz, 2014/05/20 19:44:24]

Your correct.  My mistake.
Done.

+    typedef uint64_t type;
+};
+
+
+template <typename T>
+T Simulator::ShiftOperand(T value, Shift shift_type, unsigned amount) {
+  typedef typename make_unsigned<T>::type unsignedT;

[jbramley, 2014/05/16 14:05:50]

I don't like this mechanism I'm afraid; it's not a common idiom (as far as I
know) and it's pretty confusing at first. If you can't replace it with something
cleaner, at least put the main declaration in the .cc file with the
specialisations and then add a very clear comment explaining what's going on.

Another option would be to put it in the global 'utils.h'.

In this case, can't you just operate on uint64_t values and then cast to <T> at
the end? (I presume that the purpose is to get ShiftOperand to return the proper
type, instead of just int64_t.)

[Fritz, 2014/05/16 18:11:46]

I haven't found away around this yet.

Propagating a int32_t to a uint64_t sign extends it first.
int32_t value = 0x80000000;
int32_t result32 = static_cast<uint64_t>(value) >> 16; // = 0xffff8000
int32_t result64 = static_cast<uint32_t>(value) >> 16; // = 0x00008000

Register read/write access is signed at the moment.  I could templatize
read_wreg<uint32_t> or do read_wreg_unsigned.  But I was trying to be simpler
than that.

I was trying to do a drop in replacement without changing the algorithm. 
std::make_unsigned is present in c++11
(http://www.cplusplus.com/reference/type_traits/make_unsigned/)
So the idea isn't mine.  I was just trying to backport it.  And the
implementation isn't mine either, it's cribbed together mostly from here
(http://stackoverflow.com/questions/16461561/c-conversion-from-t-to-unsigned-t)

I will look for a better alternative.

+
   if (amount == 0) {
     return value;
   }
-  int64_t mask = reg_size == kXRegSizeInBits ? kXRegMask : kWRegMask;
+
   switch (shift_type) {
     case LSL:
-      return (value << amount) & mask;
+      return value << amount;
     case LSR:
-      return static_cast<uint64_t>(value) >> amount;
+      return static_cast<unsignedT>(value) >> amount;
     case ASR: {
-      // Shift used to restore the sign.
-      unsigned s_shift = kXRegSizeInBits - reg_size;
-      // Value with its sign restored.
-      int64_t s_value = (value << s_shift) >> s_shift;
-      return (s_value >> amount) & mask;
+      return value >> amount;
     }
     case ROR: {
-      if (reg_size == kWRegSizeInBits) {
-        value &= kWRegMask;
-      }
-      return (static_cast<uint64_t>(value) >> amount) |
-             ((value & ((1L << amount) - 1L)) << (reg_size - amount));
+      return (static_cast<unsignedT>(value) >> amount) |
+              ((value & ((1L << amount) - 1L)) <<
+                  (sizeof(unsignedT) * 8 - amount));
     }
     default:
       UNIMPLEMENTED();
@@ -1289,12 +1295,17 @@ void Simulator::AddSubHelper(Instruction* instr, int64_t op2) {
 
 
 void Simulator::VisitAddSubShifted(Instruction* instr) {
-  unsigned reg_size = instr->SixtyFourBits() ? kXRegSizeInBits
-                                             : kWRegSizeInBits;
-  int64_t op2 = ShiftOperand(reg_size,
-                             reg(reg_size, instr->Rm()),
-                             static_cast<Shift>(instr->ShiftDP()),
-                             instr->ImmDPShift());
+  int64_t op2 = 0;
+  if (instr->SixtyFourBits()) {
+    op2 = ShiftOperand(xreg(instr->Rm()),
+                       static_cast<Shift>(instr->ShiftDP()),
+                       instr->ImmDPShift());
+  } else {
+    op2 = ShiftOperand(wreg(instr->Rm()),
+                       static_cast<Shift>(instr->ShiftDP()),
+                       instr->ImmDPShift());

[jbramley, 2014/05/16 14:05:50]

This would be clearer (and fit on one line) if it declared shift_type and
shift_amount, like in VisitLogicalShifted.

[Fritz, 2014/05/16 18:11:46]

Done.

+    op2 &= kWRegMask;
+  }
   AddSubHelper(instr, op2);
 }
 
@@ -1337,12 +1348,17 @@ void Simulator::VisitAddSubWithCarry(Instruction* instr) {
 
 
 void Simulator::VisitLogicalShifted(Instruction* instr) {
-  unsigned reg_size = instr->SixtyFourBits() ? kXRegSizeInBits
-                                             : kWRegSizeInBits;
   Shift shift_type = static_cast<Shift>(instr->ShiftDP());
   unsigned shift_amount = instr->ImmDPShift();
-  int64_t op2 = ShiftOperand(reg_size, reg(reg_size, instr->Rm()), shift_type,
-                             shift_amount);
+  int64_t op2 = 0;
+
+  if (instr->SixtyFourBits()) {
+    op2 = ShiftOperand(xreg(instr->Rm()), shift_type, shift_amount);
+  } else {
+    op2 = ShiftOperand(wreg(instr->Rm()), shift_type, shift_amount);
+    op2 &= kWRegMask;

[jbramley, 2014/05/16 14:05:50]

Doesn't the new ShiftOperand apply that mask? If so, that would be a good
assertion.

[Fritz, 2014/05/16 18:11:46]

ShiftOperand was returning a signed int64_t.  It's returned as a int32_t and
propagated to a int64_t.

This has been an iterative process of trying to replace one function at a time. 
Looking at this no, ShiftOperand can probably return an unsigned.  LogicalHelper
writes the register and needs to be templatized so that it isn't writing using
reg_size.

This is what I was referring to in regards to transitory functions.

+  }
+
   if (instr->Mask(NOT) == NOT) {
     op2 = ~op2;
   }
@@ -1910,29 +1926,19 @@ uint64_t Simulator::ReverseBytes(uint64_t value, ReverseByteMode mode) {
   return result;
 }
 
-
-void Simulator::VisitDataProcessing2Source(Instruction* instr) {
+template <typename T>
+void Simulator::DataProcessing2Source(Instruction* instr) {
+  typedef typename make_unsigned<T>::type unsignedT;
   Shift shift_op = NO_SHIFT;
-  int64_t result = 0;
+  T result = 0;
+  const T kMinimumInteger = static_cast<T>(1) << (sizeof(T) * 8 - 1);

[jbramley, 2014/05/16 14:05:50]

Consider adding a helper function to do this (perhaps in utils.h):

template <typename T> T MinInt();
template <> int64_t MinInt<int64_t>() { return INT64_MIN; }
template <> int32_t MinInt<int32_t>() { return INT32_MIN; }

We do that for FPDefaultNaN for example, for doubles and floats.

[Fritz, 2014/05/16 18:11:46]

Thanks, great idea.

[Sven Panne, 2014/05/19 08:12:26]

Even better idea: Use <limits>:
http://www.cplusplus.com/reference/limits/numeric_limits/

[Fritz, 2014/05/20 19:44:24]

Done.

   switch (instr->Mask(DataProcessing2SourceMask)) {
-    case SDIV_w: {
-      int32_t rn = wreg(instr->Rn());
-      int32_t rm = wreg(instr->Rm());
-      if ((rn == kWMinInt) && (rm == -1)) {
-        result = kWMinInt;
-      } else if (rm == 0) {
-        // Division by zero can be trapped, but not on A-class processors.
-        result = 0;
-      } else {
-        result = rn / rm;
-      }
-      break;
-    }
+    case SDIV_w:
     case SDIV_x: {
-      int64_t rn = xreg(instr->Rn());
-      int64_t rm = xreg(instr->Rm());
-      if ((rn == kXMinInt) && (rm == -1)) {
-        result = kXMinInt;
+      T rn = reg<T>(instr->Rn());
+      T rm = reg<T>(instr->Rm());
+      if ((rn == kMinimumInteger) && (rm == -1)) {
+        result = kMinimumInteger;
       } else if (rm == 0) {
         // Division by zero can be trapped, but not on A-class processors.
         result = 0;
@@ -1941,20 +1947,10 @@ void Simulator::VisitDataProcessing2Source(Instruction* instr) {
       }
       break;
     }
-    case UDIV_w: {
-      uint32_t rn = static_cast<uint32_t>(wreg(instr->Rn()));
-      uint32_t rm = static_cast<uint32_t>(wreg(instr->Rm()));
-      if (rm == 0) {
-        // Division by zero can be trapped, but not on A-class processors.
-        result = 0;
-      } else {
-        result = rn / rm;
-      }
-      break;
-    }
+    case UDIV_w:
     case UDIV_x: {
-      uint64_t rn = static_cast<uint64_t>(xreg(instr->Rn()));
-      uint64_t rm = static_cast<uint64_t>(xreg(instr->Rm()));
+      unsignedT rn = static_cast<unsignedT>(reg<T>(instr->Rn()));
+      unsignedT rm = static_cast<unsignedT>(reg<T>(instr->Rm()));
       if (rm == 0) {
         // Division by zero can be trapped, but not on A-class processors.
         result = 0;
@@ -1974,18 +1970,27 @@ void Simulator::VisitDataProcessing2Source(Instruction* instr) {
     default: UNIMPLEMENTED();
   }
 
-  unsigned reg_size = instr->SixtyFourBits() ? kXRegSizeInBits
-                                             : kWRegSizeInBits;
   if (shift_op != NO_SHIFT) {
     // Shift distance encoded in the least-significant five/six bits of the
     // register.
-    int mask = (instr->SixtyFourBits() == 1) ? kShiftAmountXRegMask
-                                             : kShiftAmountWRegMask;
-    unsigned shift = wreg(instr->Rm()) & mask;
-    result = ShiftOperand(reg_size, reg(reg_size, instr->Rn()), shift_op,
-                          shift);
+    unsigned shift = wreg(instr->Rm());
+    if (sizeof(T) == kWRegSize) {
+      shift &= kShiftAmountWRegMask;
+    } else {
+      shift &= kShiftAmountXRegMask;
+    }
+    result = ShiftOperand(reg<T>(instr->Rn()), shift_op, shift);
+  }
+  set_reg<T>(instr->Rd(), result);
+}
+
+
+void Simulator::VisitDataProcessing2Source(Instruction* instr) {
+  if (instr->SixtyFourBits()) {
+    DataProcessing2Source<int64_t>(instr);
+  } else {
+    DataProcessing2Source<int32_t>(instr);
   }
-  set_reg(reg_size, instr->Rd(), result);
 }