IR: Remove reference counts from ConstantData

[arsenm]

This is a follow up change to eliminating uselists for ConstantData.
In the previous revision, ConstantData had a replacement reference count
instead of a uselist. This reference count was misleading, and not useful
in the same way as it would be for another value. The references may not
have even been in the current module, since these are shared throughout
the LLVMContext.

This doesn't space leak any more than we previously did; nothing was
attempting to garbage collect unused constants.

Previously the use_empty, and hasNUses type of APIs were supported through
the reference count. These now behave as if the uses are always empty.
Ideally it would be illegal to inspect these, but this forces API complexity
into quite a few places. It may be doable to make it illegal to check these
counts, but I would like there to be a targeted fuzzing effort to make sure
every transform properly deals with a constant in every operand position.

All tests pass if I turn the hasNUses* and getNumUses queries into assertions,
only hasOneUse in particular appears to hit in some set of contexts. I've
added unit tests to ensure logical consistency between these cases

[arsenm]

• IR: Remove redundant UseList check in addUse #138676
• IR: Reorder ConstantData enum values #138638
• IR: Remove reference counts from ConstantData #137314 👈 (View in Graphite)
• IR: Remove uselist for constantdata #137313
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[llvmbot]

@llvm/pr-subscribers-llvm-ir

Author: Matt Arsenault (arsenm)

Changes

This is a follow up change to eliminating uselists for ConstantData.
In the previous revision, ConstantData had a replacement reference count
instead of a uselist. This reference count was misleading, and not useful
in the same way as it would be for another value. The references may not
have even been in the current module, since these are shared throughout
the LLVMContext.

This doesn't space leak any more than we previously did; nothing was
attempting to garbage collect unused constants.

Previously the use_empty, and hasNUses type of APIs were supported through
the reference count. These now behave as if the uses are always empty.
Ideally it would be illegal to inspect these, but this forces API complexity
into quite a few places. It may be doable to make it illegal to check these
counts, but I would like there to be a targeted fuzzing effort to make sure
every transform properly deals with a constant in every operand position.

All tests pass if I turn the hasNUses* and getNumUses queries into assertions,
only hasOneUse in particular appears to hit in some set of contexts. I've
added unit tests to ensure logical consistency between these cases

---

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

7 Files Affected:

• (modified) llvm/docs/ReleaseNotes.md (+3-1)
• (modified) llvm/include/llvm/IR/Constants.h (+2-1)
• (modified) llvm/include/llvm/IR/Use.h (+3-6)
• (modified) llvm/include/llvm/IR/Value.h (+40-78)
• (modified) llvm/lib/IR/Instruction.cpp (+1-3)
• (modified) llvm/lib/IR/Value.cpp (+13-15)
• (modified) llvm/unittests/IR/ConstantsTest.cpp (+36)

diff --git a/llvm/docs/ReleaseNotes.md b/llvm/docs/ReleaseNotes.md
index 411cefe004e16..4665302a4144c 100644
--- a/llvm/docs/ReleaseNotes.md
+++ b/llvm/docs/ReleaseNotes.md
@@ -56,7 +56,9 @@ Makes programs 10x faster by doing Special New Thing.
 Changes to the LLVM IR
 ----------------------
 
-* It is no longer permitted to inspect the uses of ConstantData
+* It is no longer permitted to inspect the uses of ConstantData. Use
+  count APIs will behave as if they have no uses (i.e. use_empty() is
+  always true).
 
 * The `nocapture` attribute has been replaced by `captures(none)`.
 * The constant expression variants of the following instructions have been
diff --git a/llvm/include/llvm/IR/Constants.h b/llvm/include/llvm/IR/Constants.h
index 7b1dbdece43f7..07d71cf7108d2 100644
--- a/llvm/include/llvm/IR/Constants.h
+++ b/llvm/include/llvm/IR/Constants.h
@@ -51,7 +51,8 @@ template <class ConstantClass> struct ConstantAggrKeyType;
 /// Since they can be in use by unrelated modules (and are never based on
 /// GlobalValues), it never makes sense to RAUW them.
 ///
-/// These do not have use lists. It is illegal to inspect the uses.
+/// These do not have use lists. It is illegal to inspect the uses. These behave
+/// as if they have no uses (i.e. use_empty() is always true).
 class ConstantData : public Constant {
   constexpr static IntrusiveOperandsAllocMarker AllocMarker{0};
 
diff --git a/llvm/include/llvm/IR/Use.h b/llvm/include/llvm/IR/Use.h
index bcd1fd6677497..dc22d69ba561d 100644
--- a/llvm/include/llvm/IR/Use.h
+++ b/llvm/include/llvm/IR/Use.h
@@ -23,7 +23,6 @@
 namespace llvm {
 
 template <typename> struct simplify_type;
-class ConstantData;
 class User;
 class Value;
 
@@ -43,7 +42,7 @@ class Use {
 
 private:
   /// Destructor - Only for zap()
-  ~Use();
+  ~Use() { removeFromList(); }
 
   /// Constructor
   Use(User *Parent) : Parent(Parent) {}
@@ -85,10 +84,8 @@ class Use {
   Use **Prev = nullptr;
   User *Parent = nullptr;
 
-  inline void addToList(unsigned &Count);
-  inline void addToList(Use *&List);
-  inline void removeFromList(unsigned &Count);
-  inline void removeFromList(Use *&List);
+  inline void addToList(Use **List);
+  inline void removeFromList();
 };
 
 /// Allow clients to treat uses just like values when using
diff --git a/llvm/include/llvm/IR/Value.h b/llvm/include/llvm/IR/Value.h
index 180b6238eda6c..ae874304c4316 100644
--- a/llvm/include/llvm/IR/Value.h
+++ b/llvm/include/llvm/IR/Value.h
@@ -116,10 +116,7 @@ class Value {
 
 private:
   Type *VTy;
-  union {
-    Use *List = nullptr;
-    unsigned Count;
-  } Uses;
+  Use *UseList = nullptr;
 
   friend class ValueAsMetadata; // Allow access to IsUsedByMD.
   friend class ValueHandleBase; // Allow access to HasValueHandle.
@@ -347,23 +344,21 @@ class Value {
 
   bool use_empty() const {
     assertModuleIsMaterialized();
-    return hasUseList() ? Uses.List == nullptr : Uses.Count == 0;
+    return UseList == nullptr;
   }
 
-  bool materialized_use_empty() const {
-    return hasUseList() ? Uses.List == nullptr : !Uses.Count;
-  }
+  bool materialized_use_empty() const { return UseList == nullptr; }
 
   using use_iterator = use_iterator_impl<Use>;
   using const_use_iterator = use_iterator_impl<const Use>;
 
   use_iterator materialized_use_begin() {
     assert(hasUseList());
-    return use_iterator(Uses.List);
+    return use_iterator(UseList);
   }
   const_use_iterator materialized_use_begin() const {
     assert(hasUseList());
-    return const_use_iterator(Uses.List);
+    return const_use_iterator(UseList);
   }
   use_iterator use_begin() {
     assertModuleIsMaterialized();
@@ -397,11 +392,11 @@ class Value {
 
   user_iterator materialized_user_begin() {
     assert(hasUseList());
-    return user_iterator(Uses.List);
+    return user_iterator(UseList);
   }
   const_user_iterator materialized_user_begin() const {
     assert(hasUseList());
-    return const_user_iterator(Uses.List);
+    return const_user_iterator(UseList);
   }
   user_iterator user_begin() {
     assertModuleIsMaterialized();
@@ -440,11 +435,7 @@ class Value {
   ///
   /// This is specialized because it is a common request and does not require
   /// traversing the whole use list.
-  bool hasOneUse() const {
-    if (!hasUseList())
-      return Uses.Count == 1;
-    return hasSingleElement(uses());
-  }
+  bool hasOneUse() const { return UseList && hasSingleElement(uses()); }
 
   /// Return true if this Value has exactly N uses.
   bool hasNUses(unsigned N) const;
@@ -518,17 +509,8 @@ class Value {
 
   /// This method should only be used by the Use class.
   void addUse(Use &U) {
-    if (hasUseList())
-      U.addToList(Uses.List);
-    else
-      U.addToList(Uses.Count);
-  }
-
-  void removeUse(Use &U) {
-    if (hasUseList())
-      U.removeFromList(Uses.List);
-    else
-      U.removeFromList(Uses.Count);
+    if (UseList || hasUseList())
+      U.addToList(&UseList);
   }
 
   /// Concrete subclass of this.
@@ -870,8 +852,7 @@ class Value {
   ///
   /// \return the first element in the list.
   ///
-  /// \note Completely ignores \a Use::PrevOrCount (doesn't read, doesn't
-  /// update).
+  /// \note Completely ignores \a Use::Prev (doesn't read, doesn't update).
   template <class Compare>
   static Use *mergeUseLists(Use *L, Use *R, Compare Cmp) {
     Use *Merged;
@@ -917,47 +898,8 @@ inline raw_ostream &operator<<(raw_ostream &OS, const Value &V) {
   return OS;
 }
 
-inline Use::~Use() {
-  if (Val)
-    Val->removeUse(*this);
-}
-
-void Use::addToList(unsigned &Count) {
-  assert(isa<ConstantData>(Val) && "Only ConstantData is ref-counted");
-  ++Count;
-
-  // We don't have a uselist - clear the remnant if we are replacing a
-  // non-constant value.
-  Prev = nullptr;
-  Next = nullptr;
-}
-
-void Use::addToList(Use *&List) {
-  assert(!isa<ConstantData>(Val) && "ConstantData has no use-list");
-
-  Next = List;
-  if (Next)
-    Next->Prev = &Next;
-  Prev = &List;
-  List = this;
-}
-
-void Use::removeFromList(unsigned &Count) {
-  assert(isa<ConstantData>(Val));
-  assert(Count > 0 && "reference count underflow");
-  assert(!Prev && !Next && "should not have uselist remnant");
-  --Count;
-}
-
-void Use::removeFromList(Use *&List) {
-  *Prev = Next;
-  if (Next)
-    Next->Prev = Prev;
-}
-
 void Use::set(Value *V) {
-  if (Val)
-    Val->removeUse(*this);
+  removeFromList();
   Val = V;
   if (V)
     V->addUse(*this);
@@ -973,8 +915,28 @@ const Use &Use::operator=(const Use &RHS) {
   return *this;
 }
 
+void Use::addToList(Use **List) {
+  Next = *List;
+  if (Next)
+    Next->Prev = &Next;
+  Prev = List;
+  *Prev = this;
+}
+
+void Use::removeFromList() {
+  if (Prev) {
+    *Prev = Next;
+    if (Next) {
+      Next->Prev = Prev;
+      Next = nullptr;
+    }
+
+    Prev = nullptr;
+  }
+}
+
 template <class Compare> void Value::sortUseList(Compare Cmp) {
-  if (!hasUseList() || !Uses.List || !Uses.List->Next)
+  if (!UseList || !UseList->Next)
     // No need to sort 0 or 1 uses.
     return;
 
@@ -987,10 +949,10 @@ template <class Compare> void Value::sortUseList(Compare Cmp) {
   Use *Slots[MaxSlots];
 
   // Collect the first use, turning it into a single-item list.
-  Use *Next = Uses.List->Next;
-  Uses.List->Next = nullptr;
+  Use *Next = UseList->Next;
+  UseList->Next = nullptr;
   unsigned NumSlots = 1;
-  Slots[0] = Uses.List;
+  Slots[0] = UseList;
 
   // Collect all but the last use.
   while (Next->Next) {
@@ -1026,15 +988,15 @@ template <class Compare> void Value::sortUseList(Compare Cmp) {
   // Merge all the lists together.
   assert(Next && "Expected one more Use");
   assert(!Next->Next && "Expected only one Use");
-  Uses.List = Next;
+  UseList = Next;
   for (unsigned I = 0; I < NumSlots; ++I)
     if (Slots[I])
-      // Since the uses in Slots[I] originally preceded those in Uses.List, send
+      // Since the uses in Slots[I] originally preceded those in UseList, send
       // Slots[I] in as the left parameter to maintain a stable sort.
-      Uses.List = mergeUseLists(Slots[I], Uses.List, Cmp);
+      UseList = mergeUseLists(Slots[I], UseList, Cmp);
 
   // Fix the Prev pointers.
-  for (Use *I = Uses.List, **Prev = &Uses.List; I; I = I->Next) {
+  for (Use *I = UseList, **Prev = &UseList; I; I = I->Next) {
     I->Prev = Prev;
     Prev = &I->Next;
   }
diff --git a/llvm/lib/IR/Instruction.cpp b/llvm/lib/IR/Instruction.cpp
index 19dd68b3a011d..a4d7a2d0fa220 100644
--- a/llvm/lib/IR/Instruction.cpp
+++ b/llvm/lib/IR/Instruction.cpp
@@ -373,9 +373,7 @@ std::optional<BasicBlock::iterator> Instruction::getInsertionPointAfterDef() {
 }
 
 bool Instruction::isOnlyUserOfAnyOperand() {
-  return any_of(operands(), [](const Value *V) {
-    return V->hasUseList() && V->hasOneUser();
-  });
+  return any_of(operands(), [](const Value *V) { return V->hasOneUser(); });
 }
 
 void Instruction::setHasNoUnsignedWrap(bool b) {
diff --git a/llvm/lib/IR/Value.cpp b/llvm/lib/IR/Value.cpp
index 74a96051f33af..926d3452e2326 100644
--- a/llvm/lib/IR/Value.cpp
+++ b/llvm/lib/IR/Value.cpp
@@ -148,15 +148,13 @@ void Value::destroyValueName() {
 }
 
 bool Value::hasNUses(unsigned N) const {
-  if (!hasUseList())
-    return Uses.Count == N;
-  return hasNItems(use_begin(), use_end(), N);
+  // TODO: Disallow for ConstantData and remove !UseList check?
+  return UseList && hasNItems(use_begin(), use_end(), N);
 }
 
 bool Value::hasNUsesOrMore(unsigned N) const {
-  if (!hasUseList())
-    return Uses.Count >= N;
-  return hasNItemsOrMore(use_begin(), use_end(), N);
+  // TODO: Disallow for ConstantData and remove !UseList check?
+  return UseList && hasNItemsOrMore(use_begin(), use_end(), N);
 }
 
 bool Value::hasOneUser() const {
@@ -259,9 +257,9 @@ bool Value::isUsedInBasicBlock(const BasicBlock *BB) const {
 }
 
 unsigned Value::getNumUses() const {
-  if (!hasUseList())
-    return Uses.Count;
-
+  // TODO: Disallow for ConstantData and remove !UseList check?
+  if (!UseList)
+    return 0;
   return (unsigned)std::distance(use_begin(), use_end());
 }
 
@@ -522,7 +520,7 @@ void Value::doRAUW(Value *New, ReplaceMetadataUses ReplaceMetaUses) {
     ValueAsMetadata::handleRAUW(this, New);
 
   while (!materialized_use_empty()) {
-    Use &U = *Uses.List;
+    Use &U = *UseList;
     // Must handle Constants specially, we cannot call replaceUsesOfWith on a
     // constant because they are uniqued.
     if (auto *C = dyn_cast<Constant>(U.getUser())) {
@@ -1102,12 +1100,12 @@ const Value *Value::DoPHITranslation(const BasicBlock *CurBB,
 LLVMContext &Value::getContext() const { return VTy->getContext(); }
 
 void Value::reverseUseList() {
-  if (!Uses.List || !Uses.List->Next || !hasUseList())
+  if (!UseList || !UseList->Next)
     // No need to reverse 0 or 1 uses.
     return;
 
-  Use *Head = Uses.List;
-  Use *Current = Uses.List->Next;
+  Use *Head = UseList;
+  Use *Current = UseList->Next;
   Head->Next = nullptr;
   while (Current) {
     Use *Next = Current->Next;
@@ -1116,8 +1114,8 @@ void Value::reverseUseList() {
     Head = Current;
     Current = Next;
   }
-  Uses.List = Head;
-  Head->Prev = &Uses.List;
+  UseList = Head;
+  Head->Prev = &UseList;
 }
 
 bool Value::isSwiftError() const {
diff --git a/llvm/unittests/IR/ConstantsTest.cpp b/llvm/unittests/IR/ConstantsTest.cpp
index a46178abd9066..4691ae19132bd 100644
--- a/llvm/unittests/IR/ConstantsTest.cpp
+++ b/llvm/unittests/IR/ConstantsTest.cpp
@@ -21,6 +21,42 @@
 namespace llvm {
 namespace {
 
+// Check that use count checks treat ConstantData like they have no uses.
+TEST(ConstantsTest, UseCounts) {
+  LLVMContext Context;
+  Type *Int32Ty = Type::getInt32Ty(Context);
+  Constant *Zero = ConstantInt::get(Int32Ty, 0);
+
+  EXPECT_TRUE(Zero->use_empty());
+  EXPECT_EQ(Zero->getNumUses(), 0u);
+  EXPECT_FALSE(Zero->hasOneUse());
+  EXPECT_FALSE(Zero->hasOneUser());
+  EXPECT_FALSE(Zero->hasNUses(1));
+  EXPECT_FALSE(Zero->hasNUsesOrMore(1));
+  EXPECT_FALSE(Zero->hasNUses(2));
+  EXPECT_FALSE(Zero->hasNUsesOrMore(2));
+
+  std::unique_ptr<Module> M(new Module("MyModule", Context));
+
+  // Introduce some uses
+  new GlobalVariable(*M, Int32Ty, /*isConstant=*/false,
+                     GlobalValue::ExternalLinkage, /*Initializer=*/Zero,
+                     "gv_user0");
+  new GlobalVariable(*M, Int32Ty, /*isConstant=*/false,
+                     GlobalValue::ExternalLinkage, /*Initializer=*/Zero,
+                     "gv_user1");
+
+  // Still looks like use_empty with uses.
+  EXPECT_TRUE(Zero->use_empty());
+  EXPECT_EQ(Zero->getNumUses(), 0u);
+  EXPECT_FALSE(Zero->hasOneUse());
+  EXPECT_FALSE(Zero->hasOneUser());
+  EXPECT_FALSE(Zero->hasNUses(1));
+  EXPECT_FALSE(Zero->hasNUsesOrMore(1));
+  EXPECT_FALSE(Zero->hasNUses(2));
+  EXPECT_FALSE(Zero->hasNUsesOrMore(2));
+}
+
 TEST(ConstantsTest, Integer_i1) {
   LLVMContext Context;
   IntegerType *Int1 = IntegerType::get(Context, 1);

[nikic]

Looks like there are some polly assertion failures.

Wrong PR, but this one has other test failures as well.

[nikic]

The general approach here makes sense to me.

For reference, this is the diff for both PRs together, which is a bit clearer as the second undoes half of the first: main...users/arsenm/ir/remove-constantdata-reference-counts

[nikic]

Compile-time for both PRs taken together looks good: https://llvm-compile-time-tracker.com/compare.php?from=12a31658ea36cda74157c6b4e6b6c031e39a19c0&to=d769267079777e4fa4cf41188bfe2aab1b9361f1&stat=instructions%3Au

(For reference, this is for the first PR only: https://llvm-compile-time-tracker.com/compare.php?from=12a31658ea36cda74157c6b4e6b6c031e39a19c0&to=88d6d482c3210c7684d9085c5a94bbc0010f455e&stat=instructions:u)

[nikic]

LGTM

[nikic]

Why the UseList || here? Isn't hasUseList() sufficient? Or is this just an optimization?

[arsenm]

hasUseList is sufficient, not sure if this is really an optimization. I was going to remove this after to see if it makes a difference

[arsenm]

Merge activity

• May 6, 11:13 AM EDT: A user started a stack merge that includes this pull request via Graphite.
• May 6, 11:21 AM EDT: Graphite rebased this pull request as part of a merge.
• May 6, 11:24 AM EDT: @arsenm merged this pull request with Graphite.