Change arm64 simulator register backing store.

src/arm64/simulator-arm64.cc

 // Copyright 2013 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <stdlib.h>
 #include <cmath>
 #include <cstdarg>
 #include "v8.h"
 
 #if V8_TARGET_ARCH_ARM64
@@ skipping 860 matching lines @@
   if (set_flags) {
     nzcv().SetN(N);
     nzcv().SetZ(Z);
     nzcv().SetC(C);
     nzcv().SetV(V);
   }
   return result;
 }
 
 
-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();
       return 0;
   }
 }
 
 
 int64_t Simulator::ExtendValue(unsigned reg_size,
                                int64_t value,
@@ skipping 365 matching lines @@
       break;
     }
     default: UNREACHABLE();
   }
 
   set_reg(reg_size, instr->Rd(), new_val, instr->RdMode());
 }
 
 
 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);
 }
 
 
 void Simulator::VisitAddSubImmediate(Instruction* instr) {
   int64_t op2 = instr->ImmAddSub() << ((instr->ShiftAddSub() == 1) ? 12 : 0);
   AddSubHelper(instr, op2);
 }
 
 
@@ skipping 22 matching lines @@
                          instr->FlagsUpdate(),
                          reg(reg_size, instr->Rn()),
                          op2,
                          nzcv().C());
 
   set_reg(reg_size, instr->Rd(), new_val);
 }
 
 
 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;
   }
   LogicalHelper(instr, op2);
 }
 
 
 void Simulator::VisitLogicalImmediate(Instruction* instr) {
   LogicalHelper(instr, instr->ImmLogical());
 }
@@ skipping 547 matching lines @@
                                                {4, 5, 6, 7, 0, 1, 2, 3},
                                                {0, 1, 2, 3, 4, 5, 6, 7} };
   uint64_t result = 0;
   for (int i = 0; i < 8; i++) {
     result <<= 8;
     result |= bytes[permute_table[mode][i]];
   }
   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;
+    case SDIV_w:
+    case SDIV_x: {
+      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;
-      } else {
-        result = rn / rm;
-      }
-      break;
-    }
-    case SDIV_x: {
-      int64_t rn = xreg(instr->Rn());
-      int64_t rm = xreg(instr->Rm());
-      if ((rn == kXMinInt) && (rm == -1)) {
-        result = kXMinInt;
-      } else 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: {
-      uint32_t rn = static_cast<uint32_t>(wreg(instr->Rn()));
-      uint32_t rm = static_cast<uint32_t>(wreg(instr->Rm()));
+    case UDIV_w:
+    case UDIV_x: {
+      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;
-      } else {
-        result = rn / rm;
-      }
-      break;
-    }
-    case UDIV_x: {
-      uint64_t rn = static_cast<uint64_t>(xreg(instr->Rn()));
-      uint64_t rm = static_cast<uint64_t>(xreg(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 LSLV_w:
     case LSLV_x: shift_op = LSL; break;
     case LSRV_w:
     case LSRV_x: shift_op = LSR; break;
     case ASRV_w:
     case ASRV_x: shift_op = ASR; break;
     case RORV_w:
     case RORV_x: shift_op = ROR; break;
     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(reg_size, instr->Rd(), result);
+  set_reg<T>(instr->Rd(), result);
 }
 
 
+void Simulator::VisitDataProcessing2Source(Instruction* instr) {
+  if (instr->SixtyFourBits()) {
+    DataProcessing2Source<int64_t>(instr);
+  } else {
+    DataProcessing2Source<int32_t>(instr);
+  }
+}
+
+
 // The algorithm used is described in section 8.2 of
 //   Hacker's Delight, by Henry S. Warren, Jr.
 // It assumes that a right shift on a signed integer is an arithmetic shift.
 static int64_t MultiplyHighSigned(int64_t u, int64_t v) {
   uint64_t u0, v0, w0;
   int64_t u1, v1, w1, w2, t;
 
   u0 = u & 0xffffffffL;
   u1 = u >> 32;
   v0 = v & 0xffffffffL;
@@ skipping 1722 matching lines @@
 
   delete[] format;
 }
 
 
 #endif  // USE_SIMULATOR
 
 } }  // namespace v8::internal
 
 #endif  // V8_TARGET_ARCH_ARM64