From 55864843449b96af822c817214f91c992fbcc14d Mon Sep 17 00:00:00 2001 From: =?utf8?q?Isaac=20S=C3=A1nchez=20Barrera?= Date: Fri, 6 Nov 2020 09:18:15 +0100 Subject: [PATCH] base: Fix `AddrRange::addIntlvBits(Addr)` and new test. MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit The methods `AddrRange::removeIntlvBits(Addr)` and `AddrRange::addIntlvBits(Addr)` should be the inverse of one another, but the latter did not insert the blanks for filling the removed bits in the correct positions. Since the masks are ordered increasingly by the position of the least significant bit of each mask, the lowest bit that has to be inserted at each iteration is always `intlv_bit`, not needing to be shifted to the left or right. The bits that need to be copied from the input address are `intlv_bit-1..0` at each iteration. The test `AddrRangeTest.AddRemoveInterleavBitsAcrossRange` has been updated have masks below bit 12, making the old code not pass the test. A new `AddrRangeTest.AddRemoveInterleavBitsAcrossContiguousRange` test has been added to include a case in which the previous code fails. The corrected code passes both tests. This function is not used anywhere other than the tests and the class `ChannelAddr`. However, it is needed to efficiently implement interleaved caches in the classic mode. Change-Id: I7d626a1f6ecf09a230fc18810d2dad2104d1a865 Signed-off-by: Isaac Sánchez Barrera Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/37175 Tested-by: kokoro Reviewed-by: Nikos Nikoleris Reviewed-by: Bobby R. Bruce Maintainer: Nikos Nikoleris Maintainer: Bobby R. Bruce --- src/base/addr_range.hh | 7 ++++--- src/base/addr_range.test.cc | 34 +++++++++++++++++++++++++++++++--- 2 files changed, 35 insertions(+), 6 deletions(-) diff --git a/src/base/addr_range.hh b/src/base/addr_range.hh index e333b32c9..8a811d495 100644 --- a/src/base/addr_range.hh +++ b/src/base/addr_range.hh @@ -523,9 +523,10 @@ class AddrRange const int intlv_bit = masks_lsb[i]; if (intlv_bit > 0) { // on every iteration we add one bit from the input - // address, and therefore the lowest invtl_bit has - // also shifted to the left by i positions. - a = insertBits(a << 1, intlv_bit + i - 1, 0, a); + // address, but the lowest invtl_bit in the iteration is + // always in the right position because they are sorted + // increasingly from the LSB + a = insertBits(a << 1, intlv_bit - 1, 0, a); } else { a <<= 1; } diff --git a/src/base/addr_range.test.cc b/src/base/addr_range.test.cc index 34921d83c..f2d4efafc 100644 --- a/src/base/addr_range.test.cc +++ b/src/base/addr_range.test.cc @@ -769,8 +769,8 @@ TEST(AddrRangeTest, AddRemoveInterleavBitsAcrossRange) std::vector masks; masks.push_back(1 << 2); masks.push_back(1 << 3); - masks.push_back(1 << 16); - masks.push_back(1 << 30); + masks.push_back(1 << 7); + masks.push_back(1 << 11); uint8_t intlv_match = 0xF; AddrRange r(start, end, masks, intlv_match); @@ -779,7 +779,35 @@ TEST(AddrRangeTest, AddRemoveInterleavBitsAcrossRange) /* * As intlv_match = 0xF, all the interleaved bits should be set. */ - EXPECT_EQ(i | (1 << 2) | (1 << 3) | (1 << 16) | (1 << 30), + EXPECT_EQ(i | (1 << 2) | (1 << 3) | (1 << 7) | (1 << 11), + r.addIntlvBits(removedBits)); + } +} + +TEST(AddrRangeTest, AddRemoveInterleavBitsAcrossContiguousRange) +{ + /* + * This purpose of this test is to ensure that removing then adding + * interleaving bits has no net effect. + * E.g.: + * addr_range.addIntlvBits(add_range.removeIntlvBits(an_address)) should + * always return an_address. + */ + Addr start = 0x00000; + Addr end = 0x10000; + std::vector masks; + masks.push_back(1 << 2); + masks.push_back(1 << 3); + masks.push_back(1 << 4); + uint8_t intlv_match = 0x7; + AddrRange r(start, end, masks, intlv_match); + + for (Addr i = 0; i < 0xFFF; i++) { + Addr removedBits = r.removeIntlvBits(i); + /* + * As intlv_match = 0x7, all the interleaved bits should be set. + */ + EXPECT_EQ(i | (1 << 2) | (1 << 3) | (1 << 4), r.addIntlvBits(removedBits)); } } -- 2.30.2