Skip to content

Conversation

@resistor
Copy link
Collaborator

@resistor resistor commented Dec 5, 2024

Because it was implemented in terms of getMaxIndexSize, it was always
rounding the values up to a multiple of 8. Additionally, it was using
the PointerSpec's BitWidth rather than its IndexBitWidth, which was
self-evidently incorrect.

Since getMaxIndexSize was only used by getMaxIndexSizeInBits, and its
name and function seem niche and somewhat confusing, go ahead and remove
it until a concrete need for it arises.

@llvmbot llvmbot added the llvm:ir label Dec 5, 2024
@llvmbot
Copy link
Member

llvmbot commented Dec 5, 2024

@llvm/pr-subscribers-llvm-ir

Author: Owen Anderson (resistor)

Changes

Because it was implemented in terms of getMaxIndexSize, it was always
rounding the values up to a multiple of 8. Additionally, it was using
the PointerSpec's BitWidth rather than its IndexBitWidth, which was
self-evidently incorrect.

Since getMaxIndexSize was only used by getMaxIndexSizeInBits, and its
name and function seem niche and somewhat confusing, go ahead and remove
it until a concrete need for it arises.


Full diff: https://github.com/llvm/llvm-project/pull/118740.diff

2 Files Affected:

  • (modified) llvm/include/llvm/IR/DataLayout.h (+1-6)
  • (modified) llvm/lib/IR/DataLayout.cpp (+2-4)
diff --git a/llvm/include/llvm/IR/DataLayout.h b/llvm/include/llvm/IR/DataLayout.h
index 93bd519f5727d8..5e6a6198a94733 100644
--- a/llvm/include/llvm/IR/DataLayout.h
+++ b/llvm/include/llvm/IR/DataLayout.h
@@ -330,9 +330,6 @@ class DataLayout {
   /// the backends/clients are updated.
   unsigned getPointerSize(unsigned AS = 0) const;
 
-  /// Returns the maximum index size over all address spaces.
-  unsigned getMaxIndexSize() const;
-
   // Index size in bytes used for address calculation,
   /// rounded up to a whole number of bytes.
   unsigned getIndexSize(unsigned AS) const;
@@ -369,9 +366,7 @@ class DataLayout {
   }
 
   /// Returns the maximum index size over all address spaces.
-  unsigned getMaxIndexSizeInBits() const {
-    return getMaxIndexSize() * 8;
-  }
+  unsigned getMaxIndexSizeInBits() const;
 
   /// Size in bits of index used for address calculation in getelementptr.
   unsigned getIndexSizeInBits(unsigned AS) const {
diff --git a/llvm/lib/IR/DataLayout.cpp b/llvm/lib/IR/DataLayout.cpp
index a4af0ead07cf61..5a110ed2b04058 100644
--- a/llvm/lib/IR/DataLayout.cpp
+++ b/llvm/lib/IR/DataLayout.cpp
@@ -740,12 +740,10 @@ unsigned DataLayout::getPointerSize(unsigned AS) const {
   return divideCeil(getPointerSpec(AS).BitWidth, 8);
 }
 
-unsigned DataLayout::getMaxIndexSize() const {
+unsigned DataLayout::getMaxIndexSizeInBits() const {
   unsigned MaxIndexSize = 0;
   for (const PointerSpec &Spec : PointerSpecs)
-    MaxIndexSize =
-        std::max(MaxIndexSize, (unsigned)divideCeil(Spec.BitWidth, 8));
-
+    MaxIndexSize =std::max(MaxIndexSize, Spec.IndexBitWidth);
   return MaxIndexSize;
 }
 

@github-actions
Copy link

github-actions bot commented Dec 5, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

Because it was implemented in terms of getMaxIndexSize, it was always
rounding the values up to a multiple of 8. Additionally, it was using
the PointerSpec's BitWidth rather than its IndexBitWidth, which was
self-evidently incorrect.

Since getMaxIndexSize was only used by getMaxIndexSizeInBits, and its
name and function seem niche and somewhat confusing, go ahead and remove
it until a concrete need for it arises.
Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@resistor resistor merged commit 698d832 into llvm:main Dec 5, 2024
8 checks passed
@resistor
Copy link
Collaborator Author

Tagging @jrtc27 and @arichardson for awareness on CHERI-derived PRs

@jrtc27
Copy link
Collaborator

jrtc27 commented Dec 10, 2024

Are there no tests that could expose the difference in behaviour?

@resistor
Copy link
Collaborator Author

Are there no tests that could expose the difference in behaviour?

Use of index-size vs pointer-size seems to still be in a transitional state, as I've encountered quite a few places where we should be using the former but are still using the latter. As we fix those, the tests for them will also form tests for this, such as the test in #119138

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants