[gem5-dev] Change in gem5/gem5[develop]: arch-arm: Fix Compare and Swap Pair instructions
Giacomo Travaglini has submitted this change. (
https://gem5-review.googlesource.com/c/public/gem5/+/39456 )
Change subject: arch-arm: Fix Compare and Swap Pair instructions
..
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
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/39456
Reviewed-by: Richard Cooper
Reviewed-by: Gabe Black
Tested-by: kokoro
---
M src/arch/arm/insts/mem64.cc
M src/arch/arm/insts/mem64.hh
M src/arch/arm/isa/insts/amo64.isa
M src/arch/arm/isa/operands.isa
M src/arch/arm/isa/templates/mem64.isa
5 files changed, 57 insertions(+), 76 deletions(-)
Approvals:
Gabe Black: Looks good to me, approved
Richard Cooper: Looks good to me, but someone else must approve
Giacomo Travaglini: Looks good to me, approved
kokoro: Regressions pass
diff --git a/src/arch/arm/insts/mem64.cc b/src/arch/arm/insts/mem64.cc
index a12c330..ed159be 100644
--- 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 @@
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 412efb0..b602d54 100644
--- 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 @@
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 51e1f38..27ee9b5 100644
--- 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 @@
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 @@
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 = 'R
[gem5-dev] Change in gem5/gem5[develop]: arch-arm: Fix Compare and Swap Pair instructions
Giacomo Travaglini has uploaded this change for review. (
https://gem5-review.googlesource.com/c/public/gem5/+/39456 )
Change subject: arch-arm: Fix Compare and Swap Pair instructions
..
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
---
M src/arch/arm/insts/mem64.cc
M src/arch/arm/insts/mem64.hh
M src/arch/arm/isa/insts/amo64.isa
M src/arch/arm/isa/operands.isa
M src/arch/arm/isa/templates/mem64.isa
5 files changed, 57 insertions(+), 76 deletions(-)
diff --git a/src/arch/arm/insts/mem64.cc b/src/arch/arm/insts/mem64.cc
index a12c330..ed159be 100644
--- 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 @@
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 412efb0..b602d54 100644
--- 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 @@
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 51e1f38..27ee9b5 100644
--- 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 @@
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 @@
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 @@
uint32_t result2 = isBigEndian64(xc->tcBase())
? (uint32_t)data
: (data >> 32);
-xc->setIntRegOperand(this, r2_dst, (result2) &
-mask(aarc
