-
Notifications
You must be signed in to change notification settings - Fork 15.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
Changes from 3 commits
38b98a5
47fbf70
b960b63
cb837e5
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. | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| A bitfield info attribute must be provided to describe the location of | ||||||||||||||||||||||||
| the bitfield within the memory referenced by the $addr argument. | ||||||||||||||||||||||||
| The $src argument is inserted at the appropriate place in the memory and | ||||||||||||||||||||||||
| the value that was stored. 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 | ||||||||||||||||||||||||
|
Member
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
Contributor
Author
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) -> !s32iOr without the comma? cir.set_bitfield #bfi_e %3, %1 : (!cir.ptr<!u16i>, !s32i) -> !s32iAnd 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); | ||||||||||||||||||||||||
|
Contributor
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?
Contributor
Author
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
Contributor
Author
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. It is also used in the incubator: In the incubator, the result was passed as the
Contributor
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. I see that you're right. Can you add a test case where the return value is used? This looks like it does that in C (though not in C++):
Contributor
Author
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. Ok, it would also be used in the case of a unary operation when it is upstream. void unOp(S* s) {
s->c++;
}
Contributor
Author
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 |
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| 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 = | ||
|
Member
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?
Contributor
Author
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.
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.
Done