Applied code review changes

Author: Andres-Salamanca

clang/include/clang/CIR/MissingFeatures.h

@@ -219,6 +219,7 @@ struct MissingFeatures {
   static bool peepholeProtection() { return false; }
   static bool instrumentation() { return false; }
   static bool cleanupAfterErrorDiags() { return false; }
+  static bool nonFineGrainedBitfields() { return false; }
 
   // Missing types
   static bool dataMemberType() { return false; }

clang/lib/CIR/CodeGen/CIRGenRecordLayout.h

@@ -41,8 +41,9 @@ namespace clang::CIRGen {
 ///     unsigned still_more_bits : 7;
 ///   };
 ///
-/// This will end up as the following cir.record. The first array is the
-/// bitfield, and the second is the padding out to a 4-byte alignment.
+/// This will end up as the following cir.record. The bitfield members are
+/// represented by two !u8i values, and the array provides padding to align the
+/// struct to a 4-byte alignment.
 ///
 ///   !rec_S = !cir.record<struct "S" padded {!s8i, !s8i, !s8i, !u8i, !u8i,
 ///   !cir.array<!u8i x 3>}>
@@ -51,16 +52,16 @@ namespace clang::CIRGen {
 /// essentially like this:
 ///
 ///   #bfi_more_bits = #cir.bitfield_info<name = "more_bits", storage_type =
-///   !u16i, size = 4, offset = 3, is_signed = false>
+///   !u8i, size = 4, offset = 3, is_signed = false>
 ///
 ///   cir.func @store_field() {
 ///     %0 = cir.alloca !rec_S, !cir.ptr<!rec_S>, ["s"] {alignment = 4 : i64}
 ///     %1 = cir.const #cir.int<2> : !s32i
 ///     %2 = cir.cast(integral, %1 : !s32i), !u32i
-///     %3 = cir.get_member %0[4] {name = "more_bits"} : !cir.ptr<!rec_S> ->
-///     !cir.ptr<!u16i>
+///     %3 = cir.get_member %0[3] {name = "more_bits"} : !cir.ptr<!rec_S> ->
+///     !cir.ptr<!u8i>
 ///     %4 = cir.set_bitfield(#bfi_more_bits, %3 :
-///     !cir.ptr<!u16i>, %2 : !u32i) -> !u32i
+///     !cir.ptr<!u8i>, %2 : !u32i) -> !u32i
 ///     cir.return
 ///   }
 ///
@@ -110,7 +111,7 @@ struct CIRGenBitFieldInfo {
         storageSize(storageSize), storageOffset(storageOffset) {}
 
   void print(llvm::raw_ostream &os) const;
-  void dump() const;
+  LLVM_DUMP_METHOD void dump() const;
 };
 
 /// This class handles record and union layout info while lowering AST types

clang/lib/CIR/CodeGen/CIRGenRecordLayoutBuilder.cpp

@@ -223,9 +223,9 @@ void CIRRecordLowering::setBitFieldInfo(const FieldDecl *fd,
 
   if (info.size > info.storageSize)
     info.size = info.storageSize;
-  // Reverse the bit offsets for big endian machines. Because we represent
-  // a bitfield as a single large integer load, we can imagine the bits
-  // counting from the most-significant-bit instead the
+  // Reverse the bit offsets for big endian machines. Since bitfields are laid
+  // out as packed bits within an integer-sized unit, we can imagine the bits
+  // counting from the most-significant-bit instead of the
   // least-significant-bit.
   assert(!cir::MissingFeatures::isBigEndian());
 
@@ -292,35 +292,25 @@ void CIRRecordLowering::fillOutputFields() {
 
 void CIRRecordLowering::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
+  // '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
+  // '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.
+  // contiguous. 'startBitOffset' is offset of the beginning of the run.
   uint64_t startBitOffset, tail = 0;
   assert(!cir::MissingFeatures::isDiscreteBitFieldABI());
 
-  // Check if OffsetInRecord (the size in bits of the current run) is better
+  // Check if 'offsetInRecord' (the size in bits of the current run) is better
   // as a single field run. When OffsetInRecord has legal integer width, and
   // its bitfield offset is naturally aligned, it is better to make the
   // bitfield a separate storage component so as it can be accessed directly
   // with lower cost.
-  auto isBetterAsSingleFieldRun = [&](uint64_t offsetInRecord,
-                                      uint64_t startBitOffset,
-                                      uint64_t nextTail = 0) {
-    if (!cirGenTypes.getCGModule().getCodeGenOpts().FineGrainedBitfieldAccesses)
-      return false;
-    cirGenTypes.getCGModule().errorNYI(field->getSourceRange(),
-                                       "NYI FineGrainedBitfield");
-    return true;
-  };
+  assert(!cir::MissingFeatures::nonFineGrainedBitfields());
 
-  // The start field is better as a single field run.
-  bool startFieldAsSingleRun = false;
   for (;;) {
     // Check to see if we need to start a new run.
     if (run == fieldEnd) {
@@ -332,27 +322,34 @@ void CIRRecordLowering::accumulateBitFields(
         run = field;
         startBitOffset = getFieldBitOffset(*field);
         tail = startBitOffset + field->getBitWidthValue();
-        startFieldAsSingleRun =
-            isBetterAsSingleFieldRun(tail - startBitOffset, startBitOffset);
+        assert(!cir::MissingFeatures::nonFineGrainedBitfields());
       }
       ++field;
       continue;
     }
 
-    // If the start field of a new run is better as a single run, or if current
-    // field (or consecutive fields) is better as a single run, or if current
-    // field has zero width bitfield and either UseZeroLengthBitfieldAlignment
-    // or UseBitFieldTypeAlignment is set to true, or if the offset of current
-    // field is inconsistent with the offset of previous field plus its offset,
-    // skip the block below and go ahead to emit the storage. Otherwise, try to
-    // add bitfields to the run.
+    // Decide whether to continue extending the current bitfield run.
+    //
+    // Skip the block below and go directly to emitting storage if any of the
+    // following is true:
+    // - 1. The first field in the run is better treated as its own run.
+    // - 2. We have reached the end of the fields.
+    // - 3. The current field (or set of fields) is better as its own run.
+    // - 4. The current field is a zero-width bitfield or:
+    //     - Zero-length bitfield alignment is enabled, and
+    //     - Bitfield type alignment is enabled.
+    // - 5. The current field's offset doesn't match the expected tail (i.e.,
+    // layout isn't contiguous).
+    //
+    // If none of the above conditions are met, add the current field to the
+    // current run.
     uint64_t nextTail = tail;
     if (field != fieldEnd)
       nextTail += field->getBitWidthValue();
 
-    if (!startFieldAsSingleRun && field != fieldEnd &&
-        !isBetterAsSingleFieldRun(tail - startBitOffset, startBitOffset,
-                                  nextTail) &&
+    // TODO: add condition 1 and 3
+    assert(!cir::MissingFeatures::nonFineGrainedBitfields());
+    if (field != fieldEnd &&
         (!field->isZeroLengthBitField() ||
          (!astContext.getTargetInfo().useZeroLengthBitfieldAlignment() &&
           !astContext.getTargetInfo().useBitFieldTypeAlignment())) &&
@@ -373,7 +370,6 @@ void CIRRecordLowering::accumulateBitFields(
       members.push_back(MemberInfo(bitsToCharUnits(startBitOffset),
                                    MemberInfo::InfoKind::Field, nullptr, *run));
     run = fieldEnd;
-    startFieldAsSingleRun = false;
   }
 }
 

clang/test/CIR/CodeGen/bitfields.cpp

@@ -12,12 +12,20 @@ struct A {
   unsigned still_more_bits : 7;
 };
 
+// CIR-DAG:  !rec_A = !cir.record<struct "A" padded {!s8i, !s8i, !s8i, !u8i, !u8i, !cir.array<!u8i x 3>}>
+// LLVM-DAG: %struct.A = type { i8, i8, i8, i8, i8, [3 x i8] }
+// OGCG-DAG: %struct.A = type <{ i8, i8, i8, i16, [3 x i8] }>
+
 typedef struct {
   int a : 4;
   int b : 5;
   int c;
 } D;
 
+// CIR-DAG:  !rec_D = !cir.record<struct "D" {!u16i, !s32i}>
+// LLVM-DAG: %struct.D = type { i16, i32 }
+// OGCG-DAG: %struct.D = type { i16, i32 }
+
 typedef struct {
   int a : 4;
   int b : 27;
@@ -27,11 +35,19 @@ typedef struct {
   unsigned f; // type other than int above, not a bitfield
 } S;
 
+// CIR-DAG:  !rec_S = !cir.record<struct "S" {!u32i, !cir.array<!u8i x 3>, !u16i, !u32i}>
+// LLVM-DAG: %struct.S = type { i32, [3 x i8], i16, i32 }
+// OGCG-DAG: %struct.S = type { i64, i16, i32 }
+
 typedef struct {
   int a : 3;  // one bitfield with size < 8
   unsigned b;
 } T;
 
+// CIR-DAG:  !rec_T = !cir.record<struct "T" {!u8i, !u32i}>
+// LLVM-DAG: %struct.T = type { i8, i32 }
+// OGCG-DAG: %struct.T = type { i8, i32 }
+
 typedef struct {
     char a;
     char b;
@@ -51,24 +67,9 @@ typedef struct {
                // because (tail - startOffset) is 65 after 'l' field
 } U;
 
-// CIR-DAG:  !rec_D = !cir.record<struct "D" {!u16i, !s32i}>
-// CIR-DAG:  !rec_T = !cir.record<struct "T" {!u8i, !u32i}>
 // CIR-DAG:  !rec_U = !cir.record<struct "U" {!s8i, !s8i, !s8i, !cir.array<!u8i x 9>}>
-// CIR-DAG:  !rec_A = !cir.record<struct "A" padded {!s8i, !s8i, !s8i, !u8i, !u8i, !cir.array<!u8i x 3>}>
-// CIR-DAG:  !rec_S = !cir.record<struct "S" {!u32i, !cir.array<!u8i x 3>, !u16i, !u32i}>
-
-
-// LLVM-DAG: %struct.D = type { i16, i32 }
 // LLVM-DAG: %struct.U = type { i8, i8, i8, [9 x i8] }
-// LLVM-DAG: %struct.A = type { i8, i8, i8, i8, i8, [3 x i8] }
-// LLVM-DAG: %struct.S = type { i32, [3 x i8], i16, i32 }
-// LLVM-DAG: %struct.T = type { i8, i32 }
-
-// OGCG-DAG: %struct.D = type { i16, i32 }
 // OGCG-DAG: %struct.U = type <{ i8, i8, i8, i8, i64 }>
-// OGCG-DAG: %struct.A = type <{ i8, i8, i8, i16, [3 x i8] }>
-// OGCG-DAG: %struct.S = type { i64, i16, i32 }
-// OGCG-DAG: %struct.T = type { i8, i32 }
 
 void def() {
   A a;