[DataLayout] Refactor storage of non-integral address spaces #105734

arichardson · 2024-08-22T21:28:24Z

Instead of storing this as a separate array of non-integral pointers,
add it to the PointerSpec class instead. This will allow for future
simplifications such as splitting the non-integral property into
multiple distinct ones: relocatable (i.e. non-stable representation) and
non-integral representation (i.e. pointers with metadata).

Created using spr 1.3.6-beta.1

llvmbot · 2024-08-22T21:28:56Z

@llvm/pr-subscribers-llvm-ir

Author: Alexander Richardson (arichardson)

Changes

Instead of storing this as a separate array of non-integral pointers,
add it to the PointerSpec class instead. This will allow for future
simplifications such as splitting the non-integral property into
multiple distinct ones: relocatable (i.e. non-stable representation) and
non-integral representation (i.e. pointers with metadata).

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

2 Files Affected:

(modified) llvm/include/llvm/IR/DataLayout.h (+17-11)
(modified) llvm/lib/IR/DataLayout.cpp (+21-9)

diff --git a/llvm/include/llvm/IR/DataLayout.h b/llvm/include/llvm/IR/DataLayout.h
index 2f06bda6c30a51..953deb6653cc9a 100644
--- a/llvm/include/llvm/IR/DataLayout.h
+++ b/llvm/include/llvm/IR/DataLayout.h
@@ -78,7 +78,11 @@ class DataLayout {
     Align ABIAlign;
     Align PrefAlign;
     uint32_t IndexBitWidth;
-
+    /// Pointers in this address space don't have a well-defined bitwise
+    /// representation (e.g. may be relocated by a copying garbage collector).
+    /// Additionally, they may also be non-integral (i.e. containing additional
+    /// metadata such as bounds information/permissions).
+    bool IsNonIntegral = false;
     bool operator==(const PointerSpec &Other) const;
   };
 
@@ -133,10 +137,6 @@ class DataLayout {
   // The StructType -> StructLayout map.
   mutable void *LayoutMap = nullptr;
 
-  /// Pointers in these address spaces are non-integral, and don't have a
-  /// well-defined bitwise representation.
-  SmallVector<unsigned, 8> NonIntegralAddressSpaces;
-
   /// Sets or updates the specification for the given primitive type.
   void setPrimitiveSpec(char Specifier, uint32_t BitWidth, Align ABIAlign,
                         Align PrefAlign);
@@ -147,7 +147,8 @@ class DataLayout {
 
   /// Sets or updates the specification for pointer in the given address space.
   void setPointerSpec(uint32_t AddrSpace, uint32_t BitWidth, Align ABIAlign,
-                      Align PrefAlign, uint32_t IndexBitWidth);
+                      Align PrefAlign, uint32_t IndexBitWidth,
+                      bool IsNonIntegral);
 
   /// Internal helper to get alignment for integer of given bitwidth.
   Align getIntegerAlignment(uint32_t BitWidth, bool abi_or_pref) const;
@@ -165,7 +166,8 @@ class DataLayout {
   Error parsePointerSpec(StringRef Spec);
 
   /// Attempts to parse a single specification.
-  Error parseSpecification(StringRef Spec);
+  Error parseSpecification(StringRef Spec,
+                           SmallVectorImpl<unsigned> &NonIntegralAddressSpaces);
 
   /// Attempts to parse a data layout string.
   Error parseLayoutString(StringRef LayoutString);
@@ -343,13 +345,17 @@ class DataLayout {
 
   /// Return the address spaces containing non-integral pointers.  Pointers in
   /// this address space don't have a well-defined bitwise representation.
-  ArrayRef<unsigned> getNonIntegralAddressSpaces() const {
-    return NonIntegralAddressSpaces;
+  SmallVector<unsigned, 8> getNonIntegralAddressSpaces() const {
+    SmallVector<unsigned, 8> AddrSpaces;
+    for (const PointerSpec &PS : PointerSpecs) {
+      if (PS.IsNonIntegral)
+        AddrSpaces.push_back(PS.AddrSpace);
+    }
+    return AddrSpaces;
   }
 
   bool isNonIntegralAddressSpace(unsigned AddrSpace) const {
-    ArrayRef<unsigned> NonIntegralSpaces = getNonIntegralAddressSpaces();
-    return is_contained(NonIntegralSpaces, AddrSpace);
+    return getPointerSpec(AddrSpace).IsNonIntegral;
   }
 
   bool isNonIntegralPointerType(PointerType *PT) const {
diff --git a/llvm/lib/IR/DataLayout.cpp b/llvm/lib/IR/DataLayout.cpp
index d295d1f5785eb9..a7a1fa8ef03ede 100644
--- a/llvm/lib/IR/DataLayout.cpp
+++ b/llvm/lib/IR/DataLayout.cpp
@@ -151,7 +151,8 @@ bool DataLayout::PrimitiveSpec::operator==(const PrimitiveSpec &Other) const {
 bool DataLayout::PointerSpec::operator==(const PointerSpec &Other) const {
   return AddrSpace == Other.AddrSpace && BitWidth == Other.BitWidth &&
          ABIAlign == Other.ABIAlign && PrefAlign == Other.PrefAlign &&
-         IndexBitWidth == Other.IndexBitWidth;
+         IndexBitWidth == Other.IndexBitWidth &&
+         IsNonIntegral == Other.IsNonIntegral;
 }
 
 namespace {
@@ -206,7 +207,8 @@ constexpr DataLayout::PrimitiveSpec DefaultVectorSpecs[] = {
 
 // Default pointer type specifications.
 constexpr DataLayout::PointerSpec DefaultPointerSpecs[] = {
-    {0, 64, Align::Constant<8>(), Align::Constant<8>(), 64} // p0:64:64:64:64
+    {0, 64, Align::Constant<8>(), Align::Constant<8>(), 64,
+     false} // p0:64:64:64:64
 };
 
 DataLayout::DataLayout()
@@ -239,13 +241,11 @@ DataLayout &DataLayout::operator=(const DataLayout &Other) {
   PointerSpecs = Other.PointerSpecs;
   StructABIAlignment = Other.StructABIAlignment;
   StructPrefAlignment = Other.StructPrefAlignment;
-  NonIntegralAddressSpaces = Other.NonIntegralAddressSpaces;
   return *this;
 }
 
 bool DataLayout::operator==(const DataLayout &Other) const {
   // NOTE: StringRepresentation might differ, it is not canonicalized.
-  // FIXME: NonIntegralAddressSpaces isn't compared.
   return BigEndian == Other.BigEndian &&
          AllocaAddrSpace == Other.AllocaAddrSpace &&
          ProgramAddrSpace == Other.ProgramAddrSpace &&
@@ -454,11 +454,13 @@ Error DataLayout::parsePointerSpec(StringRef Spec) {
     return createStringError(
         "index size cannot be larger than the pointer size");
 
-  setPointerSpec(AddrSpace, BitWidth, ABIAlign, PrefAlign, IndexBitWidth);
+  setPointerSpec(AddrSpace, BitWidth, ABIAlign, PrefAlign, IndexBitWidth,
+                 false);
   return Error::success();
 }
 
-Error DataLayout::parseSpecification(StringRef Spec) {
+Error DataLayout::parseSpecification(
+    StringRef Spec, SmallVectorImpl<unsigned> &NonIntegralAddressSpaces) {
   // The "ni" specifier is the only two-character specifier. Handle it first.
   if (Spec.starts_with("ni")) {
     // ni:<address space>[:<address space>]...
@@ -614,12 +616,21 @@ Error DataLayout::parseLayoutString(StringRef LayoutString) {
 
   // Split the data layout string into specifications separated by '-' and
   // parse each specification individually, updating internal data structures.
+  SmallVector<unsigned, 8> NonIntegralAddressSpaces;
   for (StringRef Spec : split(LayoutString, '-')) {
     if (Spec.empty())
       return createStringError("empty specification is not allowed");
-    if (Error Err = parseSpecification(Spec))
+    if (Error Err = parseSpecification(Spec, NonIntegralAddressSpaces))
       return Err;
   }
+  // Mark all address spaces that were qualified as non-integral now. This has
+  // to be done later since the non-integral property is not part of the data
+  // layout pointer specification.
+  for (unsigned AS : NonIntegralAddressSpaces) {
+    const PointerSpec &PS = getPointerSpec(AS);
+    setPointerSpec(AS, PS.BitWidth, PS.ABIAlign, PS.PrefAlign, PS.IndexBitWidth,
+                   true);
+  }
 
   return Error::success();
 }
@@ -666,16 +677,17 @@ DataLayout::getPointerSpec(uint32_t AddrSpace) const {
 
 void DataLayout::setPointerSpec(uint32_t AddrSpace, uint32_t BitWidth,
                                 Align ABIAlign, Align PrefAlign,
-                                uint32_t IndexBitWidth) {
+                                uint32_t IndexBitWidth, bool IsNonIntegral) {
   auto I = lower_bound(PointerSpecs, AddrSpace, LessPointerAddrSpace());
   if (I == PointerSpecs.end() || I->AddrSpace != AddrSpace) {
     PointerSpecs.insert(I, PointerSpec{AddrSpace, BitWidth, ABIAlign, PrefAlign,
-                                       IndexBitWidth});
+                                       IndexBitWidth, IsNonIntegral});
   } else {
     I->BitWidth = BitWidth;
     I->ABIAlign = ABIAlign;
     I->PrefAlign = PrefAlign;
     I->IndexBitWidth = IndexBitWidth;
+    I->IsNonIntegral = IsNonIntegral;
   }
 }

s-barannikov

The idea makes sense to me, the "nonintegrality" is a pointer property.
I wish they were combined into one "pn" spec.

llvm/lib/IR/DataLayout.cpp

llvm/include/llvm/IR/DataLayout.h

s-barannikov · 2024-08-23T05:39:31Z

llvm/lib/IR/DataLayout.cpp

  return Error::success();
 }

-Error DataLayout::parseSpecification(StringRef Spec) {
+Error DataLayout::parseSpecification(
+    StringRef Spec, SmallVectorImpl<unsigned> &NonIntegralAddressSpaces) {
  // The "ni" specifier is the only two-character specifier. Handle it first.
  if (Spec.starts_with("ni")) {


(optional) Since "ni" is special, may it make sense to extract this into a function and call it from parseLayoutString loop?

I tried this and it added more extra code so keeping as is.

llvm/lib/IR/DataLayout.cpp

s-barannikov · 2024-08-23T05:50:42Z

lld/wasm/lto tests are failing for some reason:

wasm-ld: /var/lib/buildkite-agent/builds/linux-56-59b8f5d88-q7g2n-1/llvm-project/github-pull-requests/llvm/lib/CodeGen/MachineFunction.cpp:248: void llvm::MachineFunction::init(): Assertion `Target.isCompatibleDataLayout(getDataLayout()) && "Can't create a MachineFunction using a Module with a " "Target-incompatible DataLayout attached\n"' failed.

arichardson · 2024-08-23T16:51:07Z

The idea makes sense to me, the "nonintegrality" is a pointer property. I wish they were combined into one "pn" spec.

Agreed, it makes more sense to me to have this be part of the pointer spec: #105735 (with the additional change of splitting the non-integral properties).

arsenm

This makes more sense

arsenm · 2024-08-28T16:58:52Z

llvm/include/llvm/IR/DataLayout.h

@@ -343,13 +345,17 @@ class DataLayout {

  /// Return the address spaces containing non-integral pointers.  Pointers in
  /// this address space don't have a well-defined bitwise representation.
-  ArrayRef<unsigned> getNonIntegralAddressSpaces() const {
-    return NonIntegralAddressSpaces;
+  SmallVector<unsigned, 8> getNonIntegralAddressSpaces() const {


Is this actually used for anything now?

It is only used in tests - I could drop those if you prefer?

No, it might be useful somewhere I suppose

Created using spr 1.3.6-beta.1

These mismatched data layouts are exposed by the refactoring in #105734 that now also compares the non-integral pointers list of the data layout. I dropped the explicit definition from all tests that use opt/llc since those tools will insert the correct value, but for the llvm-as based tests and explicit layout is required. Reviewed By: MaskRay Pull Request: #107276

Created using spr 1.3.6-beta.1

These mismatched data layouts are exposed by the refactoring in llvm#105734 that now also compares the non-integral pointers list of the data layout. I dropped the explicit definition from all tests that use opt/llc since those tools will insert the correct value, but for the llvm-as based tests and explicit layout is required. Reviewed By: MaskRay Pull Request: llvm#107276

Instead of storing this as a separate array of non-integral pointers, add it to the PointerSpec class instead. This will allow for future simplifications such as splitting the non-integral property into multiple distinct ones: relocatable (i.e. non-stable representation) and non-integral representation (i.e. pointers with metadata). Reviewed By: arsenm Pull Request: llvm#105734

[𝘀𝗽𝗿] initial version

98c8507

Created using spr 1.3.6-beta.1

llvmbot added the llvm:ir label Aug 22, 2024

arichardson requested a review from s-barannikov August 22, 2024 21:28

arichardson requested review from nikic and arsenm August 22, 2024 21:32

s-barannikov reviewed Aug 23, 2024

View reviewed changes

arsenm approved these changes Aug 28, 2024

View reviewed changes

arichardson mentioned this pull request Sep 4, 2024

[lld][WebAssembly] Update datalayout strings to latest version #107276

Merged

rebase

1bda0dd

Created using spr 1.3.6-beta.1

address feedback

4bd5209

Created using spr 1.3.6-beta.1

arichardson merged commit 305a1ce into main Oct 25, 2024
8 checks passed

arichardson deleted the users/arichardson/spr/datalayout-refactor-storage-of-non-integral-address-spaces branch October 25, 2024 17:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[DataLayout] Refactor storage of non-integral address spaces #105734

[DataLayout] Refactor storage of non-integral address spaces #105734

Uh oh!

arichardson commented Aug 22, 2024

Uh oh!

llvmbot commented Aug 22, 2024

Uh oh!

s-barannikov left a comment

Uh oh!

Uh oh!

Uh oh!

s-barannikov Aug 23, 2024

Uh oh!

arichardson Oct 25, 2024

Uh oh!

Uh oh!

s-barannikov commented Aug 23, 2024

Uh oh!

arichardson commented Aug 23, 2024

Uh oh!

arsenm left a comment

Uh oh!

arsenm Aug 28, 2024

Uh oh!

arichardson Aug 28, 2024

Uh oh!

arsenm Aug 28, 2024

Uh oh!

Uh oh!

Uh oh!

[DataLayout] Refactor storage of non-integral address spaces #105734

[DataLayout] Refactor storage of non-integral address spaces #105734

Uh oh!

Conversation

arichardson commented Aug 22, 2024

Uh oh!

llvmbot commented Aug 22, 2024

Uh oh!

s-barannikov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

s-barannikov Aug 23, 2024

Choose a reason for hiding this comment

Uh oh!

arichardson Oct 25, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

s-barannikov commented Aug 23, 2024

Uh oh!

arichardson commented Aug 23, 2024

Uh oh!

arsenm left a comment

Choose a reason for hiding this comment

Uh oh!

arsenm Aug 28, 2024

Choose a reason for hiding this comment

Uh oh!

arichardson Aug 28, 2024

Choose a reason for hiding this comment

Uh oh!

arsenm Aug 28, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!