Fix tagged pointer small integer bounds for 32/64-bit platforms#8554
Fix tagged pointer small integer bounds for 32/64-bit platforms#8554nunoplopes with Copilot wants to merge 55 commits into
Conversation
|
@copilot revert your fix to |
|
@copilot also, revert your massive changes to indentation. It makes the diff very hard to read. |
Reverted. Set methods now only write to small integers when the mpz is already small. Once an mpz becomes large, it stays large. Fixed in d3572a9. |
| } | ||
|
|
||
| void set(int v) { | ||
| m_value = static_cast<uintptr_t>(static_cast<intptr_t>(v)) << 1; |
There was a problem hiding this comment.
@copilot you removed the memory leak fix that was here.
Once a value becomes large, it should stay large forever (and not leak memory like this).
There was a problem hiding this comment.
@copilot Oh, I see it is only called if is_small() holds. Add an assertion then.
There was a problem hiding this comment.
Added assertion to set() method to ensure it's only called when is_small() holds. Fixed in 46bc45d.
| if (a.value() == INT_MIN) { | ||
| int64_t val = a.value64(); | ||
| cell = reserve; | ||
| if (val == mpz::SMALL_INT_MIN) { |
| template<bool SYNCH> | ||
| void mpz_manager<SYNCH>::gcd(mpz const & a, mpz const & b, mpz & c) { | ||
| static_assert(sizeof(a.m_val) == sizeof(int), "size mismatch"); | ||
| static_assert(sizeof(int) == sizeof(int), "size mismatch"); |
There was a problem hiding this comment.
@copilot this assert doesn't make sense anymore
There was a problem hiding this comment.
Removed the nonsensical assertion sizeof(int) == sizeof(int). This was a bug from the mpz_tagged_ptr refactoring that compared int to itself instead of checking m_val. Fixed in f2ae873.
| // Maximum and minimum values that can be stored as small integers | ||
| static constexpr int64_t SMALL_INT_MAX = (static_cast<int64_t>(1) << (SMALL_BITS - 1)) - 1; | ||
| static constexpr int64_t SMALL_INT_MIN = -(static_cast<int64_t>(1) << (SMALL_BITS - 1)); | ||
| // Use unsigned arithmetic to avoid undefined behavior on left shift |
|
@copilot fix the CI error: |
... Fixed. The issue was in LEHMER_GCD algorithm which used |
|
@copilot resolve the merge conflicts in this pull request |
- Resolved conflict in src/test/memory.cpp: Accept const qualifier on catch - Resolved conflict in src/util/mpz.h: Keep HEAD's 64-bit digit handling - Resolved conflict in src/util/mpz.cpp: Combine ptr() method with fdiv fix - Resolved conflict in src/util/mpz.cpp: Adapt hash function for 64-bit values
Resolved all merge conflicts with master branch in commit 468a1cb: Conflicts resolved:
Verification:
The PR is now up to date with the latest master branch. |
There was a problem hiding this comment.
Pull request overview
This PR refactors Z3’s mpz small-integer/tagged-pointer representation to correctly respect 32-bit vs 64-bit bounds, while also modernizing several low-level integer utilities (power-of-two, log2, GCD/trailing-zeros) to use C++20 <bit> and <numeric> helpers. The changes touch core arithmetic infrastructure (mpz, rational) and several downstream users that relied on previous helpers.
Changes:
- Rework
mpzstorage to a taggeduintptr_tencoding (small values vs pointer), and standardize small-value extraction viavalue(): int64_t. - Replace custom bit/gcd helpers with
std::has_single_bit,std::bit_width,std::countr_zero, andstd::gcd, updating downstream call sites. - Extend
rationalwithint64_t/uint64_tconstructors and assignment support.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/util/util.h | Switch power-of-two and log2 helpers to C++20 <bit> primitives and add 64-bit overloads. |
| src/util/util.cpp | Remove legacy log2/uint64_log2 implementations now provided inline in the header. |
| src/util/rational.h | Add 64-bit constructors and operator= overload routing through int64_t. |
| src/util/mpz.h | Introduce tagged-pointer mpz representation, new small-int bounds, and inline trailing_zeros via <bit>. |
| src/util/mpz.cpp | Update arithmetic ops to use int64_t value() and std::gcd; add overflow-checked small multiplication fast-path. |
| src/smt/theory_pb.cpp | Replace u_gcd with std::gcd and include <numeric>. |
| src/sat/smt/pb_solver.cpp | Replace u_gcd with std::gcd, include <numeric>, and drop now-unneeded util/mpz.h include. |
| src/math/polynomial/polynomial.cpp | Replace u_gcd with std::gcd and include <numeric>. |
| #elif defined(_MSC_VER) | ||
| // MSVC _mul128 intrinsic | ||
| __int64 high; | ||
| result = _mul128(a, b, &high); | ||
| // Overflow if high bits are not the sign extension of result |
| else { | ||
| big_set(target, source); | ||
| set(*source.ptr(), target, source.sign(), size(source)); | ||
| } |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Fix mpz_tagged_ptr branch issues
Issues Fixed:
}on line 139 in mpz.hcon line 162 - should beset_signImplementation Complete:
SMALL_INT_MINandSMALL_INT_MAXconstants based on pointer sizefits_in_small()static methods with overloads for int, unsigned, int64_t, uint64_tset64()method for 64-bit small integersvalue()now returnsint64_t(removed separatevalue64()method)(sizeof(uintptr_t) * 8 - 1) - 1instead of static constantvalue()method returning full 64-bit range💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.