[Bitfields][NFC] Make sure bitfields are contiguous

[courbet]

llvm/include/llvm/IR/Instruction.h

70

The curretn naming does not make it immediately obvious what is a generic type (e.g. AlignmentBitfieldElement, BoolBitfieldElement) from an instantiation of this type with specific semantics (e.g. AlignmentField, SwiftErrorField). I think just appending a T or Type would make things more obvious: AlignmentBitfieldElementT, BoolBitfieldElementT.

llvm/include/llvm/IR/Instructions.h

63–68

What would you think of:

using AlignmentField = AlignmentBitfieldElement<0>;          // Next bit:5
using UsedWithInAllocaField = BoolBitfieldElement<AlignmentField::next>;        // Next bit:6
using SwiftErrorField = BoolBitfieldElement<UsedWithInAllocaField::next>;              // Next bit:7

[gchatelet]

• Address comments

[gchatelet]

• Various nits

[courbet]

llvm/include/llvm/IR/InstrTypes.h

1098–1099

here too ?

[gchatelet]

rebase

llvm/include/llvm/ADT/Bitfields.h

struct Bitfield {
   struct Element {
     using Type = T;
     using IntegerType =
         typename bitfields_details::ResolveUnderlyingType<T>::type;
     static constexpr unsigned Shift = Offset;
     static constexpr unsigned Bits = Size;
     static constexpr unsigned FirstBit = Offset;
     static constexpr unsigned LastBit = Shift + Bits - 1;
+    static constexpr unsigned NextBit = Shift + Bits;
 
   private:
     template <typename, typename> friend struct bitfields_details::Impl;
 
     static_assert(Bits > 0, "Bits must be non zero");
     static constexpr size_t TypeBits = sizeof(IntegerType) * CHAR_BIT;
     static_assert(Bits <= TypeBits, "Bits may not be greater than T size");
     static_assert(!std::is_enum<T>::value || MaxValue != T(0),
static void set(StorageType &Packed, typename Bitfield::Type Value) {
     using I = bitfields_details::Impl<Bitfield, StorageType>;
     I::update(Packed, static_cast<typename Bitfield::IntegerType>(Value));
   }
 
   /// Returns whether the two bitfields share common bits.
   template <typename A, typename B> static constexpr bool isOverlapping() {
     return A::LastBit >= B::FirstBit && B::LastBit >= A::FirstBit;
   }
+
+  template <typename A> static constexpr bool areContiguous() { return true; }
+  template <typename A, typename B, typename... Others>
+  static constexpr bool areContiguous() {
+    return A::NextBit == B::FirstBit && areContiguous<B, Others...>();
+  }
 };
 
 } // namespace llvm
 
 #endif // LLVM_ADT_BITFIELDS_H

llvm/include/llvm/IR/InstrTypes.h

enum Predicate : unsigned {
     ICMP_SGE = 39, ///< signed greater or equal
     ICMP_SLT = 40, ///< signed less than
     ICMP_SLE = 41, ///< signed less or equal
     FIRST_ICMP_PREDICATE = ICMP_EQ,
     LAST_ICMP_PREDICATE = ICMP_SLE,
     BAD_ICMP_PREDICATE = ICMP_SLE + 1
   };
   using PredicateField =
-      Bitfield::Element<Predicate, 0, 6, LAST_ICMP_PREDICATE>; // Next bit:6
+      Bitfield::Element<Predicate, 0, 6, LAST_ICMP_PREDICATE>;
 
 protected:
   CmpInst(Type *ty, Instruction::OtherOps op, Predicate pred,
           Value *LHS, Value *RHS, const Twine &Name = "",
           Instruction *InsertBefore = nullptr,
           Instruction *FlagsSource = nullptr);
 
   CmpInst(Type *ty, Instruction::OtherOps op, Predicate pred,

 ///   bundle,
 /// - Zero or more subclass controlled operands
 /// - The called function.
 ///
 /// This allows this base class to easily access the called function and the
 /// start of the arguments without knowing how many other operands a particular
 /// subclass requires. Note that accessing the end of the argument list isn't
 /// as cheap as most other operations on the base class.
 class CallBase : public Instruction {
-  // The first two bits are reserved by CallInst for fast retrieving,
-  using CallInstReservedField = Bitfield::Element<unsigned, 0, 2>; // Next bit:2
-  using CallingConvField = Bitfield::Element<CallingConv::ID, 2, 10,
-                                             CallingConv::MaxID>; // Next bit:12
-
 protected:
+  // The first two bits are reserved by CallInst for fast retrieval,
+  using CallInstReservedField = Bitfield::Element<unsigned, 0, 2>;
+  using CallingConvField =
+      Bitfield::Element<CallingConv::ID, CallInstReservedField::NextBit, 10,
+                        CallingConv::MaxID>;
+  static_assert(
+      Bitfield::areContiguous<CallInstReservedField, CallingConvField>(),
+      "Bitfields must be contiguous");
+
   /// The last operand is the called operand.
   static constexpr int CalledOperandOpEndIdx = -1;
 
   AttributeList Attrs; ///< parameter attributes for callable
   FunctionType *FTy;
 
   template <class... ArgsTy>
   CallBase(AttributeList const &A, FunctionType *FT, ArgsTy &&... Args)

llvm/include/llvm/IR/Instruction.h

 #include "llvm/ADT/Bitfields.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/ilist_node.h"
 #include "llvm/IR/DebugLoc.h"
 #include "llvm/IR/SymbolTableListTraits.h"
 #include "llvm/IR/User.h"
 #include "llvm/IR/Value.h"
+#include "llvm/Support/AtomicOrdering.h"
 #include "llvm/Support/Casting.h"
 #include <algorithm>
 #include <cassert>
 #include <cstdint>
 #include <utility>
 
 namespace llvm {
 
class Instruction : public User,
 
   /// Relative order of this instruction in its parent basic block. Used for
   /// O(1) local dominance checks between instructions.
   mutable unsigned Order = 0;
 
 protected:
   // The 15 first bits of `Value::SubclassData` are available for subclasses of
   // `Instruction` to use.
-  using OpaqueField = Bitfield::Element<uint16_t, 0, 15>; // Next bit:15
+  using OpaqueField = Bitfield::Element<uint16_t, 0, 15>;
 
   // Template alias so that all Instruction storing alignment use the same
   // definiton.
   // Valid alignments are powers of two from 2^0 to 2^MaxAlignmentExponent =
   // 2^29. We store them as Log2(Alignment), so we need 5 bits to encode the 30
   // possible values.
   template <unsigned Offset>
-  using AlignmentBitfieldElement =
+  using AlignmentBitfieldElementT =
       typename Bitfield::Element<unsigned, Offset, 5,
                                  Value::MaxAlignmentExponent>;
 
+  template <unsigned Offset>
+  using BoolBitfieldElementT = typename Bitfield::Element<bool, Offset, 1>;
+
+  template <unsigned Offset>
+  using AtomicOrderingBitfieldElementT =
+      typename Bitfield::Element<AtomicOrdering, Offset, 3,
+                                 AtomicOrdering::LAST>;
+
 private:
   // The last bit is used to store whether the instruction has metadata attached
   // or not.
   using HasMetadataField = Bitfield::Element<bool, 15, 1>;
 
 protected:
   ~Instruction(); // Use deleteValue() to delete a generic Instruction.
 

llvm/include/llvm/IR/Instructions.h

 //===----------------------------------------------------------------------===//
 //                                AllocaInst Class
 //===----------------------------------------------------------------------===//
 
 /// an instruction to allocate memory on the stack
 class AllocaInst : public UnaryInstruction {
   Type *AllocatedType;
 
-  using AlignmentField = AlignmentBitfieldElement<0>;          // Next bit:5
-  using UsedWithInAllocaField = Bitfield::Element<bool, 5, 1>; // Next bit:6
-  using SwiftErrorField = Bitfield::Element<bool, 6, 1>;       // Next bit:7
+  using AlignmentField = AlignmentBitfieldElementT<0>;
+  using UsedWithInAllocaField = BoolBitfieldElementT<AlignmentField::NextBit>;
+  using SwiftErrorField = BoolBitfieldElementT<UsedWithInAllocaField::NextBit>;
+  static_assert(Bitfield::areContiguous<AlignmentField, UsedWithInAllocaField,
+                                        SwiftErrorField>(),
+                "Bitfields must be contiguous");
 
 protected:
   // Note: Instruction needs to be a friend here to call cloneImpl.
   friend class Instruction;
 
   AllocaInst *cloneImpl() const;
 
 public:

 
 //===----------------------------------------------------------------------===//
 //                                LoadInst Class
 //===----------------------------------------------------------------------===//
 
 /// An instruction for reading from memory. This uses the SubclassData field in
 /// Value to store whether or not the load is volatile.
 class LoadInst : public UnaryInstruction {
-  using VolatileField = Bitfield::Element<bool, 0, 1>;      // Next bit:1
-  using AlignmentField = AlignmentBitfieldElement<1>;       // Next bit:6
-  using OrderingField = Bitfield::Element<AtomicOrdering, 6, 3,
-                                          AtomicOrdering::LAST>; // Next bit:9
+  using VolatileField = BoolBitfieldElementT<0>;
+  using AlignmentField = AlignmentBitfieldElementT<VolatileField::NextBit>;
+  using OrderingField = AtomicOrderingBitfieldElementT<AlignmentField::NextBit>;
+  static_assert(
+      Bitfield::areContiguous<VolatileField, AlignmentField, OrderingField>(),
+      "Bitfields must be contiguous");
 
   void AssertOK();
 
 protected:
   // Note: Instruction needs to be a friend here to call cloneImpl.
   friend class Instruction;
 
   LoadInst *cloneImpl() const;

 };
 
 //===----------------------------------------------------------------------===//
 //                                StoreInst Class
 //===----------------------------------------------------------------------===//
 
 /// An instruction for storing to memory.
 class StoreInst : public Instruction {
-  using VolatileField = Bitfield::Element<bool, 0, 1>;      // Next bit:1
-  using AlignmentField = AlignmentBitfieldElement<1>;       // Next bit:6
-  using OrderingField = Bitfield::Element<AtomicOrdering, 6, 3,
-                                          AtomicOrdering::LAST>; // Next bit:9
+  using VolatileField = BoolBitfieldElementT<0>;
+  using AlignmentField = AlignmentBitfieldElementT<VolatileField::NextBit>;
+  using OrderingField = AtomicOrderingBitfieldElementT<AlignmentField::NextBit>;
+  static_assert(
+      Bitfield::areContiguous<VolatileField, AlignmentField, OrderingField>(),
+      "Bitfields must be contiguous");
 
   void AssertOK();
 
 protected:
   // Note: Instruction needs to be a friend here to call cloneImpl.
   friend class Instruction;
 
   StoreInst *cloneImpl() const;

 DEFINE_TRANSPARENT_OPERAND_ACCESSORS(StoreInst, Value)
 
 //===----------------------------------------------------------------------===//
 //                                FenceInst Class
 //===----------------------------------------------------------------------===//
 
 /// An instruction for ordering other memory operations.
 class FenceInst : public Instruction {
-  using OrderingField = Bitfield::Element<AtomicOrdering, 1, 3,
-                                          AtomicOrdering::LAST>; // Next bit:4
+  using OrderingField = AtomicOrderingBitfieldElementT<0>;
 
   void Init(AtomicOrdering Ordering, SyncScope::ID SSID);
 
 protected:
   // Note: Instruction needs to be a friend here to call cloneImpl.
   friend class Instruction;
 
   FenceInst *cloneImpl() const;
AtomicCmpXchgInst(Value *Ptr, Value *Cmp, Value *NewVal, Align Alignment,
                     AtomicOrdering FailureOrdering, SyncScope::ID SSID,
                     BasicBlock *InsertAtEnd);
 
   // allocate space for exactly three operands
   void *operator new(size_t s) {
     return User::operator new(s, 3);
   }
 
-  using VolatileField = Bitfield::Element<bool, 0, 1>;           // Next bit:1
-  using WeakField = Bitfield::Element<bool, 1, 1>;               // Next bit:2
-  using SuccessOrderingField = AtomicOrderingBitfieldElement<2>; // Next bit:5
-  using FailureOrderingField = AtomicOrderingBitfieldElement<5>; // Next bit:8
-  using AlignmentField = AlignmentBitfieldElement<8>;            // Next bit:13
+  using VolatileField = BoolBitfieldElementT<0>;
+  using WeakField = BoolBitfieldElementT<VolatileField::NextBit>;
+  using SuccessOrderingField =
+      AtomicOrderingBitfieldElementT<WeakField::NextBit>;
+  using FailureOrderingField =
+      AtomicOrderingBitfieldElementT<SuccessOrderingField::NextBit>;
+  using AlignmentField =
+      AlignmentBitfieldElementT<FailureOrderingField::NextBit>;
+  static_assert(
+      Bitfield::areContiguous<VolatileField, WeakField, SuccessOrderingField,
+                              FailureOrderingField, AlignmentField>(),
+      "Bitfields must be contiguous");
 
   /// Return the alignment of the memory that is being allocated by the
   /// instruction.
   Align getAlign() const {
     return Align(1ULL << getSubclassData<AlignmentField>());
   }
 
   void setAlignment(Align Align) {
AtomicRMWInst(BinOp Operation, Value *Ptr, Value *Val, Align Alignment,
                 AtomicOrdering Ordering, SyncScope::ID SSID,
                 BasicBlock *InsertAtEnd);
 
   // allocate space for exactly two operands
   void *operator new(size_t s) {
     return User::operator new(s, 2);
   }
 
-  using VolatileField = Bitfield::Element<bool, 0, 1>;          // Next bit:1
-  using AtomicOrderingField = AtomicOrderingBitfieldElement<1>; // Next bit:4
-  using OperationField = BinOpBitfieldElement<4>;               // Next bit:8
-  using AlignmentField = AlignmentBitfieldElement<8>;           // Next bit:13
+  using VolatileField = BoolBitfieldElementT<0>;
+  using AtomicOrderingField =
+      AtomicOrderingBitfieldElementT<VolatileField::NextBit>;
+  using OperationField = BinOpBitfieldElement<AtomicOrderingField::NextBit>;
+  using AlignmentField = AlignmentBitfieldElementT<OperationField::NextBit>;
+  static_assert(Bitfield::areContiguous<VolatileField, AtomicOrderingField,
+                                        OperationField, AlignmentField>(),
+                "Bitfields must be contiguous");
 
   BinOp getOperation() const { return getSubclassData<OperationField>(); }
 
   static StringRef getOperationName(BinOp Op);
 
   static bool isFPOperation(BinOp Op) {
     switch (Op) {
     case AtomicRMWInst::FAdd:
enum TailCallKind : unsigned {
     TCK_None = 0,
     TCK_Tail = 1,
     TCK_MustTail = 2,
     TCK_NoTail = 3,
     TCK_LAST = TCK_NoTail
   };
 
   using TailCallKindField = Bitfield::Element<TailCallKind, 0, 2, TCK_LAST>;
+  static_assert(
+      Bitfield::areContiguous<TailCallKindField, CallBase::CallingConvField>(),
+      "Bitfields must be contiguous");
 
   TailCallKind getTailCallKind() const {
     return getSubclassData<TailCallKindField>();
   }
 
   bool isTailCall() const {
     TailCallKind Kind = getTailCallKind();
     return Kind == TCK_Tail || Kind == TCK_MustTail;

 /// The landingpad instruction holds all of the information
 /// necessary to generate correct exception handling. The landingpad instruction
 /// cannot be moved from the top of a landing pad block, which itself is
 /// accessible only from the 'unwind' edge of an invoke. This uses the
 /// SubclassData field in Value to store whether or not the landingpad is a
 /// cleanup.
 ///
 class LandingPadInst : public Instruction {
-  using CleanupField = Bitfield::Element<bool, 0, 1>;
+  using CleanupField = BoolBitfieldElementT<0>;
 
   /// The number of operands actually allocated.  NumOperands is
   /// the number actually in use.
   unsigned ReservedSpace;
 
   LandingPadInst(const LandingPadInst &LP);
 
 public:

 };
 
 DEFINE_TRANSPARENT_OPERAND_ACCESSORS(ResumeInst, Value)
 
 //===----------------------------------------------------------------------===//
 //                         CatchSwitchInst Class
 //===----------------------------------------------------------------------===//
 class CatchSwitchInst : public Instruction {
-  using UnwindDestField = Bitfield::Element<unsigned, 0, 1>; // Next bit:1
+  using UnwindDestField = BoolBitfieldElementT<0>;
 
   /// The number of operands actually allocated.  NumOperands is
   /// the number actually in use.
   unsigned ReservedSpace;
 
   // Operand[0] = Outer scope
   // Operand[1] = Unwind block destination
   // Operand[n] = BasicBlock to go to on match

 
 DEFINE_TRANSPARENT_OPERAND_ACCESSORS(CatchReturnInst, Value)
 
 //===----------------------------------------------------------------------===//
 //                               CleanupReturnInst Class
 //===----------------------------------------------------------------------===//
 
 class CleanupReturnInst : public Instruction {
-  using UnwindDestField = Bitfield::Element<unsigned, 0, 1>; // Next bit:1
+  using UnwindDestField = BoolBitfieldElementT<0>;
+
 private:
   CleanupReturnInst(const CleanupReturnInst &RI);
   CleanupReturnInst(Value *CleanupPad, BasicBlock *UnwindBB, unsigned Values,
                     Instruction *InsertBefore = nullptr);
   CleanupReturnInst(Value *CleanupPad, BasicBlock *UnwindBB, unsigned Values,
                     BasicBlock *InsertAtEnd);
 
   void init(Value *CleanupPad, BasicBlock *UnwindBB);

llvm/unittests/ADT/BitFieldsTest.cpp

TEST(BitfieldsTest, isOverlapping) {
   EXPECT_TRUE((Bitfield::isOverlapping<A, B>()));
   EXPECT_TRUE((Bitfield::isOverlapping<A, D>()));
 
   EXPECT_TRUE((Bitfield::isOverlapping<B, C>()));
   EXPECT_TRUE((Bitfield::isOverlapping<B, D>()));
   EXPECT_FALSE((Bitfield::isOverlapping<C, D>()));
 }
 
+TEST(BitfieldsTest, areContiguous) {
+  using A = Bitfield::Element<unsigned, 0, 1>; // Next Bit:1
+  using B = Bitfield::Element<unsigned, 1, 4>; // Next Bit:5
+  using C = Bitfield::Element<unsigned, 5, 3>; // Next Bit:8
+  EXPECT_TRUE((Bitfield::areContiguous<A, B>()));
+  EXPECT_TRUE((Bitfield::areContiguous<A, B, C>()));
+
+  EXPECT_FALSE((Bitfield::areContiguous<A, C>()));
+  EXPECT_FALSE((Bitfield::areContiguous<A, A>()));
+  EXPECT_FALSE((Bitfield::areContiguous<B, A>()));
+}
+
 TEST(BitfieldsTest, FullUint64) {
   uint64_t Storage = 0;
   using Value = Bitfield::Element<uint64_t, 0, 64>;
   Bitfield::set<Value>(Storage, -1ULL);
   EXPECT_EQ(Bitfield::get<Value>(Storage), -1ULL);
   Bitfield::set<Value>(Storage, 0ULL);
   EXPECT_EQ(Bitfield::get<Value>(Storage), 0ULL);
 }