-
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,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 | ||
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 | ||
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); | ||
|
||
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 | ||||
---|---|---|---|---|---|---|
|
@@ -224,6 +224,10 @@ void CIRGenFunction::emitStoreThroughLValue(RValue src, LValue dst, | |||||
return; | ||||||
} | ||||||
|
||||||
assert(dst.isBitField() && "NIY LValue 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.
Suggested change
Also, why is this an assert here for an NYI? This should use the errorNYI stuff instead, perhaps just put htis in a |
||||||
emitStoreThroughBitfieldLValue(src, dst); | ||||||
return; | ||||||
|
||||||
cgm.errorNYI(dst.getPointer().getLoc(), | ||||||
"emitStoreThroughLValue: non-simple lvalue"); | ||||||
return; | ||||||
|
@@ -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; | ||||||
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. 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) { | ||||||
|
@@ -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 { | ||||||
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. don't use curleys on single-line if/else. |
||||||
emitStoreThroughLValue(rv, lv); | ||||||
} | ||||||
emitStoreThroughLValue(rv, lv); | ||||||
|
||||||
if (getLangOpts().OpenMP) { | ||||||
cgm.errorNYI(e->getSourceRange(), "openmp"); | ||||||
|
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,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 = | ||
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? |
||
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 { | ||
|
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.
Do you have any tests below for a 'volatile' load'? Also the store should be volatile too, right? Should we mention that more elequently?
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.
Perhaps adding an example would also be helpful