Skip to content

[CIR] Upstream new SetBitfieldOp for handling C and C++ struct bitfields #147609

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 84 additions & 0 deletions clang/include/clang/CIR/Dialect/IR/CIROps.td
Original file line number Diff line number Diff line change
Expand Up @@ -1669,6 +1669,90 @@ def GetGlobalOp : CIR_Op<"get_global",
}];
}

//===----------------------------------------------------------------------===//
// SetBitfieldOp
//===----------------------------------------------------------------------===//

def SetBitfieldOp : CIR_Op<"set_bitfield"> {
let summary = "Set the value of a bitfield member";
let description = [{
The `cir.set_bitfield` operation provides a store-like access to
a bit field of a record.

It expects an address of a storage where to store, a type of the storage,
a value being stored, a name of a bit field, a pointer to the storage in the
base record, a size of the storage, a size the bit field, an offset
of the bit field and a sign. Returns a value being stored.

A unit attribute `volatile` can be used to indicate a volatile load of the
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have any tests below for a 'volatile' load'? Also the store should be volatile too, right? Should we mention that more elequently?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps adding an example would also be helpful

bitfield.

Example.
Suppose we have a struct with multiple bitfields stored in
different storages. The `cir.set_bitfield` operation sets the value
of the bitfield.
```C++
typedef struct {
int a : 4;
int b : 27;
int c : 17;
int d : 2;
int e : 15;
} S;

void store_bitfield(S& s) {
s.e = 3;
}
```

```mlir
// 'e' is in the storage with the index 1
!record_type = !cir.record<struct "S" packed padded {!u64i, !u16i,
!cir.array<!u8i x 2>} #cir.record.decl.ast>
#bfi_e = #cir.bitfield_info<name = "e", storage_type = !u16i, size = 15,
offset = 0, is_signed = true>

%1 = cir.const #cir.int<3> : !s32i
%2 = cir.load %0 : !cir.ptr<!cir.ptr<!record_type>>, !cir.ptr<!record_type>
%3 = cir.get_member %2[1] {name = "e"} : !cir.ptr<!record_type>
-> !cir.ptr<!u16i>
%4 = cir.set_bitfield(#bfi_e, %3 : !cir.ptr<!u16i>, %1 : !s32i) -> !s32i
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should introduce this one without the parens for args? See https://llvm.github.io/clangir/Dialect/cir-style-guide.html

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be written like this, with a comma after the attribute?

cir.set_bitfield #bfi_e, %3, %1 : (!cir.ptr<!u16i>, !s32i) -> !s32i

Or without the comma?

cir.set_bitfield #bfi_e %3, %1 : (!cir.ptr<!u16i>, !s32i) -> !s32i

And should we also update cir.get_bitfield to follow the same format for consistency?

```
}];

let arguments = (ins
Arg<CIR_PointerType, "the address to store the value", [MemWrite]>:$addr,
CIR_AnyType:$src,
BitfieldInfoAttr:$bitfield_info,
UnitAttr:$is_volatile
);

let results = (outs CIR_IntType:$result);

let assemblyFormat = [{ `(`$bitfield_info`,` $addr`:`qualified(type($addr))`,`
$src`:`type($src) `)` attr-dict `->` type($result) }];

let builders = [
OpBuilder<(ins "mlir::Type":$type,
"mlir::Value":$addr,
"mlir::Type":$storage_type,
"mlir::Value":$src,
"llvm::StringRef":$name,
"unsigned":$size,
"unsigned":$offset,
"bool":$is_signed,
"bool":$is_volatile
),
[{
BitfieldInfoAttr info =
BitfieldInfoAttr::get($_builder.getContext(),
name, storage_type,
size, offset, is_signed);
build($_builder, $_state, type, addr, src, info, is_volatile);
}]>
];
}

//===----------------------------------------------------------------------===//
// GetBitfieldOp
//===----------------------------------------------------------------------===//
Expand Down
9 changes: 9 additions & 0 deletions clang/lib/CIR/CodeGen/CIRGenBuilder.h
Original file line number Diff line number Diff line change
Expand Up @@ -394,6 +394,15 @@ class CIRGenBuilderTy : public cir::CIRBaseBuilderTy {
return createGlobal(module, loc, uniqueName, type, linkage);
}

mlir::Value createSetBitfield(mlir::Location loc, mlir::Type resultType,
mlir::Value dstAddr, mlir::Type storageType,
mlir::Value src, const CIRGenBitFieldInfo &info,
bool isLvalueVolatile, bool useVolatile) {
return create<cir::SetBitfieldOp>(loc, resultType, dstAddr, storageType,
src, info.name, info.size, info.offset,
info.isSigned, isLvalueVolatile);
}

mlir::Value createGetBitfield(mlir::Location loc, mlir::Type resultType,
mlir::Value addr, mlir::Type storageType,
const CIRGenBitFieldInfo &info,
Expand Down
27 changes: 21 additions & 6 deletions clang/lib/CIR/CodeGen/CIRGenExpr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,10 @@ void CIRGenFunction::emitStoreThroughLValue(RValue src, LValue dst,
return;
}

assert(dst.isBitField() && "NIY LValue type");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assert(dst.isBitField() && "NIY LValue type");
assert(dst.isBitField() && "NYI LValue type");

Also, why is this an assert here for an NYI? This should use the errorNYI stuff instead, perhaps just put htis in a isBitField branch?

emitStoreThroughBitfieldLValue(src, dst);
return;

cgm.errorNYI(dst.getPointer().getLoc(),
"emitStoreThroughLValue: non-simple lvalue");
return;
Expand Down Expand Up @@ -321,9 +325,20 @@ void CIRGenFunction::emitStoreOfScalar(mlir::Value value, Address addr,

mlir::Value CIRGenFunction::emitStoreThroughBitfieldLValue(RValue src,
LValue dst) {
assert(!cir::MissingFeatures::bitfields());
cgm.errorNYI("bitfields");
return {};

assert(!cir::MissingFeatures::armComputeVolatileBitfields());

const CIRGenBitFieldInfo &info = dst.getBitFieldInfo();
mlir::Type resLTy = convertTypeForMem(dst.getType());
Address ptr = dst.getBitFieldAddress();

const bool useVolatile = false;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can weput the MissingFeatures assert right next to this?


mlir::Value dstAddr = dst.getAddress().getPointer();

return builder.createSetBitfield(dstAddr.getLoc(), resLTy, dstAddr,
ptr.getElementType(), src.getValue(), info,
dst.isVolatileQualified(), useVolatile);
}

RValue CIRGenFunction::emitLoadOfBitfieldLValue(LValue lv, SourceLocation loc) {
Expand Down Expand Up @@ -1063,10 +1078,10 @@ LValue CIRGenFunction::emitBinaryOperatorLValue(const BinaryOperator *e) {

SourceLocRAIIObject loc{*this, getLoc(e->getSourceRange())};
if (lv.isBitField()) {
cgm.errorNYI(e->getSourceRange(), "bitfields");
return {};
emitStoreThroughBitfieldLValue(rv, lv);
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't use curleys on single-line if/else.

emitStoreThroughLValue(rv, lv);
}
emitStoreThroughLValue(rv, lv);

if (getLangOpts().OpenMP) {
cgm.errorNYI(e->getSourceRange(), "openmp");
Expand Down
2 changes: 2 additions & 0 deletions clang/lib/CIR/CodeGen/CIRGenValue.h
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,8 @@ class LValue {
bool isBitField() const { return lvType == BitField; }
bool isVolatile() const { return quals.hasVolatile(); }

bool isVolatileQualified() const { return quals.hasVolatile(); }

unsigned getVRQualifiers() const {
return quals.getCVRQualifiers() & ~clang::Qualifiers::Const;
}
Expand Down
77 changes: 77 additions & 0 deletions clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2049,6 +2049,7 @@ void ConvertCIRToLLVMPass::runOnOperation() {
CIRToLLVMGetGlobalOpLowering,
CIRToLLVMGetMemberOpLowering,
CIRToLLVMSelectOpLowering,
CIRToLLVMSetBitfieldOpLowering,
CIRToLLVMShiftOpLowering,
CIRToLLVMStackRestoreOpLowering,
CIRToLLVMStackSaveOpLowering,
Expand Down Expand Up @@ -2384,6 +2385,82 @@ mlir::LogicalResult CIRToLLVMComplexImagOpLowering::matchAndRewrite(
return mlir::success();
}

mlir::LogicalResult CIRToLLVMSetBitfieldOpLowering::matchAndRewrite(
cir::SetBitfieldOp op, OpAdaptor adaptor,
mlir::ConversionPatternRewriter &rewriter) const {
mlir::OpBuilder::InsertionGuard guard(rewriter);
rewriter.setInsertionPoint(op);

cir::BitfieldInfoAttr info = op.getBitfieldInfo();
uint64_t size = info.getSize();
uint64_t offset = info.getOffset();
mlir::Type storageType = info.getStorageType();
mlir::MLIRContext *context = storageType.getContext();

unsigned storageSize = 0;

mlir::IntegerType intType =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pattern happened in a recent PR, is it worth having a shared helper for getting this integer type?

TypeSwitch<mlir::Type, mlir::IntegerType>(storageType)
.Case<cir::ArrayType>([&](cir::ArrayType atTy) {
storageSize = atTy.getSize() * 8;
return mlir::IntegerType::get(context, storageSize);
})
.Case<cir::IntType>([&](cir::IntType intTy) {
storageSize = intTy.getWidth();
return mlir::IntegerType::get(context, storageSize);
})
.Default([](mlir::Type) -> mlir::IntegerType {
llvm_unreachable(
"Either ArrayType or IntType expected for bitfields storage");
});

mlir::Value srcVal = createIntCast(rewriter, adaptor.getSrc(), intType);
unsigned srcWidth = storageSize;
mlir::Value resultVal = srcVal;

if (storageSize != size) {
assert(storageSize > size && "Invalid bitfield size.");

mlir::Value val = rewriter.create<mlir::LLVM::LoadOp>(
op.getLoc(), intType, adaptor.getAddr(), /* alignment */ 0,
op.getIsVolatile());

srcVal =
createAnd(rewriter, srcVal, llvm::APInt::getLowBitsSet(srcWidth, size));
resultVal = srcVal;
srcVal = createShL(rewriter, srcVal, offset);

// Mask out the original value.
val = createAnd(rewriter, val,
~llvm::APInt::getBitsSet(srcWidth, offset, offset + size));

// Or together the unchanged values and the source value.
srcVal = rewriter.create<mlir::LLVM::OrOp>(op.getLoc(), val, srcVal);
}

rewriter.create<mlir::LLVM::StoreOp>(op.getLoc(), srcVal, adaptor.getAddr(),
/* alignment */ 0, op.getIsVolatile());

mlir::Type resultTy = getTypeConverter()->convertType(op.getType());

if (info.getIsSigned()) {
assert(size <= storageSize);
unsigned highBits = storageSize - size;

if (highBits) {
resultVal = createShL(rewriter, resultVal, highBits);
resultVal = createAShR(rewriter, resultVal, highBits);
}
}

resultVal = createIntCast(rewriter, resultVal,
mlir::cast<mlir::IntegerType>(resultTy),
info.getIsSigned());

rewriter.replaceOp(op, resultVal);
return mlir::success();
}

mlir::LogicalResult CIRToLLVMGetBitfieldOpLowering::matchAndRewrite(
cir::GetBitfieldOp op, OpAdaptor adaptor,
mlir::ConversionPatternRewriter &rewriter) const {
Expand Down
10 changes: 10 additions & 0 deletions clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.h
Original file line number Diff line number Diff line change
Expand Up @@ -513,6 +513,16 @@ class CIRToLLVMComplexImagOpLowering
mlir::ConversionPatternRewriter &) const override;
};

class CIRToLLVMSetBitfieldOpLowering
: public mlir::OpConversionPattern<cir::SetBitfieldOp> {
public:
using mlir::OpConversionPattern<cir::SetBitfieldOp>::OpConversionPattern;

mlir::LogicalResult
matchAndRewrite(cir::SetBitfieldOp op, OpAdaptor,
mlir::ConversionPatternRewriter &) const override;
};

class CIRToLLVMGetBitfieldOpLowering
: public mlir::OpConversionPattern<cir::GetBitfieldOp> {
public:
Expand Down
26 changes: 26 additions & 0 deletions clang/test/CIR/CodeGen/bitfields.c
Original file line number Diff line number Diff line change
Expand Up @@ -134,3 +134,29 @@ unsigned int load_field_unsigned(A* s) {
//OGCG: [[TMP4:%.*]] = lshr i16 [[TMP3]], 3
//OGCG: [[TMP5:%.*]] = and i16 [[TMP4]], 15
//OGCG: [[TMP6:%.*]] = zext i16 [[TMP5]] to i32

void store_field() {
S s;
s.e = 3;
}
// CIR: cir.func {{.*@store_field}}
// CIR: [[TMP0:%.*]] = cir.alloca !rec_S, !cir.ptr<!rec_S>
// CIR: [[TMP1:%.*]] = cir.const #cir.int<3> : !s32i
// CIR: [[TMP2:%.*]] = cir.get_member [[TMP0]][1] {name = "e"} : !cir.ptr<!rec_S> -> !cir.ptr<!u16i>
// CIR: cir.set_bitfield(#bfi_e, [[TMP2]] : !cir.ptr<!u16i>, [[TMP1]] : !s32i)

// LLVM: define dso_local void @store_field()
// LLVM: [[TMP0:%.*]] = alloca %struct.S, i64 1, align 4
// LLVM: [[TMP1:%.*]] = getelementptr %struct.S, ptr [[TMP0]], i32 0, i32 1
// LLVM: [[TMP2:%.*]] = load i16, ptr [[TMP1]], align 2
// LLVM: [[TMP3:%.*]] = and i16 [[TMP2]], -32768
// LLVM: [[TMP4:%.*]] = or i16 [[TMP3]], 3
// LLVM: store i16 [[TMP4]], ptr [[TMP1]], align 2

// OGCG: define dso_local void @store_field()
// OGCG: [[TMP0:%.*]] = alloca %struct.S, align 4
// OGCG: [[TMP1:%.*]] = getelementptr inbounds nuw %struct.S, ptr [[TMP0]], i32 0, i32 1
// OGCG: [[TMP2:%.*]] = load i16, ptr [[TMP1]], align 4
// OGCG: [[TMP3:%.*]] = and i16 [[TMP2]], -32768
// OGCG: [[TMP4:%.*]] = or i16 [[TMP3]], 3
// OGCG: store i16 [[TMP4]], ptr [[TMP1]], align 4
25 changes: 25 additions & 0 deletions clang/test/CIR/CodeGen/bitfields.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -58,3 +58,28 @@ int load_field(S* s) {
// OGCG: [[TMP3:%.*]] = shl i64 [[TMP2]], 15
// OGCG: [[TMP4:%.*]] = ashr i64 [[TMP3]], 47
// OGCG: [[TMP5:%.*]] = trunc i64 [[TMP4]] to i32

void store_field() {
S s;
s.a = 3;
}
// CIR: cir.func dso_local @_Z11store_field
// CIR: [[TMP0:%.*]] = cir.alloca !rec_S, !cir.ptr<!rec_S>
// CIR: [[TMP1:%.*]] = cir.const #cir.int<3> : !s32i
// CIR: [[TMP2:%.*]] = cir.get_member [[TMP0]][0] {name = "a"} : !cir.ptr<!rec_S> -> !cir.ptr<!u64i>
// CIR: cir.set_bitfield(#bfi_a, [[TMP2]] : !cir.ptr<!u64i>, [[TMP1]] : !s32i)

// LLVM: define dso_local void @_Z11store_fieldv
// LLVM: [[TMP0:%.*]] = alloca %struct.S, i64 1, align 4
// LLVM: [[TMP1:%.*]] = getelementptr %struct.S, ptr [[TMP0]], i32 0, i32 0
// LLVM: [[TMP2:%.*]] = load i64, ptr [[TMP1]], align 8
// LLVM: [[TMP3:%.*]] = and i64 [[TMP2]], -16
// LLVM: [[TMP4:%.*]] = or i64 [[TMP3]], 3
// LLVM: store i64 [[TMP4]], ptr [[TMP1]], align 8

// OGCG: define dso_local void @_Z11store_fieldv()
// OGCG: [[TMP0:%.*]] = alloca %struct.S, align 4
// OGCG: [[TMP1:%.*]] = load i64, ptr [[TMP0]], align 4
// OGCG: [[TMP2:%.*]] = and i64 [[TMP1]], -16
// OGCG: [[TMP3:%.*]] = or i64 [[TMP2]], 3
// OGCG: store i64 [[TMP3]], ptr [[TMP0]], align 4
Loading