-
Notifications
You must be signed in to change notification settings - Fork 14.4k
[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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -1669,6 +1669,94 @@ 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 store of the | ||||||||||||||||||||||||
bitfield. | ||||||||||||||||||||||||
```mlir | ||||||||||||||||||||||||
cir.set_bitfield(#bfi, %0 : !cir.ptr<!u32i>, %1 : !s32i) {is_volatile} | ||||||||||||||||||||||||
-> !s32i | ||||||||||||||||||||||||
``` | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
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 | ||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||||||||||||||||||||
``` | ||||||||||||||||||||||||
}]; | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
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); | ||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the result of this operation ever used? If so, where? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I noticed that too the return value from the function llvm-project/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp Lines 955 to 961 in 071e302
llvm-project/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp Lines 977 to 980 in 071e302
|
||||||||||||||||||||||||
|
||||||||||||||||||||||||
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 | ||||||||||||||||||||||||
//===----------------------------------------------------------------------===// | ||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2049,6 +2049,7 @@ void ConvertCIRToLLVMPass::runOnOperation() { | |
CIRToLLVMGetGlobalOpLowering, | ||
CIRToLLVMGetMemberOpLowering, | ||
CIRToLLVMSelectOpLowering, | ||
CIRToLLVMSetBitfieldOpLowering, | ||
CIRToLLVMShiftOpLowering, | ||
CIRToLLVMStackRestoreOpLowering, | ||
CIRToLLVMStackSaveOpLowering, | ||
|
@@ -2384,6 +2385,88 @@ mlir::LogicalResult CIRToLLVMComplexImagOpLowering::matchAndRewrite( | |
return mlir::success(); | ||
} | ||
|
||
mlir::IntegerType computeBitfieldIntType(mlir::Type storageType, | ||
mlir::MLIRContext *context, | ||
unsigned &storageSize) { | ||
return 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::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 = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
computeBitfieldIntType(storageType, context, storageSize); | ||
|
||
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 { | ||
|
@@ -2399,19 +2482,7 @@ mlir::LogicalResult CIRToLLVMGetBitfieldOpLowering::matchAndRewrite( | |
unsigned storageSize = 0; | ||
|
||
mlir::IntegerType intType = | ||
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"); | ||
}); | ||
computeBitfieldIntType(storageType, context, storageSize); | ||
|
||
mlir::Value val = rewriter.create<mlir::LLVM::LoadOp>( | ||
op.getLoc(), intType, adaptor.getAddr(), 0, op.getIsVolatile()); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found the original description to be wordy and hard to follow. I was not clear whether the value returned is the entire value stored to the memory or the value that was written to the bitfield. Experimenting with the incubator, it appears to be the value that was written to the bitfield, with truncation and sign extension as needed, but I couldn't see any uses of this value.