Gabe Black has uploaded this change for review. (
https://gem5-review.googlesource.com/c/public/gem5/+/50344 )
Change subject: misc: Use AddrRangeList more consistently in the AddrRange
class.
......................................................................
misc: Use AddrRangeList more consistently in the AddrRange class.
We go through the trouble of defining an AddrRangeList typedef, but then
we don't use it consistently and use std::vector<AddrRange> instead.
This change converts the exclude method from using
std::vector<AddrRange> to AddrRangeList, and also adds a constructor
which takes an AddrRangeList.
Because there is a lot of code which uses the std::vector based
constructor, this change does not remove that method.
Change-Id: I1a03b25990025688aa760a67d3e7a2e8141384ce
---
M src/base/addr_range.hh
M src/base/addr_range.test.cc
M src/python/m5/SimObject.py
M src/python/m5/params.py
M src/python/pybind11/core.cc
5 files changed, 88 insertions(+), 85 deletions(-)
diff --git a/src/base/addr_range.hh b/src/base/addr_range.hh
index 291d376..90d9279 100644
--- a/src/base/addr_range.hh
+++ b/src/base/addr_range.hh
@@ -42,6 +42,7 @@
#define __BASE_ADDR_RANGE_HH__
#include <algorithm>
+#include <iterator>
#include <list>
#include <vector>
@@ -53,6 +54,15 @@
namespace gem5
{
+class AddrRange;
+
+/**
+ * Convenience typedef for a collection of address ranges
+ *
+ * @ingroup api_addr_range
+ */
+typedef std::list<AddrRange> AddrRangeList;
+
/**
* The AddrRange class encapsulates an address range, and supports a
* number of tests to check if two ranges intersect, if a range
@@ -89,6 +99,46 @@
/** The value to compare sel with. */
uint8_t intlvMatch;
+ protected:
+ struct Dummy {};
+
+ template <class Iterator>
+ AddrRange(Dummy, Iterator begin_it, Iterator end_it)
+ : _start(1), _end(0), intlvMatch(0)
+ {
+ if (begin_it != end_it) {
+ // get the values from the first one and check the others
+ _start = begin_it->_start;
+ _end = begin_it->_end;
+ masks = begin_it->masks;
+ intlvMatch = begin_it->intlvMatch;
+ }
+
+ auto count = std::distance(begin_it, end_it);
+ // either merge if got all ranges or keep this equal to the single
+ // interleaved range
+ if (count > 1) {
+ fatal_if(count != (1ULL << masks.size()),
+ "Got %d ranges spanning %d interleaving bits.",
+ count, masks.size());
+
+ uint8_t match = 0;
+ for (auto it = begin_it; it != end_it; it++) {
+ fatal_if(!mergesWith(*it),
+ "Can only merge ranges with the same start, end "
+ "and interleaving bits, %s %s.", to_string(),
+ it->to_string());
+
+ fatal_if(it->intlvMatch != match,
+ "Expected interleave match %d but got %d when "
+ "merging.", match, it->intlvMatch);
+ ++match;
+ }
+ masks.clear();
+ intlvMatch = 0;
+ }
+ }
+
public:
/**
@@ -215,40 +265,12 @@
*
* @ingroup api_addr_range
*/
- AddrRange(const std::vector<AddrRange>& ranges)
- : _start(1), _end(0), intlvMatch(0)
- {
- if (!ranges.empty()) {
- // get the values from the first one and check the others
- _start = ranges.front()._start;
- _end = ranges.front()._end;
- masks = ranges.front().masks;
- intlvMatch = ranges.front().intlvMatch;
- }
- // either merge if got all ranges or keep this equal to the single
- // interleaved range
- if (ranges.size() > 1) {
-
- if (ranges.size() != (1ULL << masks.size()))
- fatal("Got %d ranges spanning %d interleaving bits\n",
- ranges.size(), masks.size());
-
- uint8_t match = 0;
- for (const auto& r : ranges) {
- if (!mergesWith(r))
- fatal("Can only merge ranges with the same start, end "
- "and interleaving bits, %s %s\n", to_string(),
- r.to_string());
-
- if (r.intlvMatch != match)
- fatal("Expected interleave match %d but got %d when "
- "merging\n", match, r.intlvMatch);
- ++match;
- }
- masks.clear();
- intlvMatch = 0;
- }
- }
+ AddrRange(std::vector<AddrRange> ranges)
+ : AddrRange(Dummy{}, ranges.begin(), ranges.end())
+ {}
+ AddrRange(std::list<AddrRange> ranges)
+ : AddrRange(Dummy{}, ranges.begin(), ranges.end())
+ {}
/**
* Determine if the range is interleaved or not.
@@ -599,15 +621,15 @@
*
* @ingroup api_addr_range
*/
- std::vector<AddrRange>
- exclude(const std::vector<AddrRange> &exclude_ranges) const
+ AddrRangeList
+ exclude(const AddrRangeList &exclude_ranges) const
{
assert(!interleaved());
auto sorted_ranges = exclude_ranges;
- std::sort(sorted_ranges.begin(), sorted_ranges.end());
+ sorted_ranges.sort();
- std::vector<AddrRange> ranges;
+ std::list<AddrRange> ranges;
Addr next_start = start();
for (const auto &e : sorted_ranges) {
@@ -690,13 +712,6 @@
};
/**
- * Convenience typedef for a collection of address ranges
- *
- * @ingroup api_addr_range
- */
-typedef std::list<AddrRange> AddrRangeList;
-
-/**
* @ingroup api_addr_range
*/
inline AddrRange
diff --git a/src/base/addr_range.test.cc b/src/base/addr_range.test.cc
index 0f30ad5..ada9d3f 100644
--- a/src/base/addr_range.test.cc
+++ b/src/base/addr_range.test.cc
@@ -36,6 +36,7 @@
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/
+#include <gmock/gmock.h>
#include <gtest/gtest.h>
#include <cmath>
@@ -45,6 +46,8 @@
using namespace gem5;
+using testing::ElementsAre;
+
TEST(AddrRangeTest, ValidRange)
{
AddrRange r;
@@ -921,10 +924,10 @@
}
/*
- * The AddrRange(std::vector<AddrRange>) constructor "merges" the
interleaving
+ * The AddrRange(AddrRangeList) constructor "merges" the interleaving
* address ranges. It should be noted that this constructor simply checks
that
* these interleaving addresses can be merged then creates a new address
from
- * the start and end addresses of the first address range in the vector.
+ * the start and end addresses of the first address range in the list.
*/
TEST(AddrRangeTest, MergingInterleavingAddressRanges)
{
@@ -942,7 +945,7 @@
uint8_t intlv_match2 = 1;
AddrRange r2(start2, end2, masks2, intlv_match2);
- std::vector<AddrRange> to_merge;
+ AddrRangeList to_merge;
to_merge.push_back(r1);
to_merge.push_back(r2);
@@ -956,7 +959,7 @@
TEST(AddrRangeTest, MergingInterleavingAddressRangesOneRange)
{
/*
- * In the case where there is just one range in the vector, the merged
+ * In the case where there is just one range in the list, the merged
* address range is equal to that range.
*/
Addr start = 0x0000;
@@ -966,7 +969,7 @@
uint8_t intlv_match = 0;
AddrRange r(start, end, masks, intlv_match);
- std::vector<AddrRange> to_merge;
+ AddrRangeList to_merge;
to_merge.push_back(r);
AddrRange output(to_merge);
@@ -1105,7 +1108,7 @@
*/
TEST(AddrRangeTest, ExcludeAll)
{
- const std::vector<AddrRange> exclude_ranges{
+ const AddrRangeList exclude_ranges{
AddrRange(0x0, 0x200)
};
@@ -1129,7 +1132,7 @@
*/
TEST(AddrRangeTest, ExcludeAllEqual)
{
- const std::vector<AddrRange> exclude_ranges{
+ const AddrRangeList exclude_ranges{
AddrRange(0x0, 0x100)
};
@@ -1153,7 +1156,7 @@
*/
TEST(AddrRangeTest, ExcludeAllMultiple)
{
- const std::vector<AddrRange> exclude_ranges{
+ const AddrRangeList exclude_ranges{
AddrRange(0x0, 0x30),
AddrRange(0x30, 0x40),
AddrRange(0x40, 0x120)
@@ -1183,7 +1186,7 @@
*/
TEST(AddrRangeTest, ExcludeAllOverlapping)
{
- const std::vector<AddrRange> exclude_ranges{
+ const AddrRangeList exclude_ranges{
AddrRange(0x0, 0x150),
AddrRange(0x140, 0x220)
};
@@ -1206,7 +1209,7 @@
*/
TEST(AddrRangeTest, ExcludeEmpty)
{
- const std::vector<AddrRange> exclude_ranges;
+ const AddrRangeList exclude_ranges;
AddrRange r(0x00, 0x100);
auto ranges = r.exclude(exclude_ranges);
@@ -1230,7 +1233,7 @@
*/
TEST(AddrRangeTest, NoExclusion)
{
- const std::vector<AddrRange> exclude_ranges{
+ const AddrRangeList exclude_ranges{
AddrRange(0x100, 0x200)
};
@@ -1257,7 +1260,7 @@
*/
TEST(AddrRangeTest, DoubleExclusion)
{
- const std::vector<AddrRange> exclude_ranges{
+ const AddrRangeList exclude_ranges{
AddrRange(0x000, 0x130),
AddrRange(0x140, 0x170),
};
@@ -1269,8 +1272,7 @@
auto ranges = r.exclude(exclude_ranges);
EXPECT_EQ(ranges.size(), 2);
- EXPECT_EQ(ranges[0], expected_range1);
- EXPECT_EQ(ranges[1], expected_range2);
+ EXPECT_THAT(ranges, ElementsAre(expected_range1, expected_range2));
}
/*
@@ -1289,7 +1291,7 @@
*/
TEST(AddrRangeTest, MultipleExclusion)
{
- const std::vector<AddrRange> exclude_ranges{
+ const AddrRangeList exclude_ranges{
AddrRange(0x000, 0x130),
AddrRange(0x140, 0x170),
AddrRange(0x180, 0x210)
@@ -1302,8 +1304,7 @@
auto ranges = r.exclude(exclude_ranges);
EXPECT_EQ(ranges.size(), 2);
- EXPECT_EQ(ranges[0], expected_range1);
- EXPECT_EQ(ranges[1], expected_range2);
+ EXPECT_THAT(ranges, ElementsAre(expected_range1, expected_range2));
}
/*
@@ -1324,7 +1325,7 @@
*/
TEST(AddrRangeTest, MultipleExclusionOverlapping)
{
- const std::vector<AddrRange> exclude_ranges{
+ const AddrRangeList exclude_ranges{
AddrRange(0x000, 0x130),
AddrRange(0x140, 0x170),
AddrRange(0x150, 0x210)
@@ -1336,7 +1337,7 @@
auto ranges = r.exclude(exclude_ranges);
EXPECT_EQ(ranges.size(), 1);
- EXPECT_EQ(ranges[0], expected_range1);
+ EXPECT_THAT(ranges, ElementsAre(expected_range1));
}
/*
@@ -1359,7 +1360,7 @@
*/
TEST(AddrRangeTest, ExclusionOverlapping)
{
- const std::vector<AddrRange> exclude_ranges{
+ const AddrRangeList exclude_ranges{
AddrRange(0x120, 0x180),
AddrRange(0x130, 0x170)
};
@@ -1371,8 +1372,7 @@
auto ranges = r.exclude(exclude_ranges);
EXPECT_EQ(ranges.size(), 2);
- EXPECT_EQ(ranges[0], expected_range1);
- EXPECT_EQ(ranges[1], expected_range2);
+ EXPECT_THAT(ranges, ElementsAre(expected_range1, expected_range2));
}
/*
@@ -1393,7 +1393,7 @@
*/
TEST(AddrRangeTest, MultipleExclusionUnsorted)
{
- const std::vector<AddrRange> exclude_ranges{
+ const AddrRangeList exclude_ranges{
AddrRange(0x180, 0x210),
AddrRange(0x000, 0x130),
AddrRange(0x140, 0x170)
@@ -1406,8 +1406,7 @@
auto ranges = r.exclude(exclude_ranges);
EXPECT_EQ(ranges.size(), 2);
- EXPECT_EQ(ranges[0], expected_range1);
- EXPECT_EQ(ranges[1], expected_range2);
+ EXPECT_THAT(ranges, ElementsAre(expected_range1, expected_range2));
}
/*
@@ -1426,7 +1425,7 @@
#ifdef NDEBUG
GTEST_SKIP() << "Skipping as assetions are stripped from fast builds.";
#endif
- const std::vector<AddrRange> exclude_ranges{
+ const AddrRangeList exclude_ranges{
AddrRange(0x180, 0x210),
};
diff --git a/src/python/m5/SimObject.py b/src/python/m5/SimObject.py
index de7752c..d18d879 100644
--- a/src/python/m5/SimObject.py
+++ b/src/python/m5/SimObject.py
@@ -728,7 +728,6 @@
#include "base/compiler.hh"
#include "params/$cls.hh"
-#include "python/pybind11/core.hh"
#include "sim/init.hh"
#include "sim/sim_object.hh"
diff --git a/src/python/m5/params.py b/src/python/m5/params.py
index 8db8e6a..49f5a56 100644
--- a/src/python/m5/params.py
+++ b/src/python/m5/params.py
@@ -865,15 +865,10 @@
self.masks, int(self.intlvMatch))
def exclude(self, ranges):
- from _m5.range import AddrRangeVector
-
- # The wrapped C++ class is assuming an AddrRangeVector
- # We are therefore converting to it before excluding ranges
- # and reconverting it into a list of AddrRange before returning
- pybind_exclude = AddrRangeVector([ r.getValue() for r in ranges ])
+ pybind_exclude = list([ r.getValue() for r in ranges ])
pybind_include = self.getValue().exclude(pybind_exclude)
- return [ AddrRange(r.start(), r.end()) for r in pybind_include ]
+ return list([ AddrRange(r.start(), r.end()) for r in
pybind_include ])
# Boolean parameter type. Python doesn't let you subclass bool, since
# it doesn't want to let you create multiple instances of True and
diff --git a/src/python/pybind11/core.cc b/src/python/pybind11/core.cc
index 257ae23..6f4acd3 100644
--- a/src/python/pybind11/core.cc
+++ b/src/python/pybind11/core.cc
@@ -39,11 +39,10 @@
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/
+#include "pybind11/operators.h"
#include "pybind11/pybind11.h"
#include "pybind11/stl.h"
-#include "python/pybind11/core.hh"
-
#include <ctime>
#include "base/addr_range.hh"
@@ -170,10 +169,6 @@
.def("exclude", &AddrRange::exclude)
;
- // We need to make vectors of AddrRange opaque to avoid weird
- // memory allocation issues in PyBind's STL wrappers.
- py::bind_vector<std::vector<AddrRange>>(m, "AddrRangeVector");
-
m.def("RangeEx", &RangeEx);
m.def("RangeIn", &RangeIn);
m.def("RangeSize", &RangeSize);
--
To view, visit https://gem5-review.googlesource.com/c/public/gem5/+/50344
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: I1a03b25990025688aa760a67d3e7a2e8141384ce
Gerrit-Change-Number: 50344
Gerrit-PatchSet: 1
Gerrit-Owner: Gabe Black <[email protected]>
Gerrit-MessageType: newchange
_______________________________________________
gem5-dev mailing list -- [email protected]
To unsubscribe send an email to [email protected]
%(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
