[CIR] Upstream get_bitfield operation to load bit-field members from structs #145971

Andres-Salamanca · 2025-06-26T21:01:18Z

This PR adds support for loading bit-field members from structs using the get_bitfield operation.
It enables retrieving the address of the bitfield-packed member but does not yet support volatile bitfields this will be addressed in a future PR.

llvmbot · 2025-06-26T21:01:52Z

@llvm/pr-subscribers-clangir

@llvm/pr-subscribers-clang

Author: None (Andres-Salamanca)

Changes

This PR adds support for retrieving a bitfield member. It obtains the member in which the bitfield is packed but does not emit the load operation, as the address will be computed later using the get_bitfield operation.

Full diff: https://github.com/llvm/llvm-project/pull/145971.diff

5 Files Affected:

(modified) clang/include/clang/CIR/MissingFeatures.h (+1)
(modified) clang/lib/CIR/CodeGen/CIRGenExpr.cpp (+43-4)
(modified) clang/lib/CIR/CodeGen/CIRGenFunction.h (+4)
(modified) clang/lib/CIR/CodeGen/CIRGenValue.h (+19)
(modified) clang/test/CIR/CodeGen/bitfields.c (+8)

diff --git a/clang/include/clang/CIR/MissingFeatures.h b/clang/include/clang/CIR/MissingFeatures.h
index 9e8944d1114b8..e136d73daac89 100644
--- a/clang/include/clang/CIR/MissingFeatures.h
+++ b/clang/include/clang/CIR/MissingFeatures.h
@@ -200,6 +200,7 @@ struct MissingFeatures {
   static bool fastMathFlags() { return false; }
   static bool fpConstraints() { return false; }
   static bool generateDebugInfo() { return false; }
+  static bool getBitfieldOp() { return false; }
   static bool hip() { return false; }
   static bool implicitConstructorArgs() { return false; }
   static bool incrementProfileCounter() { return false; }
diff --git a/clang/lib/CIR/CodeGen/CIRGenExpr.cpp b/clang/lib/CIR/CodeGen/CIRGenExpr.cpp
index 5c6604d784156..a3d24e92c701a 100644
--- a/clang/lib/CIR/CodeGen/CIRGenExpr.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenExpr.cpp
@@ -326,13 +326,47 @@ mlir::Value CIRGenFunction::emitStoreThroughBitfieldLValue(RValue src,
   return {};
 }
 
+Address CIRGenFunction::getAddrOfBitFieldStorage(LValue base,
+                                                 const FieldDecl *field,
+                                                 mlir::Type fieldType,
+                                                 unsigned index) {
+  if (index == 0)
+    return base.getAddress();
+  mlir::Location loc = getLoc(field->getLocation());
+  cir::PointerType fieldPtr = cir::PointerType::get(fieldType);
+  cir::GetMemberOp sea = getBuilder().createGetMember(
+      loc, fieldPtr, base.getPointer(), field->getName(), index);
+  return Address(sea, CharUnits::One());
+}
+
+LValue CIRGenFunction::emitLValueForBitField(LValue base,
+                                             const FieldDecl *field) {
+  LValueBaseInfo baseInfo = base.getBaseInfo();
+  const CIRGenRecordLayout &layout =
+      cgm.getTypes().getCIRGenRecordLayout(field->getParent());
+  const CIRGenBitFieldInfo &info = layout.getBitFieldInfo(field);
+  assert(!cir::MissingFeatures::armComputeVolatileBitfields());
+  unsigned idx = layout.getCIRFieldNo(field);
+
+  Address addr = getAddrOfBitFieldStorage(base, field, info.storageType, idx);
+
+  mlir::Location loc = getLoc(field->getLocation());
+  if (addr.getElementType() != info.storageType)
+    addr = builder.createElementBitCast(loc, addr, info.storageType);
+
+  QualType fieldType =
+      field->getType().withCVRQualifiers(base.getVRQualifiers());
+  // TODO(cir): Support TBAA for bit fields.
+  assert(!cir::MissingFeatures::opTBAA());
+  LValueBaseInfo fieldBaseInfo(baseInfo.getAlignmentSource());
+  return LValue::makeBitfield(addr, info, fieldType, fieldBaseInfo);
+}
+
 LValue CIRGenFunction::emitLValueForField(LValue base, const FieldDecl *field) {
   LValueBaseInfo baseInfo = base.getBaseInfo();
 
-  if (field->isBitField()) {
-    cgm.errorNYI(field->getSourceRange(), "emitLValueForField: bitfield");
-    return LValue();
-  }
+  if (field->isBitField())
+    return emitLValueForBitField(base, field);
 
   QualType fieldType = field->getType();
   const RecordDecl *rec = field->getParent();
@@ -460,6 +494,11 @@ RValue CIRGenFunction::emitLoadOfLValue(LValue lv, SourceLocation loc) {
   assert(!lv.getType()->isFunctionType());
   assert(!(lv.getType()->isConstantMatrixType()) && "not implemented");
 
+  if (lv.isBitField()) {
+    assert(!cir::MissingFeatures::getBitfieldOp());
+    return RValue::getIgnored();
+  }
+
   if (lv.isSimple())
     return RValue::get(emitLoadOfScalar(lv, loc));
 
diff --git a/clang/lib/CIR/CodeGen/CIRGenFunction.h b/clang/lib/CIR/CodeGen/CIRGenFunction.h
index 2e54243f18cff..5139bc8249b3d 100644
--- a/clang/lib/CIR/CodeGen/CIRGenFunction.h
+++ b/clang/lib/CIR/CodeGen/CIRGenFunction.h
@@ -550,6 +550,9 @@ class CIRGenFunction : public CIRGenTypeCache {
     return it->second;
   }
 
+  Address getAddrOfBitFieldStorage(LValue base, const clang::FieldDecl *field,
+                                   mlir::Type fieldType, unsigned index);
+
   /// Load the value for 'this'. This function is only valid while generating
   /// code for an C++ member function.
   /// FIXME(cir): this should return a mlir::Value!
@@ -964,6 +967,7 @@ class CIRGenFunction : public CIRGenTypeCache {
   /// of the expression.
   /// FIXME: document this function better.
   LValue emitLValue(const clang::Expr *e);
+  LValue emitLValueForBitField(LValue base, const FieldDecl *field);
   LValue emitLValueForField(LValue base, const clang::FieldDecl *field);
 
   /// Like emitLValueForField, excpet that if the Field is a reference, this
diff --git a/clang/lib/CIR/CodeGen/CIRGenValue.h b/clang/lib/CIR/CodeGen/CIRGenValue.h
index a5a457ddafa9c..2d6bdb921c9fa 100644
--- a/clang/lib/CIR/CodeGen/CIRGenValue.h
+++ b/clang/lib/CIR/CodeGen/CIRGenValue.h
@@ -19,6 +19,7 @@
 #include "clang/AST/CharUnits.h"
 #include "clang/AST/Type.h"
 
+#include "CIRGenRecordLayout.h"
 #include "mlir/IR/Value.h"
 
 #include "clang/CIR/MissingFeatures.h"
@@ -162,6 +163,7 @@ class LValue {
   mlir::Value vectorIdx; // Index for vector subscript
   mlir::Type elementType;
   LValueBaseInfo baseInfo;
+  const CIRGenBitFieldInfo *bitFieldInfo{nullptr};
 
   void initialize(clang::QualType type, clang::Qualifiers quals,
                   clang::CharUnits alignment, LValueBaseInfo baseInfo) {
@@ -245,6 +247,23 @@ class LValue {
     r.initialize(t, t.getQualifiers(), vecAddress.getAlignment(), baseInfo);
     return r;
   }
+
+  /// Create a new object to represent a bit-field access.
+  ///
+  /// \param Addr - The base address of the bit-field sequence this
+  /// bit-field refers to.
+  /// \param Info - The information describing how to perform the bit-field
+  /// access.
+  static LValue makeBitfield(Address addr, const CIRGenBitFieldInfo &info,
+                             clang::QualType type, LValueBaseInfo baseInfo) {
+    LValue r;
+    r.lvType = BitField;
+    r.v = addr.getPointer();
+    r.elementType = addr.getElementType();
+    r.bitFieldInfo = &info;
+    r.initialize(type, type.getQualifiers(), addr.getAlignment(), baseInfo);
+    return r;
+  }
 };
 
 /// An aggregate value slot.
diff --git a/clang/test/CIR/CodeGen/bitfields.c b/clang/test/CIR/CodeGen/bitfields.c
index ff5c6bc1787b4..4cc1fc69ecc5a 100644
--- a/clang/test/CIR/CodeGen/bitfields.c
+++ b/clang/test/CIR/CodeGen/bitfields.c
@@ -76,3 +76,11 @@ void def() {
   T t;
   U u;
 }
+
+// CIR: cir.func {{.*@load_field}}
+// CIR:   [[TMP0:%.*]] = cir.alloca !cir.ptr<!rec_S>, !cir.ptr<!cir.ptr<!rec_S>>, ["s", init]
+// CIR:   [[TMP1:%.*]] = cir.load{{.*}} [[TMP0]] : !cir.ptr<!cir.ptr<!rec_S>>, !cir.ptr<!rec_S>
+// CIR:   [[TMP2:%.*]] = cir.get_member %2[1] {name = "e"} : !cir.ptr<!rec_S> -> !cir.ptr<!u16i>
+int load_field(S* s) {
+  return s->e;
+}

erichkeane · 2025-06-26T21:23:27Z

clang/lib/CIR/CodeGen/CIRGenExpr.cpp

+                                                 mlir::Type fieldType,
+                                                 unsigned index) {
+  if (index == 0)
+    return base.getAddress();


I wonder if even in this case we should be doing the getMemberOp? Its a no-op but is clear in IR what is going on.

Yes, in upstream we actually convert that into a bitcast, as shown here: https://godbolt.org/z/1aq6Mdajo. However, we can't do that right now because we don't have getBitField implemented yet.

I think what Erich is saying is that we should be doing cir.get_member followed by cir.get_bitfield in this case. So this function would just return the get_member op, as it does below for non-zero indexes.

Note that for non-bitfields, we do access the zero member this way.

https://godbolt.org/z/xc3aWvx1b

Yep, thats exactly what I'm saying, thanks! This is another case of 'premature optimization' in the FE due to lack of LLVM expressiveness. CIR can be better, and this is a case where it will likely make our pattern matching easier, particularly once anything that is a getBitField. But at least for now, skip the index ==0 check if we can.

I made the change. In the tests, I used s->c, which is packed inside the first struct member.

andykaylor

I appreciate the small change, but in this case I think it would be better to introduce get_bitfield together with this code, because the expected behavior without it is unclear and difficult to evaluate.

andykaylor · 2025-06-26T22:25:03Z

clang/lib/CIR/CodeGen/CIRGenExpr.cpp

+                                                 mlir::Type fieldType,
+                                                 unsigned index) {
+  if (index == 0)
+    return base.getAddress();


I think what Erich is saying is that we should be doing cir.get_member followed by cir.get_bitfield in this case. So this function would just return the get_member op, as it does below for non-zero indexes.

Note that for non-bitfields, we do access the zero member this way.

https://godbolt.org/z/xc3aWvx1b

Andres-Salamanca · 2025-06-26T22:51:59Z

I initially split this out because I also need to introduce a BitfieldInfoAttr, and combining everything in one patch would make it quite long. That said, I was already working on this in another branch, so I’ll go ahead and bring that code over and include get_bitfield here as well.

…structs

andykaylor · 2025-06-27T23:01:20Z

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

+    Holds the next information about bitfields: name, storage type, a bitfield
+    size and position in the storage, if the bitfield is signed or not.


Suggested change

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.

andykaylor · 2025-06-27T23:02:38Z

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

+  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.
+  }];


Can you add an example?

andykaylor · 2025-06-27T23:22:31Z

clang/lib/CIR/CodeGen/CIRGenBuilder.h

+                                mlir::Value addr, mlir::Type storageType,
+                                const CIRGenBitFieldInfo &info,
+                                bool isLvalueVolatile, bool useVolatile) {
+    unsigned int offset = useVolatile ? info.volatileOffset : info.offset;


Classic codegen seems to also use useVolatile to select the info.volatileSize. I suspect we'll need to do that too, but more generally I think we should delay introducing the volatile fields in the info until we have support for the target that uses them.

Yes, I agree that’s why I was always calling this function with useVolatile = false. I'm going to remove this for now and always use info.offset

andykaylor · 2025-06-27T23:34:49Z

clang/lib/CIR/CodeGen/CIRGenExpr.cpp

+  const CIRGenBitFieldInfo &info = layout.getBitFieldInfo(field);
+  assert(!cir::MissingFeatures::armComputeVolatileBitfields());
+  unsigned idx = layout.getCIRFieldNo(field);
+


Can you add a missing feature marker here for preservedAccessIndexRegion? It's not in the incubator code, but classic codegen has something here that we're missing.

andykaylor · 2025-06-27T23:40:30Z

clang/lib/CIR/Dialect/IR/CIRDialect.cpp

@@ -65,6 +65,10 @@ struct CIROpAsmDialectInterface : public OpAsmDialectInterface {
      os << (boolAttr.getValue() ? "true" : "false");
      return AliasResult::FinalAlias;
    }
+    if (auto bitfield = mlir::dyn_cast<cir::BitfieldInfoAttr>(attr)) {


Can you add an IR test to make sure this gets parsed correctly?

I was trying to add an IR test for, but I ran into the following parse error when trying to define a struct:

erro.cir:11:21: error: complete records are not yet supported !rec_D = !cir.record<struct "D" {!u16i, !s32i}> ^

llvm-project/clang/lib/CIR/Dialect/IR/CIRTypes.cpp

Line 162 in aa8afad

} else if (!incomplete) { // complete

Without the struct definition, I can't meaningfully call get_member or get_bitfield, and the resulting module ends up being empty. That makes it impossible to use CHECK directives to verify anything.

Should I consider it sufficient to check that the input runs without errors for now?

andykaylor · 2025-06-27T23:42:44Z

clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp

+  mlir::MLIRContext *context = storageType.getContext();
+  unsigned storageSize = 0;
+
+  if (auto arTy = mlir::dyn_cast<cir::ArrayType>(storageType))


Can this still happen? I thought after your recent change to align with classic codegen we wouldn't generate arrays anymore.

Yes, we can still produce arrays when clipping. Here's an example: https://godbolt.org/z/sGae8vcze. The CodeGen explanation for this case is here:
https://github.com/llvm/llvm-project/blob/fb24b4d46a0a8278031f42c1cba6c030eb6c6010/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp#L44C1-L49C27

Also, clipping can occur when the target does not support unaligned bitfield access, when !astContext.getTargetInfo().hasCheapUnalignedBitFieldAccess() is true. However, for our case, that's currently NYI.

andykaylor · 2025-06-27T23:48:19Z

clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp

+    llvm_unreachable(
+        "Either ArrayType or IntType expected for bitfields storage");
+
+  mlir::IntegerType intType = mlir::IntegerType::get(context, storageSize);


I don't like having intType here right after intTy above, but if we don't need to handle arrays that should go away.

andykaylor · 2025-06-27T23:55:49Z

clang/test/CIR/CodeGen/bitfields.c

@@ -76,3 +77,31 @@ void def() {
  T t;
  U u;
 }
+
+int load_field(S* s) {
+  return s->c;


Can you add a test case with an unsigned field?

Get Lvalue for bit-field

d2e19a9

Andres-Salamanca requested review from xlauko, erichkeane and andykaylor June 26, 2025 21:01

Andres-Salamanca requested review from lanza and bcardosolopes as code owners June 26, 2025 21:01

llvmbot added clang Clang issues not falling into any other category ClangIR Anything related to the ClangIR project labels Jun 26, 2025

Andres-Salamanca changed the title ~~[CIR] Get Lvalue for bit-field~~ [CIR] Add support for retrieving bitfield member Jun 26, 2025

erichkeane reviewed Jun 26, 2025

View reviewed changes

andykaylor requested changes Jun 26, 2025

View reviewed changes

[CIR] Upstream get_bitfield operation to load bit-field members from …

a94f579

…structs

Andres-Salamanca changed the title ~~[CIR] Add support for retrieving bitfield member~~ [CIR] Upstream get_bitfield operation to load bit-field members from structs Jun 27, 2025

Andres-Salamanca added 2 commits June 27, 2025 16:22

Remove auto

1f3479e

Remove getBitfieldOp missing feature

24d6703

Andres-Salamanca requested review from erichkeane and andykaylor June 27, 2025 21:28

andykaylor reviewed Jun 27, 2025

View reviewed changes

Apply reviews

fccad21

Andres-Salamanca requested a review from andykaylor June 28, 2025 23:07

		Holds the next information about bitfields: name, storage type, a bitfield
		size and position in the storage, if the bitfield is signed or not.

[CIR] Upstream get_bitfield operation to load bit-field members from structs #145971

Are you sure you want to change the base?

[CIR] Upstream get_bitfield operation to load bit-field members from structs #145971

Uh oh!

Conversation

Andres-Salamanca commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andykaylor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Andres-Salamanca commented Jun 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Andres-Salamanca commented Jun 26, 2025 •

edited

Loading

llvmbot commented Jun 26, 2025 •

edited

Loading