[clang] [clang] Better bitfield access units (PR #65742)

[Eli Friedman via cfe-commits]

================
@@ -394,33 +412,155 @@ void CGRecordLowering::accumulateFields() {
               : getStorageType(*Field),
           *Field));
       ++Field;
-    } else {
-      ++Field;
     }
   }
 }
 
-void
-CGRecordLowering::accumulateBitFields(RecordDecl::field_iterator Field,
-                                      RecordDecl::field_iterator FieldEnd) {
-  // Run stores the first element of the current run of bitfields.  FieldEnd is
-  // used as a special value to note that we don't have a current run.  A
-  // bitfield run is a contiguous collection of bitfields that can be stored in
-  // the same storage block.  Zero-sized bitfields and bitfields that would
-  // cross an alignment boundary break a run and start a new one.
-  RecordDecl::field_iterator Run = FieldEnd;
-  // Tail is the offset of the first bit off the end of the current run.  It's
-  // used to determine if the ASTRecordLayout is treating these two bitfields as
-  // contiguous.  StartBitOffset is offset of the beginning of the Run.
-  uint64_t StartBitOffset, Tail = 0;
+namespace {
+
+// A run of bitfields assigned to the same access unit -- the size of memory
+// loads & stores.
+class BitFieldAccessUnit {
+  RecordDecl::field_iterator Begin; // Field at start of this access unit.
+  RecordDecl::field_iterator End;   // Field just after this access unit.
+
+  CharUnits StartOffset; // Starting offset in the containing record.
+  CharUnits EndOffset;   // Finish offset (exclusive) in the containing record.
+
+  bool ContainsVolatile; // This access unit contains a volatile bitfield.
+
+public:
+  // End barrier constructor.
+  BitFieldAccessUnit(RecordDecl::field_iterator F, CharUnits Offset,
+                     bool Volatile = false)
+      : Begin(F), End(F), StartOffset(Offset), EndOffset(Offset),
+        ContainsVolatile(Volatile) {}
+
+  // Collect contiguous bitfields into an access unit.
+  BitFieldAccessUnit(RecordDecl::field_iterator FieldBegin,
+                     RecordDecl::field_iterator FieldEnd,
+                     const CGRecordLowering &CGRL);
+
+  // Compute the access unit following this one -- which might be a barrier at
+  // Limit.
+  BitFieldAccessUnit accumulateNextUnit(RecordDecl::field_iterator FieldEnd,
+                                        CharUnits Limit,
+                                        const CGRecordLowering &CGRL) const {
+    return end() != FieldEnd ? BitFieldAccessUnit(end(), FieldEnd, CGRL)
+                             : BitFieldAccessUnit(FieldEnd, Limit);
+  }
+  // Re-set the end of this unit if there is space before Probe starts.
+  void enlargeIfSpace(const BitFieldAccessUnit &Probe, CharUnits Offset) {
+    if (Probe.getStartOffset() >= Offset) {
+      End = Probe.begin();
+      EndOffset = Offset;
+    }
+  }
+
+public:
+  RecordDecl::field_iterator begin() const { return Begin; }
+  RecordDecl::field_iterator end() const { return End; }
+
+public:
+  // Accessors
+  CharUnits getSize() const { return EndOffset - StartOffset; }
+  CharUnits getStartOffset() const { return StartOffset; }
+  CharUnits getEndOffset() const { return EndOffset; }
+
+  // Predicates
+  bool isBarrier() const { return getSize().isZero(); }
+  bool hasVolatile() const { return ContainsVolatile; }
+
+  // Create the containing access unit and install the bitfields.
+  void installUnit(CGRecordLowering &CGRL) const {
+    if (!isBarrier()) {
+      // Add the storage member for the access unit to the record. The
+      // bitfields get the offset of their storage but come afterward and remain
+      // there after a stable sort.
+      llvm::Type *Type = CGRL.getIntNType(CGRL.Context.toBits(getSize()));
+      CGRL.Members.push_back(CGRL.StorageInfo(getStartOffset(), Type));
+      for (auto F : *this)
+        if (!F->isZeroLengthBitField(CGRL.Context))
+          CGRL.Members.push_back(CGRecordLowering::MemberInfo(
+              getStartOffset(), CGRecordLowering::MemberInfo::Field, nullptr,
+              F));
+    }
+  }
+};
+
+// Create an access unit of contiguous bitfields.
+BitFieldAccessUnit::BitFieldAccessUnit(RecordDecl::field_iterator FieldBegin,
+                                       RecordDecl::field_iterator FieldEnd,
+                                       const CGRecordLowering &CGRL)
+    : BitFieldAccessUnit(FieldBegin, CharUnits::Zero(),
+                         FieldBegin->getType().isVolatileQualified()) {
+  assert(End != FieldEnd);
+
+  uint64_t StartBit = CGRL.getFieldBitOffset(*FieldBegin);
+  uint64_t BitSize = End->getBitWidthValue(CGRL.Context);
+  unsigned CharBits = CGRL.Context.getCharWidth();
+
+  assert(!(StartBit % CharBits) && "Not at start of char");
+
+  ++End;
+  if (BitSize ||
+      !(CGRL.Context.getTargetInfo().useZeroLengthBitfieldAlignment() ||
+        CGRL.Context.getTargetInfo().useBitFieldTypeAlignment()))
+    // The first field is not a (zero-width) barrier. Collect contiguous fields.
+    for (; End != FieldEnd; ++End) {
+      uint64_t BitOffset = CGRL.getFieldBitOffset(*End);
+      if (End->isZeroLengthBitField(CGRL.Context)) {
+        // A zero-length bitfield might be a barrier between access units.
+        if (CGRL.Context.getTargetInfo().useZeroLengthBitfieldAlignment() ||
+            CGRL.Context.getTargetInfo().useBitFieldTypeAlignment()) {
+          // A zero-length barrier, will be in the next run.
+          assert(!(BitOffset % CharBits) &&
+                 "Barrier bitfield not at byte boundary");
+          break;
+        }
+        // When this isn't a barrier, we want it to be placed at the end of the
+        // current run so that a final zero-length bitfield (why would one do
+        // that?) doesn't get mistaken for a barrier in later processing.
----------------
efriedma-quic wrote:

As far as I know, zero-length bitfields always ensure that bitfields after the zero-length field don't overlap in the same byte as bitfields before the zero-length field.  The cross-target variations are related to other details which aren't relevant here, like the overall alignment of the struct.

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