Skip to content
Merged
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
88 changes: 88 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,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 the 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
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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the result of this operation ever used? If so, where?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I noticed that too the return value from the function emitStoreThroughBitfieldLValue, which returns the set_bitfield value, is always discarded. However, I found that it is actually used in the function VisitBinAssign:

// Store the value into the LHS. Bit-fields are handled specially because
// the result is altered by the store, i.e., [C99 6.5.16p1]
// 'An assignment expression has the value of the left operand after the
// assignment...'.
if (lhs.isBitField()) {
rhs = cgf.emitStoreThroughBitfieldLValue(RValue::get(rhs), lhs);
} else {

// If the lvalue is non-volatile, return the computed value of the
// assignment.
if (!lhs.isVolatile())
return rhs;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

The 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++):

typedef struct {
  int a : 4;
  int b : 4;
  int c : 4;
} S;

void set_bitfield(volatile S* s) {
  s->a = s->b = 3;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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++;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
//===----------------------------------------------------------------------===//
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
31 changes: 23 additions & 8 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() && "Unknown LValue type");
emitStoreThroughBitfieldLValue(src, dst);
return;

cgm.errorNYI(dst.getPointer().getLoc(),
"emitStoreThroughLValue: non-simple lvalue");
return;
Expand Down Expand Up @@ -321,9 +325,21 @@ 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();

assert(!cir::MissingFeatures::armComputeVolatileBitfields());
const bool useVolatile = false;

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 @@ -1062,11 +1078,10 @@ LValue CIRGenFunction::emitBinaryOperatorLValue(const BinaryOperator *e) {
LValue lv = emitLValue(e->getLHS());

SourceLocRAIIObject loc{*this, getLoc(e->getSourceRange())};
if (lv.isBitField()) {
cgm.errorNYI(e->getSourceRange(), "bitfields");
return {};
}
emitStoreThroughLValue(rv, lv);
if (lv.isBitField())
emitStoreThroughBitfieldLValue(rv, lv);
else
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
97 changes: 84 additions & 13 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,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 =
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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 {
Expand All @@ -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());
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
Loading