Gabe Black has uploaded this change for review. (
https://gem5-review.googlesource.com/c/public/gem5/+/50253 )
Change subject: cpu: Simplify and revamp the InstResult class.
......................................................................
cpu: Simplify and revamp the InstResult class.
The InstResult class is always used to store a register value, and also
only used to store a RegVal and not any more complex type like a
VecRegContainer. This is partially because the methods that *would*
store a complex result only have a pointer to work with, and don't have
a type to cast to to store the result in the InstResult.
This change reworks the InstResult class to hold the RegClass the
register goes with, and also either a standard RegVal, or a pointer to a
blob of memory holding the actual value if RegVal isn't appropriate. The
InstResult type now also has a "flags" member which keeps track of whether
it is valid in general, and also if it stores a RegVal or a blob of bytes.
To make working with InstResult easier, it also now has an "asString"
method which will just call into the RegClass's valString method with
the appropriate pointer.
By removing the ultimately unnecessary generality of the original class,
this change also simplifies InstResult significantly.
Change-Id: I71ace4da6c99b5dd82757e5365c493d795496fe5
---
M src/cpu/checker/cpu.hh
M src/cpu/checker/cpu_impl.hh
M src/cpu/inst_res.hh
M src/cpu/o3/dyn_inst.hh
4 files changed, 150 insertions(+), 159 deletions(-)
diff --git a/src/cpu/checker/cpu.hh b/src/cpu/checker/cpu.hh
index 273dc5e..7864473 100644
--- a/src/cpu/checker/cpu.hh
+++ b/src/cpu/checker/cpu.hh
@@ -194,15 +194,17 @@
void
setRegOperand(const StaticInst *si, int idx, RegVal val) override
{
- thread->setReg(si->destRegIdx(idx), val);
- result.emplace(val);
+ RegId id = si->destRegIdx(idx);
+ thread->setReg(id, val);
+ result.emplace(id.regClass(), val);
}
void
setRegOperand(const StaticInst *si, int idx, const void *val) override
{
- thread->setReg(si->destRegIdx(idx), val);
- //TODO setVecResult, setVecPredResult setVecElemResult?
+ RegId id = si->destRegIdx(idx);
+ thread->setReg(id, val);
+ result.emplace(id.regClass(), val);
}
bool readPredicate() const override { return thread->readPredicate(); }
diff --git a/src/cpu/checker/cpu_impl.hh b/src/cpu/checker/cpu_impl.hh
index 83b0a07..7486c9b 100644
--- a/src/cpu/checker/cpu_impl.hh
+++ b/src/cpu/checker/cpu_impl.hh
@@ -467,24 +467,22 @@
InstResult inst_val;
int idx = -1;
bool result_mismatch = false;
- bool scalar_mismatch = false;
if (inst->isUnverifiable()) {
// Unverifiable instructions assume they were executed
// properly by the CPU. Grab the result from the
// instruction and write it to the register.
- copyResult(inst, InstResult((RegVal)0), idx);
+ copyResult(inst, InstResult(), idx);
} else if (inst->numDestRegs() > 0 && !result.empty()) {
DPRINTF(Checker, "Dest regs %d, number of checker dest regs %d\n",
inst->numDestRegs(), result.size());
for (int i = 0; i < inst->numDestRegs() && !result.empty(); i++) {
checker_val = result.front();
result.pop();
- inst_val = inst->popResult(InstResult((RegVal)0));
+ inst_val = inst->popResult();
if (checker_val != inst_val) {
result_mismatch = true;
idx = i;
- scalar_mismatch = checker_val.is<RegVal>();
}
}
} // Checker CPU checks all the saved results in the dyninst passed by
@@ -494,12 +492,9 @@
// this is ok and not a bug. May be worthwhile to try and correct
this.
if (result_mismatch) {
- if (scalar_mismatch) {
- warn("%lli: Instruction results (%i) do not match! (Values may"
- " not actually be integers) Inst: %#x, checker: %#x",
- curTick(), idx, inst_val.asNoAssert<RegVal>(),
- checker_val.as<RegVal>());
- }
+ warn("%lli: Instruction results (%i) do not match! Inst: %s, "
+ "checker: %s",
+ curTick(), idx, inst_val.asString(), checker_val.asString());
// It's useful to verify load values from memory, but in MP
// systems the value obtained at execute may be different than
@@ -581,56 +576,30 @@
// so do the fix-up then start with the next dest reg;
if (start_idx >= 0) {
const RegId& idx = inst->destRegIdx(start_idx);
- switch (idx.classValue()) {
- case InvalidRegClass:
- break;
- case IntRegClass:
- case FloatRegClass:
- case VecElemClass:
- case CCRegClass:
- thread->setReg(idx, mismatch_val.as<RegVal>());
- break;
- case VecRegClass:
- {
- auto val = mismatch_val.as<TheISA::VecRegContainer>();
- thread->setReg(idx, &val);
- }
- break;
- case MiscRegClass:
- thread->setMiscReg(idx.index(), mismatch_val.as<RegVal>());
- break;
- default:
- panic("Unknown register class: %d", (int)idx.classValue());
- }
+
+ if (idx.classValue() == InvalidRegClass)
+ ; // Do nothing.
+ else if (idx.classValue() == MiscRegClass)
+ thread->setMiscReg(idx.index(), mismatch_val.asRegVal());
+ else if (mismatch_val.isBlob())
+ thread->setReg(idx, mismatch_val.asBlob());
+ else
+ thread->setReg(idx, mismatch_val.asRegVal());
}
start_idx++;
InstResult res;
for (int i = start_idx; i < inst->numDestRegs(); i++) {
const RegId& idx = inst->destRegIdx(i);
res = inst->popResult();
- switch (idx.classValue()) {
- case InvalidRegClass:
- break;
- case IntRegClass:
- case FloatRegClass:
- case VecElemClass:
- case CCRegClass:
- thread->setReg(idx, res.as<RegVal>());
- break;
- case VecRegClass:
- {
- auto val = res.as<TheISA::VecRegContainer>();
- thread->setReg(idx, &val);
- }
- break;
- case MiscRegClass:
- // Try to get the proper misc register index for ARM here...
- thread->setMiscReg(idx.index(), 0);
- break;
- // else Register is out of range...
- default:
- panic("Unknown register class: %d", (int)idx.classValue());
- }
+
+ if (idx.classValue() == InvalidRegClass)
+ ; // Do nothing.
+ else if (idx.classValue() == MiscRegClass)
+ thread->setMiscReg(idx.index(), mismatch_val.asRegVal());
+ else if (mismatch_val.isBlob())
+ thread->setReg(idx, mismatch_val.asBlob());
+ else
+ thread->setReg(idx, mismatch_val.asRegVal());
}
}
diff --git a/src/cpu/inst_res.hh b/src/cpu/inst_res.hh
index fd84cb6..6df13ba 100644
--- a/src/cpu/inst_res.hh
+++ b/src/cpu/inst_res.hh
@@ -38,11 +38,13 @@
#ifndef __CPU_INST_RES_HH__
#define __CPU_INST_RES_HH__
-#include <any>
-#include <type_traits>
+#include <cstdint>
+#include <cstring>
+#include <string>
#include "base/logging.hh"
#include "base/types.hh"
+#include "cpu/reg_class.hh"
namespace gem5
{
@@ -50,65 +52,97 @@
class InstResult
{
private:
- std::any result;
- std::function<bool(const std::any &a, const std::any &b)> equals;
+ enum
+ {
+ Valid = 0x1,
+ Blob = 0x2
+ };
+ uint8_t flags = 0;
+
+ bool valid() const { return flags & Valid; }
+ void
+ valid(bool val)
+ {
+ if (val)
+ flags |= Valid;
+ else
+ flags &= ~Valid;
+ }
+
+ bool blob() const { return flags & Blob; }
+ void
+ blob(bool val)
+ {
+ if (val)
+ flags |= Blob;
+ else
+ flags &= ~Blob;
+ }
+
+ const RegClass *_regClass = nullptr;
+ union Value
+ {
+ RegVal reg;
+ const uint8_t *bytes = nullptr;
+ } value;
+
+ void
+ setBytes(const void *val)
+ {
+ const size_t size = _regClass->regBytes();
+ if (blob())
+ delete[] value.bytes;
+ uint8_t *temp = new uint8_t[size];
+ std::memcpy(temp, val, size);
+ value.bytes = temp;
+ blob(true);
+ }
+
+ void
+ setRegVal(RegVal val)
+ {
+ if (blob())
+ delete[] value.bytes;
+ blob(false);
+ value.reg = val;
+ }
public:
/** Default constructor creates an invalid result. */
- InstResult() :
- // This InstResult is empty, and will only equal other InstResults
- // which are also empty.
- equals([](const std::any &a, const std::any &b) -> bool {
- gem5_assert(!a.has_value());
- return !b.has_value();
- })
- {}
+ InstResult() {}
InstResult(const InstResult &) = default;
- template <typename T>
- explicit InstResult(T val) : result(val),
-
- // Set equals so it knows how to compare results of type T.
- equals([](const std::any &a, const std::any &b) -> bool {
- // If one has a value but the other doesn't, not equal.
- if (a.has_value() != b.has_value())
- return false;
- // If they are both empty, equal.
- if (!a.has_value())
- return true;
- // At least the local object should be of the right type.
- gem5_assert(a.type() == typeid(T));
- // If these aren't the same type, not equal.
- if (a.type() != b.type())
- return false;
- // We now know these both hold a result of the right type.
- return std::any_cast<const T&>(a) == std::any_cast<const
T&>(b);
- })
+ InstResult(const RegClass ®_class, RegVal val) :
_regClass(®_class)
{
- static_assert(!std::is_pointer_v<T>,
- "InstResult shouldn't point to external data.");
- // Floating point values should be converted to/from ints using
- // floatToBits and bitsToFloat, and not stored in InstResult
directly.
- static_assert(!std::is_floating_point_v<T>,
- "Floating point values should be converted to/from ints.");
+ valid(true);
+ setRegVal(val);
+ }
+ InstResult(const RegClass ®_class, const void *val) :
+ _regClass(®_class)
+ {
+ valid(true);
+ setBytes(val);
}
- // Convert floating point values to integers.
- template <typename T,
- std::enable_if_t<std::is_floating_point_v<T>, int> = 0>
- explicit InstResult(T val) : InstResult(floatToBits(val)) {}
-
- // Convert all integer types to RegVal.
- template <typename T,
- std::enable_if_t<std::is_integral_v<T> && !std::is_same_v<T,
RegVal>,
- int> = 0>
- explicit InstResult(T val) : InstResult(static_cast<RegVal>(val)) {}
+ virtual ~InstResult()
+ {
+ if (blob())
+ delete[] value.bytes;
+ }
InstResult &
operator=(const InstResult& that)
{
- result = that.result;
- equals = that.equals;
+ valid(that.valid());
+ _regClass = that._regClass;
+
+ if (valid()) {
+ if (that.blob())
+ setBytes(that.value.bytes);
+ else
+ setRegVal(that.value.reg);
+ }
+
return *this;
}
@@ -119,7 +153,21 @@
bool
operator==(const InstResult& that) const
{
- return equals(result, that.result);
+ // If we're not valid, they're equal to us iff they are also not
valid.
+ if (!valid())
+ return !that.valid();
+
+ // If both are valid, check if th register class and flags.
+ if (_regClass != that._regClass || flags != that.flags)
+ return false;
+
+ // Check our data based on whether we store a blob or a RegVal.
+ if (blob()) {
+ return std::memcmp(value.bytes, that.value.bytes,
+ _regClass->regBytes()) == 0;
+ } else {
+ return value.reg == that.value.reg;
+ }
}
bool
@@ -128,61 +176,32 @@
return !operator==(that);
}
- /** Checks */
- /** @{ */
+ const RegClass ®Class() const { return *_regClass; }
+ bool isValid() const { return valid(); }
+ bool isBlob() const { return blob(); }
- template <typename T>
- bool
- is() const
+ RegVal
+ asRegVal() const
{
- static_assert(!std::is_floating_point_v<T>,
- "Floating point values should be converted to/from ints.");
- return result.type() == typeid(T);
+ assert(!blob());
+ return value.reg;
}
- template <typename T>
- std::enable_if_t<std::is_integral_v<T> && !std::is_same_v<T, RegVal>,
bool>
- is() const
+ const void *
+ asBlob() const
{
- return is<RegVal>();
+ assert(blob());
+ return value.bytes;
}
- /** Is this a valid result?. */
- bool isValid() const { return result.has_value(); }
- /** @} */
-
- /** Explicit cast-like operations. */
- /** @{ */
- template <typename T>
- T
- as() const
+ std::string
+ asString() const
{
- assert(is<T>());
- return std::any_cast<T>(result);
+ if (blob())
+ return _regClass->valString(value.bytes);
+ else
+ return _regClass->valString(&value.reg);
}
-
- template <typename T>
- std::enable_if_t<std::is_integral_v<T> && !std::is_same_v<T, RegVal>,
- RegVal>
- as() const
- {
- return as<RegVal>();
- }
-
- /** Cast to integer without checking type.
- * This is required to have the o3 cpu checker happy, as it
- * compares results as integers without being fully aware of
- * their nature. */
- template <typename T>
- T
- asNoAssert() const
- {
- if (!is<T>())
- return T{};
- return as<T>();
- }
-
- /** @} */
};
} // namespace gem5
diff --git a/src/cpu/o3/dyn_inst.hh b/src/cpu/o3/dyn_inst.hh
index bd5aed3..277555e 100644
--- a/src/cpu/o3/dyn_inst.hh
+++ b/src/cpu/o3/dyn_inst.hh
@@ -762,10 +762,10 @@
/** @{ */
template<typename T>
void
- setResult(T &&t)
+ setResult(const RegClass ®_class, T &&t)
{
if (instFlags[RecordResult]) {
- instResult.emplace(std::forward<T>(t));
+ instResult.emplace(reg_class, std::forward<T>(t));
}
}
/** @} */
@@ -1190,14 +1190,15 @@
if (reg->is(InvalidRegClass))
return;
cpu->setReg(reg, val);
- setResult(val);
+ setResult(reg->regClass(), val);
}
void
setRegOperand(const StaticInst *si, int idx, const void *val) override
{
- cpu->setReg(regs.renamedDestIdx(idx), val);
- //TODO setResult
+ const PhysRegIdPtr reg = regs.renamedDestIdx(idx);
+ cpu->setReg(reg, val);
+ setResult(reg->regClass(), val);
}
};
--
To view, visit https://gem5-review.googlesource.com/c/public/gem5/+/50253
To unsubscribe, or for help writing mail filters, visit
https://gem5-review.googlesource.com/settings
Gerrit-Project: public/gem5
Gerrit-Branch: develop
Gerrit-Change-Id: I71ace4da6c99b5dd82757e5365c493d795496fe5
Gerrit-Change-Number: 50253
Gerrit-PatchSet: 1
Gerrit-Owner: Gabe Black <gabe.bl...@gmail.com>
Gerrit-MessageType: newchange
_______________________________________________
gem5-dev mailing list -- gem5-dev@gem5.org
To unsubscribe send an email to gem5-dev-le...@gem5.org
%(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
