SIL: improve inline bitfields in SILNode, SILBasicBlock and Operand

* Let the customBits and lastInitializedBitfieldID share a single uint64_t. This increases the number of available bits in SILNode and Operand from 8 to 20. Also, it simplifies the Operand class because no PointerIntPairs are used anymore to store the operand pointer fields.
* Instead make the "deleted" flag a separate bool field in SILNode (instead of encoding it with the sign of lastInitializedBitfieldID). Another simplification
* Enable important invariant checks also in release builds by using `require` instead of `assert`. Not catching such errors in release builds would be a disaster.
* Let the Swift optimization passes use all the available bits and not only a fixed amount of 8 (SILNode) and 16 (SILBasicBlock).

Author: eeckstein

include/swift/SIL/SILBasicBlock.h

@@ -130,16 +130,13 @@ public SwiftObjectHeader {
   ///    DD and EEE are uninitialized
   ///
   /// See also: SILBitfield::bitfieldID, SILFunction::currentBitfieldID.
-  int64_t lastInitializedBitfieldID = 0;
+  uint64_t lastInitializedBitfieldID = 0;
 
   // Used by `BasicBlockBitfield`.
   unsigned getCustomBits() const { return customBits; }
   // Used by `BasicBlockBitfield`.
   void setCustomBits(unsigned value) { customBits = value; }
 
-  // Used by `BasicBlockBitfield`.
-  enum { numCustomBits = std::numeric_limits<CustomBitsType>::digits };
-
   friend struct llvm::ilist_traits<SILBasicBlock>;
 
   SILBasicBlock();
@@ -155,7 +152,8 @@ public SwiftObjectHeader {
 
   ~SILBasicBlock();
 
-  bool isMarkedAsDeleted() const { return lastInitializedBitfieldID < 0; }
+  enum { numCustomBits = std::numeric_limits<CustomBitsType>::digits };
+  enum { maxBitfieldID = std::numeric_limits<uint64_t>::max() };
 
   /// Gets the ID (= index in the function's block list) of the block.
   ///

include/swift/SIL/SILBitfield.h

@@ -17,6 +17,7 @@
 #ifndef SWIFT_SIL_SILBITFIELD_H
 #define SWIFT_SIL_SILBITFIELD_H
 
+#include "swift/Basic/Require.h"
 #include "swift/SIL/SILFunction.h"
 
 namespace swift {
@@ -30,7 +31,7 @@ template <class Impl, class T> class SILBitfield {
   /// that the bits of that block are not initialized yet.
   /// See also: SILBasicBlock::lastInitializedBitfieldID,
   ///           SILFunction::currentBitfieldID
-  int64_t bitfieldID;
+  uint64_t bitfieldID;
 
   short startBit;
   short endBit;
@@ -55,11 +56,13 @@ template <class Impl, class T> class SILBitfield {
       parent(parent),
       function(function) {
     assert(size > 0 && "bit field size must be > 0");
-    assert(endBit <= T::numCustomBits && "too many/large bit fields allocated in function");
+    require(endBit <= T::numCustomBits,
+            "too many/large bit fields allocated in function");
     assert((!parent || bitfieldID > parent->bitfieldID) &&
            "BasicBlockBitfield indices are not in order");
+    require(function->currentBitfieldID < T::maxBitfieldID,
+            "currentBitfieldID overflow");
     ++function->currentBitfieldID;
-    assert(function->currentBitfieldID != 0 && "currentBitfieldID overflow");
   }
 
   SILBitfield(const SILBitfield &) = delete;
@@ -85,9 +88,6 @@ template <class Impl, class T> class SILBitfield {
     unsigned clearMask = mask;
     if (bitfieldID > entity->lastInitializedBitfieldID) {
 
-      if (entity->isMarkedAsDeleted())
-        return;
-
       // The bitfield is not initialized yet in this block.
       // Initialize the bitfield, and also initialize all parent bitfields,
       // which are not initialized, yet. Example:

include/swift/SIL/SILFunction.h

@@ -294,7 +294,7 @@ class SILFunction
   /// A monotonically increasing ID which is incremented whenever a
   /// BasicBlockBitfield, NodeBitfield, or OperandBitfield is constructed.  For
   /// details see SILBitfield::bitfieldID;
-  int64_t currentBitfieldID = 1;
+  uint64_t currentBitfieldID = 1;
 
   /// Unique identifier for vector indexing and deterministic sorting.
   /// May be reused when zombie functions are recovered.

include/swift/SIL/SILNode.h

@@ -126,6 +126,9 @@ class alignas(8) SILNode :
   enum { NumAllocRefTailTypesBits = 4 };
   enum { NumMarkDependenceKindBits = 2 };
 
+  enum { numCustomBits = 20 };
+  enum { maxBitfieldID = std::numeric_limits<uint64_t>::max() >> numCustomBits };
+
 protected:
   friend class SILInstruction;
   template <class, class> friend class SILBitfield;
@@ -135,11 +138,7 @@ class alignas(8) SILNode :
 
   uint8_t kind;
 
-  // Used by `NodeBitfield`.
-  enum { numCustomBits = 8 };
-
-  // Used by `NodeBitfield`.
-  uint8_t customBits;
+  bool deleted = false;
 
   // Part of SILInstruction's debug location. Together with
   // `SILInstruction::locationStorage` this forms the SILLocation.
@@ -364,6 +363,9 @@ class alignas(8) SILNode :
 
   //===---------------------- end of shared fields ------------------------===//
 
+  // Used by `NodeBitfield`.
+  uint64_t customBits : numCustomBits;
+
   /// The NodeBitfield ID of the last initialized bitfield in `customBits`.
   /// Example:
   ///
@@ -377,20 +379,19 @@ class alignas(8) SILNode :
   /// -> AAA, BB and C are initialized,
   ///    DD and EEE are uninitialized
   ///
-  /// If the ID is negative, it means that the node (in case it's an instruction)
-  /// is deleted, i.e. it does not belong to the function anymore. Conceptually
-  /// this results in setting all bitfields to zero, which e.g. "removes" the
-  /// node from all NodeSets.
+  /// The size of lastInitializedBitfieldID should be more than 32 bits to
+  /// absolutely avoid an overflow.
   ///
   /// See also: SILBitfield::bitfieldID, SILFunction::currentBitfieldID.
-  int64_t lastInitializedBitfieldID = 0;
+  uint64_t lastInitializedBitfieldID : (64 - numCustomBits);
 
 private:
   SwiftMetatype getSILNodeMetatype(SILNodeKind kind);
 
 protected:
   SILNode(SILNodeKind kind) : SwiftObjectHeader(getSILNodeMetatype(kind)),
-                              kind((uint8_t)kind) {
+                              kind((uint8_t)kind),
+                              customBits(0), lastInitializedBitfieldID(0) {
     _sharedUInt8_private.opaque = 0;
     _sharedUInt32_private.opaque = 0;
   }
@@ -442,11 +443,8 @@ class alignas(8) SILNode :
     lastInitializedBitfieldID = 0;
   }
 
-  void markAsDeleted() {
-    lastInitializedBitfieldID = -1;
-  }
-
-  bool isMarkedAsDeleted() const { return lastInitializedBitfieldID < 0; }
+  void markAsDeleted() { deleted = true; }
+  bool isMarkedAsDeleted() const { return deleted; }
 
   static SILNode *instAsNode(SILInstruction *inst);
   static const SILNode *instAsNode(const SILInstruction *inst);

include/swift/SIL/SILValue.h

@@ -1019,38 +1019,40 @@ ValueOwnershipKind::getForwardingOperandOwnership(bool allowUnowned) const {
 /// A formal SIL reference to a value, suitable for use as a stored
 /// operand.
 class Operand {
+public:
+  enum { numCustomBits = 8 };
+  enum { maxBitfieldID = std::numeric_limits<uint64_t>::max() >> numCustomBits };
+
+private:
   template <class, class> friend class SILBitfield;
 
-  /// The value used as this operand combined with three bits we use for
-  /// `customBits`.
-  llvm::PointerIntPair<SILValue, 3> TheValueAndThreeBits = {SILValue(), 0};
+  /// The value used as this operand.
+  SILValue TheValue;
 
-  /// The next operand in the use-chain. Note that the chain holds every use of
-  /// the current ValueBase, not just those of the designated result.
-  ///
-  /// We use 3 bits of the pointer for customBits.
-  llvm::PointerIntPair<Operand *, 3> NextUseAndThreeBits = {nullptr, 0};
+  /// The next operand in the use-chain.  Note that the chain holds
+  /// every use of the current ValueBase, not just those of the
+  /// designated result.
+  Operand *NextUse = nullptr;
 
   /// A back-pointer in the use-chain, required for fast patching
   /// of use-chains.
-  ///
-  /// We use 2 bits of the pointer for customBits.
-  llvm::PointerIntPair<Operand **, 3> BackAndThreeBits = {nullptr, 0};
+  Operand **Back = nullptr;
 
   /// The owner of this operand.
   /// FIXME: this could be space-compressed.
   SILInstruction *Owner;
 
-  /// Used by `OperandBitfield`
-  enum { numCustomBits = 8 };
+  uint64_t customBits : numCustomBits;
 
-  /// Used by `OperandBitfield`
-  int64_t lastInitializedBitfieldID = 0;
+  // For details see SILNode::lastInitializedBitfieldID
+  uint64_t lastInitializedBitfieldID : (64 - numCustomBits);
 
 public:
-  Operand(SILInstruction *owner) : Owner(owner) {}
+  Operand(SILInstruction *owner)
+      : Owner(owner), customBits(0), lastInitializedBitfieldID(0) {}
   Operand(SILInstruction *owner, SILValue theValue)
-      : TheValueAndThreeBits(theValue), Owner(owner) {
+      : TheValue(theValue), Owner(owner),
+        customBits(0), lastInitializedBitfieldID(0) {
     insertIntoCurrent();
   }
 
@@ -1062,14 +1064,14 @@ class Operand {
   Operand &operator=(Operand &&) = default;
 
   /// Return the current value being used by this operand.
-  SILValue get() const { return TheValueAndThreeBits.getPointer(); }
+  SILValue get() const { return TheValue; }
 
   /// Set the current value being used by this operand.
   void set(SILValue newValue) {
     // It's probably not worth optimizing for the case of switching
     // operands on a single value.
     removeFromCurrent();
-    TheValueAndThreeBits.setPointer(newValue);
+    TheValue = newValue;
     insertIntoCurrent();
   }
 
@@ -1083,9 +1085,9 @@ class Operand {
   /// Remove this use of the operand.
   void drop() {
     removeFromCurrent();
-    TheValueAndThreeBits = {SILValue(), 0};
-    NextUseAndThreeBits = {nullptr, 0};
-    BackAndThreeBits = {nullptr, 0};
+    TheValue = SILValue();
+    NextUse = nullptr;
+    Back = nullptr;
     Owner = nullptr;
   }
 
@@ -1097,7 +1099,7 @@ class Operand {
   SILInstruction *getUser() { return Owner; }
   const SILInstruction *getUser() const { return Owner; }
 
-  Operand *getNextUse() const { return NextUseAndThreeBits.getPointer(); }
+  Operand *getNextUse() const { return NextUse; }
 
   /// Return true if this operand is a type dependent operand.
   ///
@@ -1147,63 +1149,36 @@ class Operand {
   SILBasicBlock *getParentBlock() const;
   SILFunction *getParentFunction() const;
 
-  unsigned getCustomBits() const {
-    unsigned bits = 0;
-    bits |= TheValueAndThreeBits.getInt();
-    bits |= NextUseAndThreeBits.getInt() << 3;
-    bits |= BackAndThreeBits.getInt() << 2;
-    return bits;
-  }
-
-  void setCustomBits(unsigned bits) {
-    assert(bits < 256 && "Can only store a byte?!");
-    TheValueAndThreeBits.setInt(bits & 0x7);
-    NextUseAndThreeBits.setInt((bits >> 3) & 0x7);
-    BackAndThreeBits.setInt((bits >> 6) & 0x3);
-  }
+  unsigned getCustomBits() const { return customBits; }
+  void setCustomBits(unsigned bits) {customBits = bits; }
 
   // Called when transferring basic blocks from one function to another.
   void resetBitfields() {
     lastInitializedBitfieldID = 0;
   }
 
-  void markAsDeleted() {
-    lastInitializedBitfieldID = -1;
-  }
-
-  bool isMarkedAsDeleted() const { return lastInitializedBitfieldID < 0; }
-
   SILFunction *getFunction() const;
 
   void print(llvm::raw_ostream &os) const;
   SWIFT_DEBUG_DUMP;
 
 private:
   void removeFromCurrent() {
-    auto *back = getBack();
-    if (!back)
-      return;
-    auto *nextUse = getNextUse();
-    *back = nextUse;
-    if (nextUse)
-      nextUse->setBack(back);
+    if (!Back)
+       return;
+     *Back = NextUse;
+     if (NextUse)
+       NextUse->Back = Back;
   }
 
   void insertIntoCurrent() {
-    auto **firstUse = &get()->FirstUse;
-    setBack(firstUse);
-    setNextUse(*firstUse);
-    if (auto *nextUse = getNextUse())
-      nextUse->setBack(NextUseAndThreeBits.getAddrOfPointer());
-    get()->FirstUse = this;
+    Back = &TheValue->FirstUse;
+    NextUse = TheValue->FirstUse;
+    if (NextUse)
+      NextUse->Back = &NextUse;
+    TheValue->FirstUse = this;
   }
 
-  void setNextUse(Operand *op) { NextUseAndThreeBits.setPointer(op); }
-
-  Operand **getBack() const { return BackAndThreeBits.getPointer(); }
-
-  void setBack(Operand **newValue) { BackAndThreeBits.setPointer(newValue); }
-
   friend class ValueBase;
   friend class ValueBaseUseIterator;
   friend class ConsumingUseIterator;

include/swift/SILOptimizer/PassManager/PassManager.h

@@ -14,6 +14,7 @@
 #include "swift/SIL/InstructionUtils.h"
 #include "swift/SIL/BasicBlockBits.h"
 #include "swift/SIL/NodeBits.h"
+#include "swift/SIL/OperandBits.h"
 #include "swift/SILOptimizer/Analysis/Analysis.h"
 #include "swift/SILOptimizer/PassManager/PassPipeline.h"
 #include "swift/SILOptimizer/PassManager/Passes.h"
@@ -73,12 +74,12 @@ class SwiftPassInvocation {
 
   SILSSAUpdater *ssaUpdater = nullptr;
 
-  static constexpr int BlockSetCapacity = 16;
+  static constexpr int BlockSetCapacity = SILBasicBlock::numCustomBits;
   char blockSetStorage[sizeof(BasicBlockSet) * BlockSetCapacity];
   bool aliveBlockSets[BlockSetCapacity];
   int numBlockSetsAllocated = 0;
 
-  static constexpr int NodeSetCapacity = 8;
+  static constexpr int NodeSetCapacity = SILNode::numCustomBits;
   char nodeSetStorage[sizeof(NodeSet) * NodeSetCapacity];
   bool aliveNodeSets[NodeSetCapacity];
   int numNodeSetsAllocated = 0;

lib/SILOptimizer/PassManager/PassManager.cpp

@@ -1402,8 +1402,8 @@ FixedSizeSlab *SwiftPassInvocation::freeSlab(FixedSizeSlab *slab) {
 }
 
 BasicBlockSet *SwiftPassInvocation::allocBlockSet() {
-  assert(numBlockSetsAllocated < BlockSetCapacity - 1 &&
-         "too many BasicBlockSets allocated");
+  require(numBlockSetsAllocated < BlockSetCapacity,
+          "too many BasicBlockSets allocated");
 
   auto *storage = (BasicBlockSet *)blockSetStorage + numBlockSetsAllocated;
   BasicBlockSet *set = new (storage) BasicBlockSet(function);
@@ -1426,8 +1426,8 @@ void SwiftPassInvocation::freeBlockSet(BasicBlockSet *set) {
 }
 
 NodeSet *SwiftPassInvocation::allocNodeSet() {
-  assert(numNodeSetsAllocated < NodeSetCapacity - 1 &&
-         "too many BasicNodeSets allocated");
+  require(numNodeSetsAllocated < NodeSetCapacity,
+          "too many NodeSets allocated");
 
   auto *storage = (NodeSet *)nodeSetStorage + numNodeSetsAllocated;
   NodeSet *set = new (storage) NodeSet(function);

unittests/SIL/SILBitfieldTest.cpp

@@ -21,20 +21,20 @@ class BasicBlockBitfield;
 
 struct SILFunction {
   BasicBlockBitfield *newestAliveBlockBitfield = nullptr;
-  int64_t currentBitfieldID = 1;
+  uint64_t currentBitfieldID = 1;
 };
 
 struct SILBasicBlock {
   SILFunction *function;
   uint32_t customBits = 0;
-  int64_t lastInitializedBitfieldID = 0;
+  uint64_t lastInitializedBitfieldID = 0;
 
   enum { numCustomBits = 32 };
+  enum { maxBitfieldID = std::numeric_limits<uint64_t>::max() };
 
   SILBasicBlock(SILFunction *function): function(function) {}
 
   SILFunction *getFunction() const { return function; }
-  bool isMarkedAsDeleted() const { return false; }
 
   unsigned getCustomBits() const { return customBits; }
   void setCustomBits(unsigned value) { customBits = value; }