[llvm] [IR] Replace blockaddress refcount with single flag (PR #138239)

[via llvm-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-llvm-ir

Author: Nikita Popov (nikic)

<details>
<summary>Changes</summary>

BasicBlock currently has a blockaddress refcount. However, blockaddresses are uniqued, which means that there can only be a single blockaddress for a given BasicBlock. Prior to #<!-- -->137958 there were some edge case exceptions to this, but now it always holds. As such, replace the refcount with a single flag.

As we're now just tracking two bits, I've dropped the BasicBlockBits structure, as I found it more annoying than useful (esp with the weird AIX workarounds). Instead handle the subclass data the same way we do in Operators.

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


3 Files Affected:

- (modified) llvm/include/llvm/IR/BasicBlock.h (+21-62) 
- (modified) llvm/lib/IR/BasicBlock.cpp (+1-3) 
- (modified) llvm/lib/IR/Constants.cpp (+4-4) 


``````````diff
diff --git a/llvm/include/llvm/IR/BasicBlock.h b/llvm/include/llvm/IR/BasicBlock.h
index c9169cb601809..9ee0bacb5c258 100644
--- a/llvm/include/llvm/IR/BasicBlock.h
+++ b/llvm/include/llvm/IR/BasicBlock.h
@@ -564,6 +564,24 @@ class BasicBlock final : public Value, // Basic blocks are data objects also
                            BasicBlock::iterator FromBeginIt,
                            BasicBlock::iterator FromEndIt);
 
+  enum {
+    HasAddressTaken = 1 << 0,
+    InstrOrderValid = 1 << 1,
+  };
+
+  void setHasAddressTaken(bool B) {
+    if (B)
+      SubclassOptionalData |= HasAddressTaken;
+    else
+      SubclassOptionalData &= ~HasAddressTaken;
+  }
+
+  /// Shadow Value::setValueSubclassData with a private forwarding method so
+  /// that any future subclasses cannot accidentally use it.
+  void setValueSubclassData(unsigned short D) {
+    Value::setValueSubclassData(D);
+  }
+
 public:
   /// Returns a pointer to the symbol table if one exists.
   ValueSymbolTable *getValueSymbolTable();
@@ -669,7 +687,7 @@ class BasicBlock final : public Value, // Basic blocks are data objects also
   /// Returns true if there are any uses of this basic block other than
   /// direct branches, switches, etc. to it.
   bool hasAddressTaken() const {
-    return getBasicBlockBits().BlockAddressRefCount != 0;
+    return SubclassOptionalData & HasAddressTaken;
   }
 
   /// Update all phi nodes in this basic block to refer to basic block \p New
@@ -711,15 +729,13 @@ class BasicBlock final : public Value, // Basic blocks are data objects also
 
   /// Returns true if the Order field of child Instructions is valid.
   bool isInstrOrderValid() const {
-    return getBasicBlockBits().InstrOrderValid;
+    return SubclassOptionalData & InstrOrderValid;
   }
 
   /// Mark instruction ordering invalid. Done on every instruction insert.
   void invalidateOrders() {
     validateInstrOrdering();
-    BasicBlockBits Bits = getBasicBlockBits();
-    Bits.InstrOrderValid = false;
-    setBasicBlockBits(Bits);
+    SubclassOptionalData &= ~InstrOrderValid;
   }
 
   /// Renumber instructions and mark the ordering as valid.
@@ -734,63 +750,6 @@ class BasicBlock final : public Value, // Basic blocks are data objects also
   /// each ordering to ensure that transforms have the same algorithmic
   /// complexity when asserts are enabled as when they are disabled.
   void validateInstrOrdering() const;
-
-private:
-#if defined(_AIX) && (!defined(__GNUC__) || defined(__clang__))
-// Except for GCC; by default, AIX compilers store bit-fields in 4-byte words
-// and give the `pack` pragma push semantics.
-#define BEGIN_TWO_BYTE_PACK() _Pragma("pack(2)")
-#define END_TWO_BYTE_PACK() _Pragma("pack(pop)")
-#else
-#define BEGIN_TWO_BYTE_PACK()
-#define END_TWO_BYTE_PACK()
-#endif
-
-  BEGIN_TWO_BYTE_PACK()
-  /// Bitfield to help interpret the bits in Value::SubclassData.
-  struct BasicBlockBits {
-    unsigned short BlockAddressRefCount : 15;
-    unsigned short InstrOrderValid : 1;
-  };
-  END_TWO_BYTE_PACK()
-
-#undef BEGIN_TWO_BYTE_PACK
-#undef END_TWO_BYTE_PACK
-
-  /// Safely reinterpret the subclass data bits to a more useful form.
-  BasicBlockBits getBasicBlockBits() const {
-    static_assert(sizeof(BasicBlockBits) == sizeof(unsigned short),
-                  "too many bits for Value::SubclassData");
-    unsigned short ValueData = getSubclassDataFromValue();
-    BasicBlockBits AsBits;
-    memcpy(&AsBits, &ValueData, sizeof(AsBits));
-    return AsBits;
-  }
-
-  /// Reinterpret our subclass bits and store them back into Value.
-  void setBasicBlockBits(BasicBlockBits AsBits) {
-    unsigned short D;
-    memcpy(&D, &AsBits, sizeof(D));
-    Value::setValueSubclassData(D);
-  }
-
-  /// Increment the internal refcount of the number of BlockAddresses
-  /// referencing this BasicBlock by \p Amt.
-  ///
-  /// This is almost always 0, sometimes one possibly, but almost never 2, and
-  /// inconceivably 3 or more.
-  void AdjustBlockAddressRefCount(int Amt) {
-    BasicBlockBits Bits = getBasicBlockBits();
-    Bits.BlockAddressRefCount += Amt;
-    setBasicBlockBits(Bits);
-    assert(Bits.BlockAddressRefCount < 255 && "Refcount wrap-around");
-  }
-
-  /// Shadow Value::setValueSubclassData with a private forwarding method so
-  /// that any future subclasses cannot accidentally use it.
-  void setValueSubclassData(unsigned short D) {
-    Value::setValueSubclassData(D);
-  }
 };
 
 // Create wrappers for C Binding types (see CBindingWrapping.h).
diff --git a/llvm/lib/IR/BasicBlock.cpp b/llvm/lib/IR/BasicBlock.cpp
index c632b1b2dc2ab..b4c16447bc686 100644
--- a/llvm/lib/IR/BasicBlock.cpp
+++ b/llvm/lib/IR/BasicBlock.cpp
@@ -732,9 +732,7 @@ void BasicBlock::renumberInstructions() {
     I.Order = Order++;
 
   // Set the bit to indicate that the instruction order valid and cached.
-  BasicBlockBits Bits = getBasicBlockBits();
-  Bits.InstrOrderValid = true;
-  setBasicBlockBits(Bits);
+  SubclassOptionalData |= InstrOrderValid;
 
   NumInstrRenumberings++;
 }
diff --git a/llvm/lib/IR/Constants.cpp b/llvm/lib/IR/Constants.cpp
index 49107a421149e..d7c847e541052 100644
--- a/llvm/lib/IR/Constants.cpp
+++ b/llvm/lib/IR/Constants.cpp
@@ -1911,7 +1911,7 @@ BlockAddress *BlockAddress::get(Function *F, BasicBlock *BB) {
 BlockAddress::BlockAddress(Type *Ty, BasicBlock *BB)
     : Constant(Ty, Value::BlockAddressVal, AllocMarker) {
   setOperand(0, BB);
-  BB->AdjustBlockAddressRefCount(1);
+  BB->setHasAddressTaken(true);
 }
 
 BlockAddress *BlockAddress::lookup(const BasicBlock *BB) {
@@ -1926,7 +1926,7 @@ BlockAddress *BlockAddress::lookup(const BasicBlock *BB) {
 /// Remove the constant from the constant table.
 void BlockAddress::destroyConstantImpl() {
   getType()->getContext().pImpl->BlockAddresses.erase(getBasicBlock());
-  getBasicBlock()->AdjustBlockAddressRefCount(-1);
+  getBasicBlock()->setHasAddressTaken(false);
 }
 
 Value *BlockAddress::handleOperandChangeImpl(Value *From, Value *To) {
@@ -1939,14 +1939,14 @@ Value *BlockAddress::handleOperandChangeImpl(Value *From, Value *To) {
   if (NewBA)
     return NewBA;
 
-  getBasicBlock()->AdjustBlockAddressRefCount(-1);
+  getBasicBlock()->setHasAddressTaken(false);
 
   // Remove the old entry, this can't cause the map to rehash (just a
   // tombstone will get added).
   getContext().pImpl->BlockAddresses.erase(getBasicBlock());
   NewBA = this;
   setOperand(0, NewBB);
-  getBasicBlock()->AdjustBlockAddressRefCount(1);
+  getBasicBlock()->setHasAddressTaken(true);
 
   // If we just want to keep the existing value, then return null.
   // Callers know that this means we shouldn't delete this value.

``````````

</details>


https://github.com/llvm/llvm-project/pull/138239