[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:

We should honor zero-width bit-fields as barriers even on these terrible targets unless we don't have a choice.  That is, as long as the ABI hasn't actually colocated two bit-fields on opposite sides of a zero-width bit-field within the same byte, we should honor the zero-width bit-field and not let accesses interfere across it.

I think that might simplify the basic operation here — you want to accumulate bit-fields until the next bit-field starts at a byte boundary, and on well-behaved ABIs that means you'll never cross a zero-width bit-field, and on poorly-behaved ABIs you'll only cross the zero-width bit-fields that you absolutely have to.

I continue to feel like there's a simpler structure to this code that's trying to get out.  Can you think of this as doing a single pass over the bit-field run, accumulating into the current access unit, and then calling out to manage various "events" like reaching a barrier?  So basically:

```
func handleBitFieldRun() {
   var accumulator = {}
   var curAccessUnit = {}
   for field in run {
     if field.offset % byteSize {
       curAccessUnit.add(field)
     } else {
       if !curAccessUnit.empty() {
         accumulator.add(curAccessUnit)
         curAccessUnit = {}
       }
       if field.isZeroWidth {
         accumulator.barrier()
       } else {
         curAccessUnit.add(field)
       }
     }
   }
   accumulator.barrier()
}

func accumulator::barrier() {
  // emit the current access unit if we have one, then reset 
}

func accumulator::add(newAccessUnit) {
  // either merge the new access unit into the current one or
  // emit the current one and start building from the new one
}
```

Your merge algorithm is still online, right?  That is, you can think of it as always considering whether to merge the current access unit with the next, as opposed to deciding to merge the 2nd and 3rd and then later deciding whether to merge that with the 1st?

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