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

[John McCall 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.
----------------
rjmccall 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.

Unfortunately, the combination tested for here — where neither `useZeroLengthBitfieldAlignment()` nor `useBitFieldTypeAlignment()` is set — does cause struct layout to not even byte-align after a zero-width bit-field.  There are no targets that clear both bits by default, but one of them can be cleared with a flag, and I do know that we have some code at Apple that builds in that mode.  It's possible that we can break compatibility, but that's unconfirmed, so for now we should maintain existing behavior.

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