DynamicAPInt: optimize size of structure #97831
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reuse the APInt::BitWidth to eliminate DynamicAPInt::HoldsLarge, cutting the size of DynamicAPInt by four bytes. This is implemented by making DynamicAPInt a friend of SlowDynamicAPInt and APInt, so it can directly access SlowDynamicAPInt::Val and APInt::BitWidth.
|
@llvm/pr-subscribers-llvm-support @llvm/pr-subscribers-llvm-adt Author: Ramkumar Ramachandra (artagnon) ChangesReuse the APInt::BitWidth to eliminate DynamicAPInt::HoldsLarge, cutting the size of DynamicAPInt by four bytes. This is implemented by making DynamicAPInt a friend of SlowDynamicAPInt and APInt, so it can directly access SlowDynamicAPInt::Val and APInt::BitWidth. Full diff: https://github.com/llvm/llvm-project/pull/97831.diff 3 Files Affected:
diff --git a/llvm/include/llvm/ADT/APInt.h b/llvm/include/llvm/ADT/APInt.h
index 6cfa6ec665084..108df7e0eaeaa 100644
--- a/llvm/include/llvm/ADT/APInt.h
+++ b/llvm/include/llvm/ADT/APInt.h
@@ -30,6 +30,7 @@ class StringRef;
class hash_code;
class raw_ostream;
struct Align;
+class DynamicAPInt;
template <typename T> class SmallVectorImpl;
template <typename T> class ArrayRef;
@@ -1895,6 +1896,9 @@ class [[nodiscard]] APInt {
friend struct DenseMapInfo<APInt, void>;
friend class APSInt;
+ // Make DynamicAPInt a friend so it can access BitWidth directly.
+ friend DynamicAPInt;
+
/// This constructor is used only internally for speed of construction of
/// temporaries. It is unsafe since it takes ownership of the pointer, so it
/// is not public.
diff --git a/llvm/include/llvm/ADT/DynamicAPInt.h b/llvm/include/llvm/ADT/DynamicAPInt.h
index f312f776df971..03325d3313b5a 100644
--- a/llvm/include/llvm/ADT/DynamicAPInt.h
+++ b/llvm/include/llvm/ADT/DynamicAPInt.h
@@ -35,21 +35,20 @@ namespace llvm {
/// We always_inline all operations; removing these results in a 1.5x
/// performance slowdown.
///
-/// When HoldsLarge is true, a SlowMPInt is held in the union. If it is false,
-/// the int64_t is held. Using std::variant instead would lead to significantly
-/// worse performance.
+/// When isLarge returns true, a SlowMPInt is held in the union. If isSmall
+/// returns true, the int64_t is held. Using std::variant instead would lead to
+/// significantly worse performance.
class DynamicAPInt {
union {
int64_t ValSmall;
detail::SlowDynamicAPInt ValLarge;
};
- unsigned HoldsLarge;
LLVM_ATTRIBUTE_ALWAYS_INLINE void initSmall(int64_t O) {
if (LLVM_UNLIKELY(isLarge()))
ValLarge.detail::SlowDynamicAPInt::~SlowDynamicAPInt();
ValSmall = O;
- HoldsLarge = false;
+ ValLarge.Val.BitWidth = 0;
}
LLVM_ATTRIBUTE_ALWAYS_INLINE void
initLarge(const detail::SlowDynamicAPInt &O) {
@@ -66,14 +65,13 @@ class DynamicAPInt {
// and leak it.
ValLarge = O;
}
- HoldsLarge = true;
}
LLVM_ATTRIBUTE_ALWAYS_INLINE explicit DynamicAPInt(
const detail::SlowDynamicAPInt &Val)
- : ValLarge(Val), HoldsLarge(true) {}
- LLVM_ATTRIBUTE_ALWAYS_INLINE bool isSmall() const { return !HoldsLarge; }
- LLVM_ATTRIBUTE_ALWAYS_INLINE bool isLarge() const { return HoldsLarge; }
+ : ValLarge(Val) {}
+ constexpr bool isSmall() const { return ValLarge.Val.BitWidth == 0; }
+ constexpr bool isLarge() const { return !isSmall(); }
/// Get the stored value. For getSmall/Large,
/// the stored value should be small/large.
LLVM_ATTRIBUTE_ALWAYS_INLINE int64_t getSmall() const {
@@ -105,14 +103,17 @@ class DynamicAPInt {
public:
LLVM_ATTRIBUTE_ALWAYS_INLINE explicit DynamicAPInt(int64_t Val)
- : ValSmall(Val), HoldsLarge(false) {}
+ : ValSmall(Val) {
+ ValLarge.Val.BitWidth = 0;
+ }
LLVM_ATTRIBUTE_ALWAYS_INLINE DynamicAPInt() : DynamicAPInt(0) {}
LLVM_ATTRIBUTE_ALWAYS_INLINE ~DynamicAPInt() {
if (LLVM_UNLIKELY(isLarge()))
ValLarge.detail::SlowDynamicAPInt::~SlowDynamicAPInt();
}
LLVM_ATTRIBUTE_ALWAYS_INLINE DynamicAPInt(const DynamicAPInt &O)
- : ValSmall(O.ValSmall), HoldsLarge(false) {
+ : ValSmall(O.ValSmall) {
+ ValLarge.Val.BitWidth = 0;
if (LLVM_UNLIKELY(O.isLarge()))
initLarge(O.ValLarge);
}
diff --git a/llvm/include/llvm/ADT/SlowDynamicAPInt.h b/llvm/include/llvm/ADT/SlowDynamicAPInt.h
index 1678bda046fec..cda5f39eb98c3 100644
--- a/llvm/include/llvm/ADT/SlowDynamicAPInt.h
+++ b/llvm/include/llvm/ADT/SlowDynamicAPInt.h
@@ -21,6 +21,10 @@
#include "llvm/ADT/APInt.h"
#include "llvm/Support/raw_ostream.h"
+namespace llvm {
+class DynamicAPInt;
+} // namespace llvm
+
namespace llvm::detail {
/// A simple class providing dynamic arbitrary-precision arithmetic. Internally,
/// it stores an APInt, whose width is doubled whenever an overflow occurs at a
@@ -69,6 +73,9 @@ class SlowDynamicAPInt {
/// Overload to compute a hash_code for a SlowDynamicAPInt value.
friend hash_code hash_value(const SlowDynamicAPInt &X); // NOLINT
+ // Make DynamicAPInt a friend so it can access Val directly.
+ friend DynamicAPInt;
+
unsigned getBitWidth() const { return Val.getBitWidth(); }
void print(raw_ostream &OS) const;
|
nikic
approved these changes
Jul 5, 2024
Will LLVM Compile Time Tracker be sufficient? |
No, I meant for the Presburger library. I thought you had benchmarks for it, but maybe that was someone else? |
The best I can do is statistical benchmarking of wall-clock-time of the unit-test executable of the Presburger library. I don't have any other benchmarks. |
|
Did not review, but am happy to accept in principle if you do not see a regression in the performance of the Presburger unit tests. Can you confirm this? |
Superty
reviewed
Jul 5, 2024
|
Okay, the results are in, and I'm confused about why there is a 36% slowdown:
|
|
Okay, the symbol table shows that |
|
Nice 4% speedup.
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
artagnon
force-pushed
the
dynamicapint-optz
branch
from
e48667c
to
71fcd1e
Compare
Superty
approved these changes
Jul 6, 2024
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/35/builds/728 Here is the relevant piece of the build log for the reference: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reuse the APInt::BitWidth to eliminate DynamicAPInt::HoldsLarge, cutting the size of DynamicAPInt by four bytes. This is implemented by making DynamicAPInt a friend of SlowDynamicAPInt and APInt, so it can directly access SlowDynamicAPInt::Val and APInt::BitWidth.
We get a speedup of 4% with this patch.