[gem5-dev] [M] Change in gem5/gem5[develop]: dev: Add an offset checking mechanism to RegisterBank.
Gabe Black has submitted this change. (
https://gem5-review.googlesource.com/c/public/gem5/+/66431?usp=email )
Change subject: dev: Add an offset checking mechanism to RegisterBank.
..
dev: Add an offset checking mechanism to RegisterBank.
When adding a long list of registers, it can be easy to miss one which
will offset all the registers after it. It can be hard to find those
sorts of problems, and tedious and error prone to fix them.
This change adds a mechanism to simply annotate what offset a register
should have. That should also make the register list more self
documenting, since you'll be able to easily see what offset a register
has from the source without having to count up everything in front of it.
Change-Id: Ia7e419ffb062a64a10106305f875cec6f9fe9a80
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/66431
Reviewed-by: Yu-hsin Wang
Maintainer: Gabe Black
Tested-by: kokoro
---
M src/dev/reg_bank.hh
M src/dev/reg_bank.test.cc
2 files changed, 120 insertions(+), 11 deletions(-)
Approvals:
Yu-hsin Wang: Looks good to me, approved
Gabe Black: Looks good to me, approved
kokoro: Regressions pass
diff --git a/src/dev/reg_bank.hh b/src/dev/reg_bank.hh
index 42af7bc..31c0ce5 100644
--- a/src/dev/reg_bank.hh
+++ b/src/dev/reg_bank.hh
@@ -37,6 +37,7 @@
#include
#include
#include
+#include
#include
#include
@@ -84,6 +85,9 @@
* entire device, with the address from accesses passed into read or write
* unmodified.
*
+ * The base(), size() and name() methods can be used to access each of
those
+ * read only properties of the RegisterBank instance.
+ *
* To add actual registers to the RegisterBank (discussed below), you can
use
* either the addRegister method which adds a single register, or
addRegisters
* which adds an initializer list of them all at once. The register will be
@@ -91,8 +95,19 @@
* existing registers. The size of the bank is automatically accumulated as
* registers are added.
*
- * The base(), size() and name() methods can be used to access each of
those
- * read only properties of the RegisterBank instance.
+ * When adding a lot of registers, you might accidentally add an extra,
+ * or accidentally skip one in a long list. Because the offset is handled
+ * automatically, some of your registers might end up shifted higher or
lower
+ * than you expect. To help mitigate this, you can set what offset you
expect
+ * a register to have by specifying it as an offset, register pair.
+ *
+ * addRegisters({{0x1000, reg0}, reg1, reg2});
+ *
+ * If the register would end up at a different offset, gem5 will panic. You
+ * can also leave off the register if you want to just check the offset,
for
+ * instance between groups of registers.
+ *
+ * addRegisters({reg0, reg1, reg2, 0x100c})
*
* While the RegisterBank itself doesn't have any data in it directly and
so
* has no endianness, it's very likely all the registers within it will
have
@@ -805,19 +820,52 @@
virtual ~RegisterBank() {}
-void
-addRegisters(
-std::initializer_list>
regs)
+class RegisterAdder
{
-panic_if(regs.size() == 0, "Adding an empty list of registers
to %s?",
- name());
-for (auto : regs) {
-_offsetMap.emplace(_base + _size, reg);
-_size += reg.get().size();
+ private:
+std::optional offset;
+std::optional reg;
+
+ public:
+// Nothing special to do for this register.
+RegisterAdder(RegisterBase _reg) : reg(_reg) {}
+// Ensure that this register is added at a particular offset.
+RegisterAdder(Addr new_offset, RegisterBase _reg) :
+offset(new_offset), reg(_reg)
+{}
+// No register, just check that the offset is what we expect.
+RegisterAdder(Addr new_offset) : offset(new_offset) {}
+
+friend class RegisterBank;
+};
+
+void
+addRegisters(std::initializer_list adders)
+{
+panic_if(std::empty(adders),
+"Adding an empty list of registers to %s?", name());
+for (auto : adders) {
+const Addr offset = _base + _size;
+
+if (adder.reg) {
+auto *reg = adder.reg.value();
+if (adder.offset && adder.offset.value() != offset) {
+panic(
+"Expected offset of register %s.%s to be %#x,
is %#x.",
+name(), reg->name(), adder.offset.value(), offset);
+}
+_offsetMap.emplace(offset, *reg);
+_size += reg->size();
+} else if (adder.offset) {
+if (adder.offset.value() != offset) {
+panic("Expected current offset of %s to be %#x,
is %#x.",
+name(), adder.offset.value(), offset);
+}
+}
[gem5-dev] [M] Change in gem5/gem5[develop]: dev: Add an offset checking mechanism to RegisterBank.
Gabe Black has uploaded this change for review. (
https://gem5-review.googlesource.com/c/public/gem5/+/66431?usp=email )
Change subject: dev: Add an offset checking mechanism to RegisterBank.
..
dev: Add an offset checking mechanism to RegisterBank.
When adding a long list of registers, it can be easy to miss one which
will offset all the registers after it. It can be hard to find those
sorts of problems, and tedious and error prone to fix them.
This change adds a mechanism to simply annotate what offset a register
should have. That should also make the register list more self
documenting, since you'll be able to easily see what offset a register
has from the source without having to count up everything in front of it.
Change-Id: Ia7e419ffb062a64a10106305f875cec6f9fe9a80
---
M src/dev/reg_bank.hh
M src/dev/reg_bank.test.cc
2 files changed, 117 insertions(+), 11 deletions(-)
diff --git a/src/dev/reg_bank.hh b/src/dev/reg_bank.hh
index 42af7bc..6348597 100644
--- a/src/dev/reg_bank.hh
+++ b/src/dev/reg_bank.hh
@@ -84,6 +84,9 @@
* entire device, with the address from accesses passed into read or write
* unmodified.
*
+ * The base(), size() and name() methods can be used to access each of
those
+ * read only properties of the RegisterBank instance.
+ *
* To add actual registers to the RegisterBank (discussed below), you can
use
* either the addRegister method which adds a single register, or
addRegisters
* which adds an initializer list of them all at once. The register will be
@@ -91,8 +94,19 @@
* existing registers. The size of the bank is automatically accumulated as
* registers are added.
*
- * The base(), size() and name() methods can be used to access each of
those
- * read only properties of the RegisterBank instance.
+ * When adding a lot of registers, you might accidentally add an extra,
+ * or accidentally skip one in a long list. Because the offset is handled
+ * automatically, some of your registers might end up shifted higher or
lower
+ * than you expect. To help mitigate this, you can set what offset you
expect
+ * a register to have by specifying it as an offset, register pair.
+ *
+ * addRegisters({{0x1000, reg0}, reg1, reg2});
+ *
+ * If the register would end up at a different offset, gem5 will panic. You
+ * can also leave off the register if you want to just check the offset,
for
+ * instance between groups of registers.
+ *
+ * addRegisters({reg0, reg1, reg2, 0x100c})
*
* While the RegisterBank itself doesn't have any data in it directly and
so
* has no endianness, it's very likely all the registers within it will
have
@@ -805,19 +819,54 @@
virtual ~RegisterBank() {}
-void
-addRegisters(
-std::initializer_list>
regs)
+class RegisterAdder
{
-panic_if(regs.size() == 0, "Adding an empty list of registers
to %s?",
- name());
-for (auto : regs) {
-_offsetMap.emplace(_base + _size, reg);
-_size += reg.get().size();
+ private:
+bool checkOffset = false;
+Addr offset = 0;
+RegisterBase *reg = nullptr;
+
+ public:
+// Nothing special to do for this register.
+RegisterAdder(RegisterBase _reg) : reg(_reg) {}
+// Ensure that this register is added at a particular offset.
+RegisterAdder(Addr new_offset, RegisterBase _reg) :
+checkOffset(true), offset(new_offset), reg(_reg)
+{}
+// No register, just check that the offset is what we expect.
+RegisterAdder(Addr new_offset) : checkOffset(true),
offset(new_offset)
+{}
+
+friend class RegisterBank;
+};
+
+void
+addRegisters(std::initializer_list adders)
+{
+panic_if(adders.size() == 0,
+"Adding an empty list of registers to %s?", name());
+for (auto : adders) {
+const Addr offset = _base + _size;
+auto *reg = adder.reg;
+
+if (reg) {
+if (adder.checkOffset && adder.offset != offset) {
+panic(
+"Expected offset of register %s.%s to be %#x,
is %#x.",
+name(), reg->name(), adder.offset, offset);
+}
+_offsetMap.emplace(offset, *reg);
+_size += reg->size();
+} else if (adder.checkOffset) {
+if (adder.offset != offset) {
+panic("Expected current offset of %s to be %#x,
is %#x.",
+name(), adder.offset, offset);
+}
+}
}
}
-void addRegister(RegisterBase ) { addRegisters({reg}); }
+void addRegister(RegisterAdder reg) { addRegisters({reg}); }
Addr base() const { return _base; }
Addr size() const { return _size; }
diff --git a/src/dev/reg_bank.test.cc b/src/dev/reg_bank.test.cc
