arch-arm: Fix Compare and Swap Pair instructions

Those instructions were broken after:

https://gem5-review.googlesource.com/c/public/gem5/+/38381/4

Which is effectively replacing the generic StaticInst src and dest
reg array with an instruction specific one.
The size of the array is evaluated by the ISA parser, which is
counting the operands when parsing the isa code.
Alas, Compare and Swap Pair instructions were augmenting the number
of destination and source registers in the C++ world, which is
invisible to the parser. This lead to an out of bounds access
of the arrays.

This patch is fixing this behaviour by defining XResult2, which
is the second compare/result register for a paired CAS

Change-Id: Ie35c26256f42459805e007847896ac58b178fd42
Signed-off-by: Giacomo Travaglini <giacomo.travaglini@arm.com>
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/39456
Reviewed-by: Richard Cooper <richard.cooper@arm.com>
Reviewed-by: Gabe Black <gabe.black@gmail.com>
Tested-by: kokoro <noreply+kokoro@google.com>

diff --git a/src/arch/arm/insts/mem64.cc b/src/arch/arm/insts/mem64.cc
index a12c33058238c5e5c8f5f1eb23bdcdf0d2033490..ed159becaf582181c13be92518f77cfe4861a1ef 100644 (file)
--- a/src/arch/arm/insts/mem64.cc
+++ b/src/arch/arm/insts/mem64.cc
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2011-2013,2018 ARM Limited
+ * Copyright (c) 2011-2013,2018, 2021 ARM Limited
  * All rights reserved
  *
  * The license below extends only to copyright in the software and shall
@@ -200,4 +200,24 @@ MemoryLiteral64::generateDisassembly(
     ccprintf(ss, ", #%d", pc + imm);
     return ss.str();
 }
+
+std::string
+MemoryAtomicPair64::generateDisassembly(
+        Addr pc, const Loader::SymbolTable *symtab) const
+{
+    std::stringstream ss;
+    printMnemonic(ss, "", false);
+    printIntReg(ss, result);
+    ccprintf(ss, ", ");
+    printIntReg(ss, result2);
+    ccprintf(ss, ", ");
+    printIntReg(ss, dest);
+    ccprintf(ss, ", ");
+    printIntReg(ss, dest2);
+    ccprintf(ss, ", [");
+    printIntReg(ss, base);
+    ccprintf(ss, "]");
+    return ss.str();
+}
+
 }

diff --git a/src/arch/arm/insts/mem64.hh b/src/arch/arm/insts/mem64.hh
index 412efb0ad4b6cbc160a14ca98d2dea49a35e3bf8..b602d5470b73fdd717019765d352d60009f60530 100644 (file)
--- a/src/arch/arm/insts/mem64.hh
+++ b/src/arch/arm/insts/mem64.hh
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2011-2013,2017-2019 ARM Limited
+ * Copyright (c) 2011-2013,2017-2019, 2021 ARM Limited
  * All rights reserved
  *
  * The license below extends only to copyright in the software and shall
@@ -263,6 +263,26 @@ class MemoryLiteral64 : public Memory64
             Addr pc, const Loader::SymbolTable *symtab) const override;
 };
 
+class MemoryAtomicPair64 : public Memory64
+{
+  protected:
+    IntRegIndex dest2;
+    IntRegIndex result;
+    IntRegIndex result2;
+
+    MemoryAtomicPair64(const char *mnem, ExtMachInst _machInst,
+                       OpClass __opClass, IntRegIndex _dest, IntRegIndex _base,
+                       IntRegIndex _result)
+        : Memory64(mnem, _machInst, __opClass, _dest, _base),
+          dest2((IntRegIndex)(_dest + (IntRegIndex)(1))),
+          result(_result),
+          result2((IntRegIndex)(_result + (IntRegIndex)(1)))
+    {}
+
+    std::string generateDisassembly(
+            Addr pc, const Loader::SymbolTable *symtab) const override;
+};
+
 }
 
 #endif //__ARCH_ARM_INSTS_MEM_HH__

diff --git a/src/arch/arm/isa/insts/amo64.isa b/src/arch/arm/isa/insts/amo64.isa
index 51e1f38e53a2bf258e6d40c5dff944ff147f09f3..27ee9b595ab78c380928e481a05f85e3e8179c79 100644 (file)
--- a/src/arch/arm/isa/insts/amo64.isa
+++ b/src/arch/arm/isa/insts/amo64.isa
@@ -1,5 +1,6 @@
 // -*- mode:c++ -*-
 
+// Copyright (c) 2021 ARM Limited
 // Copyright (c) 2018 Metempsy Technology Consulting
 // All rights reserved
 //
@@ -259,8 +260,8 @@ let {{
                    flavor="release").emit(OP_DICT['CAS'])
 
     class CasPair64(AtomicInst64):
-        decConstBase = 'AmoPairOp'
-        base = 'ArmISA::MemoryEx64'
+        decConstBase = 'AmoOp'
+        base = 'ArmISA::MemoryAtomicPair64'
         writeback = True
         post = False
         execBase = 'AmoOp'
@@ -279,8 +280,8 @@ let {{
                                           isBigEndian64(xc->tcBase()));
                             uint64_t result2 = cSwap(Mem%(suffix)s[1],
                                            isBigEndian64(xc->tcBase()));
-                            xc->setIntRegOperand(this, r2_dst, (result2)
-                                                    & mask(aarch64 ? 64 : 32));
+
+                            XResult2 = result2;
                             '''
             elif self.size == 4:
                 self.res = 'Result_uw'
@@ -296,8 +297,8 @@ let {{
                     uint32_t result2 = isBigEndian64(xc->tcBase())
                                    ? (uint32_t)data
                                    : (data >> 32);
-                    xc->setIntRegOperand(this, r2_dst, (result2) &
-                                                mask(aarch64 ? 64 : 32));
+
+                    XResult2 = result2;
                             '''
 
             store_res = store_res % {"result":self.res, "suffix":self.suffix}
@@ -312,18 +313,13 @@ let {{
             # Code that actually handles the access
 
             if self.size == 4:
-                accCode = \
-                  "uint32_t result2 = ((xc->readIntRegOperand(this, r2_src))"\
-                  " & mask(aarch64 ? 64 : 32)) ;\n"\
-                  " uint32_t dest2 = ((xc->readIntRegOperand(this, d2_src)) "\
-                  " & mask(aarch64 ? 64 : 32)) ;"
-                accCode += '''
-                     uint64_t data = dest2;
+                accCode = '''
+                     uint64_t data = XDest2;
                      data = isBigEndian64(xc->tcBase())
                           ? ((uint64_t(WDest_uw) << 32) | data)
                                  : ((data << 32) | WDest_uw);
                      valRs = cSwap(data, isBigEndian64(xc->tcBase()));
-                     uint64_t data2 = result2 ;
+                     uint64_t data2 = XResult2 ;
                      data2 = isBigEndian64(xc->tcBase())
                           ? ((uint64_t(Result_uw) << 32) | data2)
                                  : ((data2 << 32) | Result_uw);
@@ -336,21 +332,16 @@ let {{
                       " %(type)s c){ %(op)s });\n"
 
             elif self.size == 8:
-                accCode = ""\
-                  "uint64_t result2 = ((xc->readIntRegOperand(this, r2_src))"\
-                  " & mask(aarch64 ? 64 : 32)) ;\n"\
-                  " uint64_t dest2 = ((xc->readIntRegOperand(this, d2_src)) "\
-                  " & mask(aarch64 ? 64 : 32)) ;"
-                accCode += '''
+                accCode = '''
                    // This temporary needs to be here so that the parser
                    // will correctly identify this instruction as a store.
                    std::array<uint64_t, 2> temp;
                    temp[0] = cSwap(XDest_ud,isBigEndian64(xc->tcBase()));
-                   temp[1] = cSwap(dest2,isBigEndian64(xc->tcBase()));
+                   temp[1] = cSwap(XDest2,isBigEndian64(xc->tcBase()));
                    valRs = temp;
                    std::array<uint64_t, 2> temp2;
                    temp2[0] = cSwap(XResult_ud,isBigEndian64(xc->tcBase()));
-                   temp2[1] = cSwap(result2,isBigEndian64(xc->tcBase()));
+                   temp2[1] = cSwap(XResult2,isBigEndian64(xc->tcBase()));
                    Mem_tud = temp2;
                      '''
 

diff --git a/src/arch/arm/isa/operands.isa b/src/arch/arm/isa/operands.isa
index 134c51f80b2ab85e39468c660992b3fd05975bbd..0f18ffd0193b3d6fc09f6a1a50df6d77252835f8 100644 (file)
--- a/src/arch/arm/isa/operands.isa
+++ b/src/arch/arm/isa/operands.isa
@@ -1,5 +1,5 @@
 // -*- mode:c++ -*-
-// Copyright (c) 2010-2014, 2016-2018 ARM Limited
+// Copyright (c) 2010-2014, 2016-2018, 2021 ARM Limited
 // All rights reserved
 //
 // The license below extends only to copyright in the software and shall
@@ -200,6 +200,7 @@ def operands {{
     'IWDest2': intRegIWPC('dest2'),
     'Result': intReg('result'),
     'XResult': intRegX64('result'),
+    'XResult2': intRegX64('result2'),
     'XBase': intRegX64('base', id = srtBase),
     'Base': intRegAPC('base', id = srtBase),
     'XOffset': intRegX64('offset'),

diff --git a/src/arch/arm/isa/templates/mem64.isa b/src/arch/arm/isa/templates/mem64.isa
index f7034e684ddc216b2810e19499e18603b5bae66d..444560ab0fb988e982b0aa6976eebc7c96cdfc88 100644 (file)
--- a/src/arch/arm/isa/templates/mem64.isa
+++ b/src/arch/arm/isa/templates/mem64.isa
@@ -889,57 +889,6 @@ def template AmoOpConstructor {{
     }
 }};
 
-def template AmoPairOpDeclare {{
-    class %(class_name)s : public %(base_class)s
-    {
-      private:
-        %(reg_idx_arr_decl)s;
-
-      public:
-        uint32_t d2_src ;
-        uint32_t r2_src ;
-        uint32_t r2_dst ;
-        /// Constructor.
-        %(class_name)s(ExtMachInst machInst, IntRegIndex _dest,
-                       IntRegIndex _base, IntRegIndex _result);
-
-        Fault execute(ExecContext *, Trace::InstRecord *) const override;
-        Fault initiateAcc(ExecContext *, Trace::InstRecord *) const override;
-        Fault completeAcc(PacketPtr, ExecContext *,
-                          Trace::InstRecord *) const override;
-
-        void
-        annotateFault(ArmISA::ArmFault *fault) override
-        {
-            %(fa_code)s
-        }
-    };
-}};
-
-
-def template AmoPairOpConstructor {{
-    %(class_name)s::%(class_name)s(ExtMachInst machInst,
-            IntRegIndex _dest, IntRegIndex _base, IntRegIndex _result) :
-         %(base_class)s("%(mnemonic)s", machInst, %(op_class)s,
-                        _dest,  _base, _result)
-    {
-        %(set_reg_idx_arr)s;
-        %(constructor)s;
-
-        uint32_t d2 = RegId(IntRegClass, dest).index() + 1 ;
-        uint32_t r2 = RegId(IntRegClass, result).index() + 1 ;
-
-        d2_src = _numSrcRegs ;
-        setSrcRegIdx(_numSrcRegs++, RegId(IntRegClass, d2));
-        r2_src = _numSrcRegs ;
-        setSrcRegIdx(_numSrcRegs++, RegId(IntRegClass, r2));
-        r2_dst = _numDestRegs ;
-        setDestRegIdx(_numDestRegs++, RegId(IntRegClass, r2));
-        flags[IsStore] = false;
-        flags[IsLoad] = false;
-    }
-}};
-
 def template AmoArithmeticOpDeclare {{
     class %(class_name)s : public %(base_class)s
     {