[PATCH] D144686: [CodeGen] Remove the template specialization of `Address` that stores alignment information into pointers

2023-02-24 Thread Akira Hatanaka via Phabricator via cfe-commits 
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGe419e22ff6fd: [CodeGen] Stop storing alignment information 
into pointers in Address (authored by ahatanak).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144686/new/

https://reviews.llvm.org/D144686

Files:
  clang/lib/CodeGen/Address.h

Index: clang/lib/CodeGen/Address.h
===
--- clang/lib/CodeGen/Address.h
+++ clang/lib/CodeGen/Address.h
@@ -25,80 +25,20 @@
 // Indicates whether a pointer is known not to be null.
 enum KnownNonNull_t { NotKnownNonNull, KnownNonNull };
 
-// We try to save some space by using 6 bits over two PointerIntPairs to store
-// the alignment. However, some arches don't support 3 bits in a PointerIntPair
-// so we fallback to storing the alignment separately.
-template = 8> class AddressImpl {};
-
-template  class AddressImpl {
+/// An aligned address.
+class Address {
   llvm::PointerIntPair PointerAndKnownNonNull;
   llvm::Type *ElementType;
   CharUnits Alignment;
 
-public:
-  AddressImpl(llvm::Value *Pointer, llvm::Type *ElementType,
-  CharUnits Alignment, KnownNonNull_t IsKnownNonNull)
-  : PointerAndKnownNonNull(Pointer, IsKnownNonNull),
-ElementType(ElementType), Alignment(Alignment) {}
-  llvm::Value *getPointer() const {
-return PointerAndKnownNonNull.getPointer();
-  }
-  llvm::Type *getElementType() const { return ElementType; }
-  CharUnits getAlignment() const { return Alignment; }
-  KnownNonNull_t isKnownNonNull() const {
-return (KnownNonNull_t)PointerAndKnownNonNull.getInt();
-  }
-  void setKnownNonNull() { PointerAndKnownNonNull.setInt(true); }
-};
-
-template  class AddressImpl {
-  // Int portion stores the non-null bit and the upper 2 bits of the log of the
-  // alignment.
-  llvm::PointerIntPair Pointer;
-  // Int portion stores lower 3 bits of the log of the alignment.
-  llvm::PointerIntPair ElementType;
-
-public:
-  AddressImpl(llvm::Value *Pointer, llvm::Type *ElementType,
-  CharUnits Alignment, KnownNonNull_t IsKnownNonNull)
-  : Pointer(Pointer), ElementType(ElementType) {
-if (Alignment.isZero()) {
-  this->Pointer.setInt(IsKnownNonNull << 2);
-  return;
-}
-// Currently the max supported alignment is exactly 1 << 32 and is
-// guaranteed to be a power of 2, so we can store the log of the alignment
-// into 5 bits.
-assert(Alignment.isPowerOfTwo() && "Alignment cannot be zero");
-auto AlignLog = llvm::Log2_64(Alignment.getQuantity());
-assert(AlignLog < (1 << 5) && "cannot fit alignment into 5 bits");
-this->Pointer.setInt(IsKnownNonNull << 2 | AlignLog >> 3);
-this->ElementType.setInt(AlignLog & 7);
-  }
-  llvm::Value *getPointer() const { return Pointer.getPointer(); }
-  llvm::Type *getElementType() const { return ElementType.getPointer(); }
-  CharUnits getAlignment() const {
-unsigned AlignLog = ((Pointer.getInt() & 0x3) << 3) | ElementType.getInt();
-return CharUnits::fromQuantity(CharUnits::QuantityType(1) << AlignLog);
-  }
-  KnownNonNull_t isKnownNonNull() const {
-return (KnownNonNull_t)(!!(Pointer.getInt() & 0x4));
-  }
-  void setKnownNonNull() { Pointer.setInt(Pointer.getInt() | 0x4); }
-};
-
-/// An aligned address.
-class Address {
-  AddressImpl A;
-
 protected:
-  Address(std::nullptr_t)
-  : A(nullptr, nullptr, CharUnits::Zero(), NotKnownNonNull) {}
+  Address(std::nullptr_t) : ElementType(nullptr) {}
 
 public:
   Address(llvm::Value *Pointer, llvm::Type *ElementType, CharUnits Alignment,
   KnownNonNull_t IsKnownNonNull = NotKnownNonNull)
-  : A(Pointer, ElementType, Alignment, IsKnownNonNull) {
+  : PointerAndKnownNonNull(Pointer, IsKnownNonNull),
+ElementType(ElementType), Alignment(Alignment) {
 assert(Pointer != nullptr && "Pointer cannot be null");
 assert(ElementType != nullptr && "Element type cannot be null");
 assert(llvm::cast(Pointer->getType())
@@ -107,11 +47,13 @@
   }
 
   static Address invalid() { return Address(nullptr); }
-  bool isValid() const { return A.getPointer() != nullptr; }
+  bool isValid() const {
+return PointerAndKnownNonNull.getPointer() != nullptr;
+  }
 
   llvm::Value *getPointer() const {
 assert(isValid());
-return A.getPointer();
+return PointerAndKnownNonNull.getPointer();
   }
 
   /// Return the type of the pointer value.
@@ -122,7 +64,7 @@
   /// Return the type of the values stored in this address.
   llvm::Type *getElementType() const {
 assert(isValid());
-return A.getElementType();
+return ElementType;
   }
 
   /// Return the address space that this address resides in.
@@ -138,7 +80,7 @@
   /// Return the alignment of this pointer.
   CharUnits getAlignment() const {
 assert(isValid());
-return A.getAlignment();
+return Alignment;
   }

[PATCH] D144686: [CodeGen] Remove the template specialization of `Address` that stores alignment information into pointers

2023-02-24 Thread Akira Hatanaka via Phabricator via cfe-commits 
ahatanak updated this revision to Diff 500222.
ahatanak added a comment.

Remove AddressImpl altogether.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144686/new/

https://reviews.llvm.org/D144686

Files:
  clang/lib/CodeGen/Address.h

Index: clang/lib/CodeGen/Address.h
===
--- clang/lib/CodeGen/Address.h
+++ clang/lib/CodeGen/Address.h
@@ -25,80 +25,20 @@
 // Indicates whether a pointer is known not to be null.
 enum KnownNonNull_t { NotKnownNonNull, KnownNonNull };
 
-// We try to save some space by using 6 bits over two PointerIntPairs to store
-// the alignment. However, some arches don't support 3 bits in a PointerIntPair
-// so we fallback to storing the alignment separately.
-template = 8> class AddressImpl {};
-
-template  class AddressImpl {
+/// An aligned address.
+class Address {
   llvm::PointerIntPair PointerAndKnownNonNull;
   llvm::Type *ElementType;
   CharUnits Alignment;
 
-public:
-  AddressImpl(llvm::Value *Pointer, llvm::Type *ElementType,
-  CharUnits Alignment, KnownNonNull_t IsKnownNonNull)
-  : PointerAndKnownNonNull(Pointer, IsKnownNonNull),
-ElementType(ElementType), Alignment(Alignment) {}
-  llvm::Value *getPointer() const {
-return PointerAndKnownNonNull.getPointer();
-  }
-  llvm::Type *getElementType() const { return ElementType; }
-  CharUnits getAlignment() const { return Alignment; }
-  KnownNonNull_t isKnownNonNull() const {
-return (KnownNonNull_t)PointerAndKnownNonNull.getInt();
-  }
-  void setKnownNonNull() { PointerAndKnownNonNull.setInt(true); }
-};
-
-template  class AddressImpl {
-  // Int portion stores the non-null bit and the upper 2 bits of the log of the
-  // alignment.
-  llvm::PointerIntPair Pointer;
-  // Int portion stores lower 3 bits of the log of the alignment.
-  llvm::PointerIntPair ElementType;
-
-public:
-  AddressImpl(llvm::Value *Pointer, llvm::Type *ElementType,
-  CharUnits Alignment, KnownNonNull_t IsKnownNonNull)
-  : Pointer(Pointer), ElementType(ElementType) {
-if (Alignment.isZero()) {
-  this->Pointer.setInt(IsKnownNonNull << 2);
-  return;
-}
-// Currently the max supported alignment is exactly 1 << 32 and is
-// guaranteed to be a power of 2, so we can store the log of the alignment
-// into 5 bits.
-assert(Alignment.isPowerOfTwo() && "Alignment cannot be zero");
-auto AlignLog = llvm::Log2_64(Alignment.getQuantity());
-assert(AlignLog < (1 << 5) && "cannot fit alignment into 5 bits");
-this->Pointer.setInt(IsKnownNonNull << 2 | AlignLog >> 3);
-this->ElementType.setInt(AlignLog & 7);
-  }
-  llvm::Value *getPointer() const { return Pointer.getPointer(); }
-  llvm::Type *getElementType() const { return ElementType.getPointer(); }
-  CharUnits getAlignment() const {
-unsigned AlignLog = ((Pointer.getInt() & 0x3) << 3) | ElementType.getInt();
-return CharUnits::fromQuantity(CharUnits::QuantityType(1) << AlignLog);
-  }
-  KnownNonNull_t isKnownNonNull() const {
-return (KnownNonNull_t)(!!(Pointer.getInt() & 0x4));
-  }
-  void setKnownNonNull() { Pointer.setInt(Pointer.getInt() | 0x4); }
-};
-
-/// An aligned address.
-class Address {
-  AddressImpl A;
-
 protected:
-  Address(std::nullptr_t)
-  : A(nullptr, nullptr, CharUnits::Zero(), NotKnownNonNull) {}
+  Address(std::nullptr_t) : ElementType(nullptr) {}
 
 public:
   Address(llvm::Value *Pointer, llvm::Type *ElementType, CharUnits Alignment,
   KnownNonNull_t IsKnownNonNull = NotKnownNonNull)
-  : A(Pointer, ElementType, Alignment, IsKnownNonNull) {
+  : PointerAndKnownNonNull(Pointer, IsKnownNonNull),
+ElementType(ElementType), Alignment(Alignment) {
 assert(Pointer != nullptr && "Pointer cannot be null");
 assert(ElementType != nullptr && "Element type cannot be null");
 assert(llvm::cast(Pointer->getType())
@@ -107,11 +47,13 @@
   }
 
   static Address invalid() { return Address(nullptr); }
-  bool isValid() const { return A.getPointer() != nullptr; }
+  bool isValid() const {
+return PointerAndKnownNonNull.getPointer() != nullptr;
+  }
 
   llvm::Value *getPointer() const {
 assert(isValid());
-return A.getPointer();
+return PointerAndKnownNonNull.getPointer();
   }
 
   /// Return the type of the pointer value.
@@ -122,7 +64,7 @@
   /// Return the type of the values stored in this address.
   llvm::Type *getElementType() const {
 assert(isValid());
-return A.getElementType();
+return ElementType;
   }
 
   /// Return the address space that this address resides in.
@@ -138,7 +80,7 @@
   /// Return the alignment of this pointer.
   CharUnits getAlignment() const {
 assert(isValid());
-return A.getAlignment();
+return Alignment;
   }
 
   /// Return address with different pointer, but same element type and
@@ -159,13 +101,13 @@
   /// Whether the pointer is known not to be null.
   Kn

[PATCH] D144686: [CodeGen] Remove the template specialization of `Address` that stores alignment information into pointers

2023-02-23 Thread Arthur Eubanks via Phabricator via cfe-commits 
aeubanks added inline comments.



Comment at: clang/lib/CodeGen/Address.h:28
 
-// We try to save some space by using 6 bits over two PointerIntPairs to store
-// the alignment. However, some arches don't support 3 bits in a PointerIntPair
-// so we fallback to storing the alignment separately.
-template = 8> class AddressImpl {};
-
-template  class AddressImpl {
+class AddressImpl {
   llvm::PointerIntPair PointerAndKnownNonNull;

we should remove this class altogether


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144686/new/

https://reviews.llvm.org/D144686

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144686: [CodeGen] Remove the template specialization of `Address` that stores alignment information into pointers

2023-02-23 Thread Akira Hatanaka via Phabricator via cfe-commits 
ahatanak added a comment.

I don't have a test case for this change because there is a bug that prevents 
using the maximum allowed alignment (see 
https://github.com/llvm/llvm-project/issues/60752).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144686/new/

https://reviews.llvm.org/D144686

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144686: [CodeGen] Remove the template specialization of `Address` that stores alignment information into pointers

2023-02-23 Thread Akira Hatanaka via Phabricator via cfe-commits 
ahatanak created this revision.
ahatanak added reviewers: aeubanks, efriedma, rjmccall.
ahatanak added a project: clang.
Herald added a project: All.
ahatanak requested review of this revision.

This fixes a bug introduced in https://reviews.llvm.org/D142584. The patch 
reduced the number of bits used for alignment from 6 bits to 5 bits, which made 
it impossible to encode the maximum allowed alignment in llvm (1 << 32).

This reverts b1613f05ae0ce4efc6b6475ea4459957ebcb0150 
. The 
change was made in an attempt to reduce memory consumption by storing the 
alignment information into pointers, but it turns out it doesn't make much 
difference.

https://llvm-compile-time-tracker.com/compare.php?from=998ad085e865f2e5acc589d6bee0e3379042da2e&to=5de4a1989c474f37ac03f20ccb0aef50f6e3b854&stat=max-rss


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D144686

Files:
  clang/lib/CodeGen/Address.h


Index: clang/lib/CodeGen/Address.h
===
--- clang/lib/CodeGen/Address.h
+++ clang/lib/CodeGen/Address.h
@@ -25,12 +25,7 @@
 // Indicates whether a pointer is known not to be null.
 enum KnownNonNull_t { NotKnownNonNull, KnownNonNull };
 
-// We try to save some space by using 6 bits over two PointerIntPairs to store
-// the alignment. However, some arches don't support 3 bits in a PointerIntPair
-// so we fallback to storing the alignment separately.
-template = 8> class AddressImpl {};
-
-template  class AddressImpl {
+class AddressImpl {
   llvm::PointerIntPair PointerAndKnownNonNull;
   llvm::Type *ElementType;
   CharUnits Alignment;
@@ -51,45 +46,9 @@
   void setKnownNonNull() { PointerAndKnownNonNull.setInt(true); }
 };
 
-template  class AddressImpl {
-  // Int portion stores the non-null bit and the upper 2 bits of the log of the
-  // alignment.
-  llvm::PointerIntPair Pointer;
-  // Int portion stores lower 3 bits of the log of the alignment.
-  llvm::PointerIntPair ElementType;
-
-public:
-  AddressImpl(llvm::Value *Pointer, llvm::Type *ElementType,
-  CharUnits Alignment, KnownNonNull_t IsKnownNonNull)
-  : Pointer(Pointer), ElementType(ElementType) {
-if (Alignment.isZero()) {
-  this->Pointer.setInt(IsKnownNonNull << 2);
-  return;
-}
-// Currently the max supported alignment is exactly 1 << 32 and is
-// guaranteed to be a power of 2, so we can store the log of the alignment
-// into 5 bits.
-assert(Alignment.isPowerOfTwo() && "Alignment cannot be zero");
-auto AlignLog = llvm::Log2_64(Alignment.getQuantity());
-assert(AlignLog < (1 << 5) && "cannot fit alignment into 5 bits");
-this->Pointer.setInt(IsKnownNonNull << 2 | AlignLog >> 3);
-this->ElementType.setInt(AlignLog & 7);
-  }
-  llvm::Value *getPointer() const { return Pointer.getPointer(); }
-  llvm::Type *getElementType() const { return ElementType.getPointer(); }
-  CharUnits getAlignment() const {
-unsigned AlignLog = ((Pointer.getInt() & 0x3) << 3) | ElementType.getInt();
-return CharUnits::fromQuantity(CharUnits::QuantityType(1) << AlignLog);
-  }
-  KnownNonNull_t isKnownNonNull() const {
-return (KnownNonNull_t)(!!(Pointer.getInt() & 0x4));
-  }
-  void setKnownNonNull() { Pointer.setInt(Pointer.getInt() | 0x4); }
-};
-
 /// An aligned address.
 class Address {
-  AddressImpl A;
+  AddressImpl A;
 
 protected:
   Address(std::nullptr_t)


Index: clang/lib/CodeGen/Address.h
===
--- clang/lib/CodeGen/Address.h
+++ clang/lib/CodeGen/Address.h
@@ -25,12 +25,7 @@
 // Indicates whether a pointer is known not to be null.
 enum KnownNonNull_t { NotKnownNonNull, KnownNonNull };
 
-// We try to save some space by using 6 bits over two PointerIntPairs to store
-// the alignment. However, some arches don't support 3 bits in a PointerIntPair
-// so we fallback to storing the alignment separately.
-template = 8> class AddressImpl {};
-
-template  class AddressImpl {
+class AddressImpl {
   llvm::PointerIntPair PointerAndKnownNonNull;
   llvm::Type *ElementType;
   CharUnits Alignment;
@@ -51,45 +46,9 @@
   void setKnownNonNull() { PointerAndKnownNonNull.setInt(true); }
 };
 
-template  class AddressImpl {
-  // Int portion stores the non-null bit and the upper 2 bits of the log of the
-  // alignment.
-  llvm::PointerIntPair Pointer;
-  // Int portion stores lower 3 bits of the log of the alignment.
-  llvm::PointerIntPair ElementType;
-
-public:
-  AddressImpl(llvm::Value *Pointer, llvm::Type *ElementType,
-  CharUnits Alignment, KnownNonNull_t IsKnownNonNull)
-  : Pointer(Pointer), ElementType(ElementType) {
-if (Alignment.isZero()) {
-  this->Pointer.setInt(IsKnownNonNull << 2);
-  return;
-}
-// Currently the max supported alignment is exactly 1 << 32 and is
-// guaranteed to be a power of 2, so we can store the log of the alignme