Apply reviews

Author: Andres-Salamanca

clang/include/clang/CIR/Dialect/IR/CIRAttrs.td

@@ -460,8 +460,33 @@ def CIR_VisibilityAttr : CIR_EnumAttr<CIR_VisibilityKind, "visibility"> {
 def BitfieldInfoAttr : CIR_Attr<"BitfieldInfo", "bitfield_info"> {
   let summary = "Represents a bit field info";
   let description = [{
-    Holds the next information about bitfields: name, storage type, a bitfield
-    size and position in the storage, if the bitfield is signed or not.
+    Holds the following information about bitfields: name, storage type, size
+    and position in the storage, and signedness.
+    Example:
+      Given the following struct with bitfields:
+        ```c++
+        typedef struct {
+          int a : 4;
+          int b : 27;
+          int c : 17;
+          int d : 2;
+          int e : 15;
+        } S;
+        ```
+
+      The CIR representation of the struct `S` might look like:
+      ```mlir
+        !rec_S = !cir.record<struct "S" packed padded {!u64i, !u16i,
+                                                       !cir.array<!u8i x 2>}>
+      ```
+      And the bitfield info attribute for member `a` would be:
+      ```mlir
+        #bfi_a = #cir.bitfield_info<name = "a", storage_type = !u64i,
+                                    size = 4, offset = 0, is_signed = true>
+      ```
+
+      This metadata describes that field `a` is stored in a 64-bit integer,
+      is 4 bits wide, starts at offset 0, and is signed.
   }];
   let parameters = (ins "mlir::StringAttr":$name,
                         "mlir::Type":$storageType,

clang/lib/CIR/CodeGen/CIRGenBuilder.h

@@ -398,9 +398,8 @@ class CIRGenBuilderTy : public cir::CIRBaseBuilderTy {
                                 mlir::Value addr, mlir::Type storageType,
                                 const CIRGenBitFieldInfo &info,
                                 bool isLvalueVolatile, bool useVolatile) {
-    unsigned int offset = useVolatile ? info.volatileOffset : info.offset;
     return create<cir::GetBitfieldOp>(loc, resultType, addr, storageType,
-                                      info.name, info.size, offset,
+                                      info.name, info.size, info.offset,
                                       info.isSigned, isLvalueVolatile);
   }
 };

clang/lib/CIR/CodeGen/CIRGenExpr.cpp

@@ -360,6 +360,7 @@ LValue CIRGenFunction::emitLValueForBitField(LValue base,
       cgm.getTypes().getCIRGenRecordLayout(field->getParent());
   const CIRGenBitFieldInfo &info = layout.getBitFieldInfo(field);
   assert(!cir::MissingFeatures::armComputeVolatileBitfields());
+  assert(!cir::MissingFeatures::preservedAccessIndexRegion());
   unsigned idx = layout.getCIRFieldNo(field);
 
   Address addr = getAddrOfBitFieldStorage(base, field, info.storageType, idx);

clang/test/CIR/CodeGen/bitfields.c

@@ -13,6 +13,7 @@ typedef struct {
 } A;
 
 // CIR-DAG:  !rec_A = !cir.record<struct "A" packed padded {!s8i, !s8i, !s8i, !u16i, !cir.array<!u8i x 3>}>
+// CIR-DAG:  #bfi_more_bits = #cir.bitfield_info<name = "more_bits", storageType = !u16i, size = 4, offset = 3, isSigned = false>
 // LLVM-DAG: %struct.A = type <{ i8, i8, i8, i16, [3 x i8] }>
 // OGCG-DAG: %struct.A = type <{ i8, i8, i8, i16, [3 x i8] }>
 
@@ -105,3 +106,31 @@ int load_field(S* s) {
 // OGCG:  [[TMP3:%.*]] = shl i64 [[TMP2]], 15
 // OGCG:  [[TMP4:%.*]] = ashr i64 [[TMP3]], 47
 // OGCG:  [[TMP5:%.*]] = trunc i64 [[TMP4]] to i32
+
+unsigned int load_field_unsigned(A* s) {
+  return s->more_bits;
+}
+
+//CIR: cir.func dso_local @load_field_unsigned
+//CIR:   [[TMP0:%.*]] = cir.alloca !cir.ptr<!rec_A>, !cir.ptr<!cir.ptr<!rec_A>>, ["s", init] {alignment = 8 : i64}
+//CIR:   [[TMP1:%.*]] = cir.load align(8) [[TMP0]] : !cir.ptr<!cir.ptr<!rec_A>>, !cir.ptr<!rec_A>
+//CIR:   [[TMP2:%.*]] = cir.get_member [[TMP1]][3] {name = "more_bits"} : !cir.ptr<!rec_A> -> !cir.ptr<!u16i>
+//CIR:   [[TMP3:%.*]] = cir.get_bitfield(#bfi_more_bits, [[TMP2]] : !cir.ptr<!u16i>) -> !u32i
+
+//LLVM: define dso_local i32 @load_field_unsigned
+//LLVM:   [[TMP0:%.*]] = alloca ptr, i64 1, align 8
+//LLVM:   [[TMP1:%.*]] = load ptr, ptr [[TMP0]], align 8
+//LLVM:   [[TMP2:%.*]] = getelementptr %struct.A, ptr [[TMP1]], i32 0, i32 3
+//LLVM:   [[TMP3:%.*]] = load i16, ptr [[TMP2]], align 2
+//LLVM:   [[TMP4:%.*]] = lshr i16 [[TMP3]], 3
+//LLVM:   [[TMP5:%.*]] = and i16 [[TMP4]], 15
+//LLVM:   [[TMP6:%.*]] = zext i16 [[TMP5]] to i32
+
+//OGCG: define dso_local i32 @load_field_unsigned
+//OGCG:   [[TMP0:%.*]] = alloca ptr, align 8
+//OGCG:   [[TMP1:%.*]] = load ptr, ptr [[TMP0]], align 8
+//OGCG:   [[TMP2:%.*]] = getelementptr inbounds nuw %struct.A, ptr [[TMP1]], i32 0, i32 3
+//OGCG:   [[TMP3:%.*]] = load i16, ptr [[TMP2]], align 1
+//OGCG:   [[TMP4:%.*]] = lshr i16 [[TMP3]], 3
+//OGCG:   [[TMP5:%.*]] = and i16 [[TMP4]], 15
+//OGCG:   [[TMP6:%.*]] = zext i16 [[TMP5]] to i32