From df82adca43db7f5820a27624ae3e2b60c9085efd Mon Sep 17 00:00:00 2001 From: Niels Boehm Date: Thu, 22 Jun 2017 12:36:54 +0200 Subject: [PATCH 1/2] Add function testing whether a bitmask is valid. This one verifies bitmasks where low bits are set to 1 (hence the name). Any stray 0 among the lower ones or any stray 1 among the higher zeros renders the mask invalid. The edge cases of all zeros and all ones are considered valid masks. It uses an efficient implementation. It's the counterpart of https://graphics.stanford.edu/~seander/bithacks.html#DetermineIfPowerOf2 --- Source/Core/Common/BitUtils.h | 22 ++++++++++++++++ Source/UnitTests/Common/BitUtilsTest.cpp | 32 ++++++++++++++++++++++++ 2 files changed, 54 insertions(+) diff --git a/Source/Core/Common/BitUtils.h b/Source/Core/Common/BitUtils.h index eacf6bf099..8d5398fb39 100644 --- a/Source/Core/Common/BitUtils.h +++ b/Source/Core/Common/BitUtils.h @@ -99,4 +99,26 @@ constexpr Result ExtractBits(const T src) noexcept return ExtractBits(src, begin, end); } + +/// +/// Verifies whether the supplied value is a valid bit mask of the form 0b00...0011...11. +/// Both edge cases of all zeros and all ones are considered valid masks, too. +/// +/// @param mask The mask value to test for validity. +/// +/// @tparam T The type of the value. +/// +/// @return A bool indicating whether the mask is valid. +/// +template +constexpr bool IsValidLowMask(const T mask) noexcept +{ + static_assert(std::is_integral::value, "Mask must be an integral type."); + static_assert(std::is_unsigned::value, "Signed masks can introduce hard to find bugs."); + + // Can be efficiently determined without looping or bit counting. It's the counterpart + // to https://graphics.stanford.edu/~seander/bithacks.html#DetermineIfPowerOf2 + // and doesn't require special casing either edge case. + return (mask & (mask + 1)) == 0; +} } // namespace Common diff --git a/Source/UnitTests/Common/BitUtilsTest.cpp b/Source/UnitTests/Common/BitUtilsTest.cpp index 987868e6e4..75f7adf50e 100644 --- a/Source/UnitTests/Common/BitUtilsTest.cpp +++ b/Source/UnitTests/Common/BitUtilsTest.cpp @@ -57,3 +57,35 @@ TEST(BitUtils, ExtractBits) // Ensure bit extraction with type overriding works as expected EXPECT_EQ((Common::ExtractBits<0, 31, s32, s32>(negative_one)), -1); } + +TEST(BitUtils, IsValidLowMask) +{ + EXPECT_TRUE(Common::IsValidLowMask(0b0u)); + EXPECT_TRUE(Common::IsValidLowMask(0b1u)); + EXPECT_FALSE(Common::IsValidLowMask(0b10u)); + EXPECT_TRUE(Common::IsValidLowMask(0b11u)); + EXPECT_FALSE(Common::IsValidLowMask(0b1110u)); + EXPECT_TRUE(Common::IsValidLowMask(0b1111u)); + EXPECT_FALSE(Common::IsValidLowMask(0b10000u)); + EXPECT_FALSE(Common::IsValidLowMask(0b101111u)); + + EXPECT_TRUE(Common::IsValidLowMask((u8)~0b0)); + EXPECT_FALSE(Common::IsValidLowMask((u8)(~0b0 - 1))); + EXPECT_FALSE(Common::IsValidLowMask((u8)~(0b10000))); + EXPECT_FALSE(Common::IsValidLowMask((u8)(~((u8)(~0b0) >> 1) | 0b1111))); + + EXPECT_TRUE(Common::IsValidLowMask((u16)~0b0)); + EXPECT_FALSE(Common::IsValidLowMask((u16)(~0b0 - 1))); + EXPECT_FALSE(Common::IsValidLowMask((u16)~(0b10000))); + EXPECT_FALSE(Common::IsValidLowMask((u16)(~((u16)(~0b0) >> 1) | 0b1111))); + + EXPECT_TRUE(Common::IsValidLowMask((u32)~0b0)); + EXPECT_FALSE(Common::IsValidLowMask((u32)(~0b0 - 1))); + EXPECT_FALSE(Common::IsValidLowMask((u32)~(0b10000))); + EXPECT_FALSE(Common::IsValidLowMask((u32)(~((u32)(~0b0) >> 1) | 0b1111))); + + EXPECT_TRUE(Common::IsValidLowMask((u64)~0b0)); + EXPECT_FALSE(Common::IsValidLowMask((u64)(~0b0 - 1))); + EXPECT_FALSE(Common::IsValidLowMask((u64)~(0b10000))); + EXPECT_FALSE(Common::IsValidLowMask((u64)(~((u64)(~0b0) >> 1) | 0b1111))); +} From 56158ca176eee120f29375a286182b5dd3d7fa60 Mon Sep 17 00:00:00 2001 From: Niels Boehm Date: Thu, 22 Jun 2017 13:04:00 +0200 Subject: [PATCH 2/2] Replace MMU mask tests with dedicated function. The efficient function (that is nearly the same as https://graphics.stanford.edu/~seander/bithacks.html#DetermineIfPowerOf2) replaces one loop based instance (which also reused the xx variable afterwards, whereas it should have used htabmask instead) and one instance using the population count a.k.a. Hamming weigth. --- Source/Core/Core/PowerPC/MMU.cpp | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/Source/Core/Core/PowerPC/MMU.cpp b/Source/Core/Core/PowerPC/MMU.cpp index b5e2448617..ad218b4244 100644 --- a/Source/Core/Core/PowerPC/MMU.cpp +++ b/Source/Core/Core/PowerPC/MMU.cpp @@ -7,7 +7,7 @@ #include #include "Common/Atomic.h" -#include "Common/BitSet.h" +#include "Common/BitUtils.h" #include "Common/CommonTypes.h" #include "Core/ConfigManager.h" @@ -947,26 +947,17 @@ static void GenerateISIException(u32 _EffectiveAddress) void SDRUpdated() { u32 htabmask = SDR1_HTABMASK(PowerPC::ppcState.spr[SPR_SDR]); - u32 x = 1; - u32 xx = 0; - int n = 0; - while ((htabmask & x) && (n < 9)) - { - n++; - xx |= x; - x <<= 1; - } - if (htabmask & ~xx) + if (!Common::IsValidLowMask(htabmask)) { return; } u32 htaborg = SDR1_HTABORG(PowerPC::ppcState.spr[SPR_SDR]); - if (htaborg & xx) + if (htaborg & htabmask) { return; } PowerPC::ppcState.pagetable_base = htaborg << 16; - PowerPC::ppcState.pagetable_hashmask = ((xx << 10) | 0x3ff); + PowerPC::ppcState.pagetable_hashmask = ((htabmask << 10) | 0x3ff); } enum TLBLookupResult @@ -1175,7 +1166,7 @@ static void UpdateBATs(BatTable& bat_table, u32 base_spr) // implemented this way for invalid BATs as well. WARN_LOG(POWERPC, "Bad BAT setup: BPRN overlaps BL"); } - if (CountSetBits((u32)(batu.BL + 1)) != 1) + if (!Common::IsValidLowMask((u32)batu.BL)) { // With a valid BAT, the simplest way of masking is // (input & ~BL_mask) for matching and (input & BL_mask) for