Skip to content

[CIR] Upstream get_bitfield operation to load bit-field members from structs #145971

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 5 commits 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
63 changes: 63 additions & 0 deletions clang/include/clang/CIR/Dialect/IR/CIRAttrs.td
Original file line number Diff line number Diff line change
Expand Up @@ -375,4 +375,67 @@ def CIR_VisibilityAttr : CIR_EnumAttr<CIR_VisibilityKind, "visibility"> {
}];
}

//===----------------------------------------------------------------------===//
// BitfieldInfoAttr
//===----------------------------------------------------------------------===//

def BitfieldInfoAttr : CIR_Attr<"BitfieldInfo", "bitfield_info"> {
let summary = "Represents a bit field info";
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
let summary = "Represents a bit field info";
let summary = "Represents info for a bit-field member";

let description = [{
Holds the following information about bitfields: name, storage type, size
and position in the storage, and signedness.
Example:
Given the following struct with bitfields:
```c++
typedef struct {
int a : 4;
int b : 27;
int c : 17;
int d : 2;
int e : 15;
} S;
```

The CIR representation of the struct `S` might look like:
```mlir
!rec_S = !cir.record<struct "S" packed padded {!u64i, !u16i,
!cir.array<!u8i x 2>}>
```
And the bitfield info attribute for member `a` would be:
```mlir
#bfi_a = #cir.bitfield_info<name = "a", storage_type = !u64i,
size = 4, offset = 0, is_signed = true>
```

This metadata describes that field `a` is stored in a 64-bit integer,
is 4 bits wide, starts at offset 0, and is signed.
}];
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add an example?

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 parameters = (ins "mlir::StringAttr":$name,
"mlir::Type":$storageType,
"uint64_t":$size,
"uint64_t":$offset,
"bool":$isSigned);

let assemblyFormat = [{`<` struct($name,
$storageType,
$size,
$offset,
$isSigned)
`>`
}];

let builders = [
AttrBuilder<(ins "llvm::StringRef":$name,
"mlir::Type":$storageType,
"uint64_t":$size,
"uint64_t":$offset,
"bool":$isSigned
), [{
return $_get($_ctxt, mlir::StringAttr::get($_ctxt, name), storageType,
size, offset, isSigned);
}]>
];
}


#endif // LLVM_CLANG_CIR_DIALECT_IR_CIRATTRS_TD
79 changes: 79 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,85 @@ def GetGlobalOp : CIR_Op<"get_global",
}];
}

//===----------------------------------------------------------------------===//
// GetBitfieldOp
//===----------------------------------------------------------------------===//

def GetBitfieldOp : CIR_Op<"get_bitfield"> {
let summary = "Get a bitfield";
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
let summary = "Get a bitfield";
let summary = "Get the information for a bitfield member";

let description = [{
The `cir.get_bitfield` operation provides a load-like access to
a bit field of a record.

It expects a name if a bit field, a pointer to a storage in the
base record, a type of the storage, a name of the bitfield,
a size the bit field, an offset of the bit field and a sign.

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.get_bitfield` operation gets the value
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
different storages. The `cir.get_bitfield` operation gets the value
different members. The `cir.get_bitfield` operation gets the value

of the bitfield
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
of the bitfield
of the bitfield.

```C++
typedef struct {
int a : 4;
int b : 27;
int c : 17;
int d : 2;
int e : 15;
} S;

int load_bitfield(S& s) {
return s.e;
}
```

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

%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.get_bitfield(#bfi_e, %3 : !cir.ptr<!u16i>) -> !s32i
```
}];

let arguments = (ins
Arg<CIR_PointerType, "the address to load from", [MemRead]>:$addr,
BitfieldInfoAttr:$bitfield_info,
UnitAttr:$is_volatile
);

let results = (outs CIR_IntType:$result);

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

let builders = [
OpBuilder<(ins "mlir::Type":$type,
"mlir::Value":$addr,
"mlir::Type":$storage_type,
"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, info, is_volatile);
}]>
];
}

//===----------------------------------------------------------------------===//
// GetMemberOp
//===----------------------------------------------------------------------===//
Expand Down
14 changes: 14 additions & 0 deletions clang/include/clang/CIR/LoweringHelpers.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,4 +37,18 @@ std::optional<mlir::Attribute>
lowerConstArrayAttr(cir::ConstArrayAttr constArr,
const mlir::TypeConverter *converter);

mlir::Value getConstAPInt(mlir::OpBuilder &bld, mlir::Location loc,
mlir::Type typ, const llvm::APInt &val);

mlir::Value getConst(mlir::OpBuilder &bld, mlir::Location loc, mlir::Type typ,
unsigned val);

mlir::Value createShL(mlir::OpBuilder &bld, mlir::Value lhs, unsigned rhs);

mlir::Value createAShR(mlir::OpBuilder &bld, mlir::Value lhs, unsigned rhs);

mlir::Value createAnd(mlir::OpBuilder &bld, mlir::Value lhs,
const llvm::APInt &rhs);

mlir::Value createLShR(mlir::OpBuilder &bld, mlir::Value lhs, unsigned rhs);
#endif
10 changes: 10 additions & 0 deletions clang/lib/CIR/CodeGen/CIRGenBuilder.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#define LLVM_CLANG_LIB_CIR_CODEGEN_CIRGENBUILDER_H

#include "Address.h"
#include "CIRGenRecordLayout.h"
#include "CIRGenTypeCache.h"
#include "clang/CIR/Interfaces/CIRFPTypeInterface.h"
#include "clang/CIR/MissingFeatures.h"
Expand Down Expand Up @@ -405,6 +406,15 @@ class CIRGenBuilderTy : public cir::CIRBaseBuilderTy {

return createGlobal(module, loc, uniqueName, type, linkage);
}

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

} // namespace clang::CIRGen
Expand Down
60 changes: 56 additions & 4 deletions clang/lib/CIR/CodeGen/CIRGenExpr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -326,13 +326,62 @@ mlir::Value CIRGenFunction::emitStoreThroughBitfieldLValue(RValue src,
return {};
}

RValue CIRGenFunction::emitLoadOfBitfieldLValue(LValue lv, SourceLocation loc) {
const CIRGenBitFieldInfo &info = lv.getBitFieldInfo();

// Get the output type.
mlir::Type resLTy = convertType(lv.getType());
Address ptr = lv.getBitFieldAddress();

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

mlir::Value field = builder.createGetBitfield(getLoc(loc), resLTy, ptr.getPointer(),
ptr.getElementType(), info,
lv.isVolatile(), false);
assert(!cir::MissingFeatures::opLoadEmitScalarRangeCheck() && "NYI");
return RValue::get(field);
}

Address CIRGenFunction::getAddrOfBitFieldStorage(LValue base,
const FieldDecl *field,
mlir::Type fieldType,
unsigned index) {
mlir::Location loc = getLoc(field->getLocation());
cir::PointerType fieldPtr = cir::PointerType::get(fieldType);
cir::GetMemberOp sea = getBuilder().createGetMember(
loc, fieldPtr, base.getPointer(), field->getName(), index);
return Address(sea, CharUnits::One());
}

LValue CIRGenFunction::emitLValueForBitField(LValue base,
const FieldDecl *field) {
LValueBaseInfo baseInfo = base.getBaseInfo();
const CIRGenRecordLayout &layout =
cgm.getTypes().getCIRGenRecordLayout(field->getParent());
const CIRGenBitFieldInfo &info = layout.getBitFieldInfo(field);
assert(!cir::MissingFeatures::armComputeVolatileBitfields());
assert(!cir::MissingFeatures::preservedAccessIndexRegion());
unsigned idx = layout.getCIRFieldNo(field);

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a missing feature marker here for preservedAccessIndexRegion? It's not in the incubator code, but classic codegen has something here that we're missing.

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

Address addr = getAddrOfBitFieldStorage(base, field, info.storageType, idx);

mlir::Location loc = getLoc(field->getLocation());
if (addr.getElementType() != info.storageType)
addr = builder.createElementBitCast(loc, addr, info.storageType);

QualType fieldType =
field->getType().withCVRQualifiers(base.getVRQualifiers());
// TODO(cir): Support TBAA for bit fields.
assert(!cir::MissingFeatures::opTBAA());
LValueBaseInfo fieldBaseInfo(baseInfo.getAlignmentSource());
return LValue::makeBitfield(addr, info, fieldType, fieldBaseInfo);
}

LValue CIRGenFunction::emitLValueForField(LValue base, const FieldDecl *field) {
LValueBaseInfo baseInfo = base.getBaseInfo();

if (field->isBitField()) {
cgm.errorNYI(field->getSourceRange(), "emitLValueForField: bitfield");
return LValue();
}
if (field->isBitField())
return emitLValueForBitField(base, field);

QualType fieldType = field->getType();
const RecordDecl *rec = field->getParent();
Expand Down Expand Up @@ -460,6 +509,9 @@ RValue CIRGenFunction::emitLoadOfLValue(LValue lv, SourceLocation loc) {
assert(!lv.getType()->isFunctionType());
assert(!(lv.getType()->isConstantMatrixType()) && "not implemented");

if (lv.isBitField())
return emitLoadOfBitfieldLValue(lv, loc);

if (lv.isSimple())
return RValue::get(emitLoadOfScalar(lv, loc));

Expand Down
6 changes: 6 additions & 0 deletions clang/lib/CIR/CodeGen/CIRGenFunction.h
Original file line number Diff line number Diff line change
Expand Up @@ -550,6 +550,9 @@ class CIRGenFunction : public CIRGenTypeCache {
return it->second;
}

Address getAddrOfBitFieldStorage(LValue base, const clang::FieldDecl *field,
mlir::Type fieldType, unsigned index);

/// Load the value for 'this'. This function is only valid while generating
/// code for an C++ member function.
/// FIXME(cir): this should return a mlir::Value!
Expand Down Expand Up @@ -944,6 +947,8 @@ class CIRGenFunction : public CIRGenTypeCache {
/// ignoring the result.
void emitIgnoredExpr(const clang::Expr *e);

RValue emitLoadOfBitfieldLValue(LValue lv, SourceLocation loc);

/// Given an expression that represents a value lvalue, this method emits
/// the address of the lvalue, then loads the result as an rvalue,
/// returning the rvalue.
Expand All @@ -964,6 +969,7 @@ class CIRGenFunction : public CIRGenTypeCache {
/// of the expression.
/// FIXME: document this function better.
LValue emitLValue(const clang::Expr *e);
LValue emitLValueForBitField(LValue base, const FieldDecl *field);
LValue emitLValueForField(LValue base, const clang::FieldDecl *field);

/// Like emitLValueForField, excpet that if the Field is a reference, this
Expand Down
34 changes: 34 additions & 0 deletions clang/lib/CIR/CodeGen/CIRGenValue.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include "clang/AST/CharUnits.h"
#include "clang/AST/Type.h"

#include "CIRGenRecordLayout.h"
#include "mlir/IR/Value.h"

#include "clang/CIR/MissingFeatures.h"
Expand Down Expand Up @@ -162,6 +163,7 @@ class LValue {
mlir::Value vectorIdx; // Index for vector subscript
mlir::Type elementType;
LValueBaseInfo baseInfo;
const CIRGenBitFieldInfo *bitFieldInfo{nullptr};

void initialize(clang::QualType type, clang::Qualifiers quals,
clang::CharUnits alignment, LValueBaseInfo baseInfo) {
Expand Down Expand Up @@ -245,6 +247,38 @@ class LValue {
r.initialize(t, t.getQualifiers(), vecAddress.getAlignment(), baseInfo);
return r;
}

// bitfield lvalue
Address getBitFieldAddress() const {
return Address(getBitFieldPointer(), elementType, getAlignment());
}

mlir::Value getBitFieldPointer() const {
assert(isBitField());
return v;
}

const CIRGenBitFieldInfo &getBitFieldInfo() const {
assert(isBitField());
return *bitFieldInfo;
}

/// Create a new object to represent a bit-field access.
///
/// \param Addr - The base address of the bit-field sequence this
/// bit-field refers to.
/// \param Info - The information describing how to perform the bit-field
/// access.
static LValue makeBitfield(Address addr, const CIRGenBitFieldInfo &info,
clang::QualType type, LValueBaseInfo baseInfo) {
LValue r;
r.lvType = BitField;
r.v = addr.getPointer();
r.elementType = addr.getElementType();
r.bitFieldInfo = &info;
r.initialize(type, type.getQualifiers(), addr.getAlignment(), baseInfo);
return r;
}
};

/// An aggregate value slot.
Expand Down
4 changes: 4 additions & 0 deletions clang/lib/CIR/Dialect/IR/CIRDialect.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,10 @@ struct CIROpAsmDialectInterface : public OpAsmDialectInterface {
os << (boolAttr.getValue() ? "true" : "false");
return AliasResult::FinalAlias;
}
if (auto bitfield = mlir::dyn_cast<cir::BitfieldInfoAttr>(attr)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add an IR test to make sure this gets parsed correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to add an IR test for, but I ran into the following parse error when trying to define a struct:

erro.cir:11:21: error: complete records are not yet supported
!rec_D = !cir.record<struct "D" {!u16i, !s32i}>
                    ^

} else if (!incomplete) { // complete

Without the struct definition, I can't meaningfully call get_member or get_bitfield, and the resulting module ends up being empty. That makes it impossible to use CHECK directives to verify anything.

Should I consider it sufficient to check that the input runs without errors for now?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I see the problem with parsing complete records. You're right that you can't add your test until that is upstreamed. I think you can just proceed without the test for now. We can add it as a separate PR later.

os << "bfi_" << bitfield.getName().str();
return AliasResult::FinalAlias;
}
return AliasResult::NoAlias;
}
};
Expand Down
Loading