Skip to content

[CIR] Unblock simple C++ structure support #138368

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

Merged
merged 3 commits into from
May 7, 2025
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
9 changes: 6 additions & 3 deletions clang/lib/CIR/CodeGen/CIRGenExpr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -322,9 +322,12 @@ LValue CIRGenFunction::emitLValueForField(LValue base, const FieldDecl *field) {
assert(!cir::MissingFeatures::opTBAA());

Address addr = base.getAddress();
if (isa<CXXRecordDecl>(rec)) {
cgm.errorNYI(field->getSourceRange(), "emitLValueForField: C++ class");
return LValue();
if (auto *classDecl = dyn_cast<CXXRecordDecl>(rec)) {
if (cgm.getCodeGenOpts().StrictVTablePointers &&
classDecl->isDynamicClass()) {
cgm.errorNYI(field->getSourceRange(),
"emitLValueForField: strict vtable for dynamic class");
}
}

unsigned recordCVR = base.getVRQualifiers();
Expand Down
32 changes: 27 additions & 5 deletions clang/lib/CIR/CodeGen/CIRGenExprConstant.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include "CIRGenConstantEmitter.h"
#include "CIRGenFunction.h"
#include "CIRGenModule.h"
#include "CIRGenRecordLayout.h"
#include "mlir/IR/Attributes.h"
#include "mlir/IR/BuiltinAttributeInterfaces.h"
#include "mlir/IR/BuiltinAttributes.h"
Expand Down Expand Up @@ -364,12 +365,33 @@ mlir::Attribute ConstantEmitter::tryEmitPrivateForVarInit(const VarDecl &d) {
// initialization of memory to all NULLs.
if (!d.hasLocalStorage()) {
QualType ty = cgm.getASTContext().getBaseElementType(d.getType());
if (ty->isRecordType())
if (d.getInit() && isa<CXXConstructExpr>(d.getInit())) {
cgm.errorNYI(d.getInit()->getBeginLoc(),
"tryEmitPrivateForVarInit CXXConstructExpr");
return {};
if (ty->isRecordType()) {
if (const auto *e = dyn_cast_or_null<CXXConstructExpr>(d.getInit())) {
const CXXConstructorDecl *cd = e->getConstructor();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does classic code-gen assume that cd is non-null here? I'm not positive it always is.

Also, do we/should we model elidable here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Classic codegen does assume that cd is non-null here. I'm pretty sure that if the dyn_cast_or_null gives us a CXXConstructorExpr we can count on if having a CXXConstructorDecl.

The AST handles any decisions about which constructors are to be called. Currently, the incubator just represents constructors, when needed, as cir.call operations. It may be worth doing more than that. In the case of code like this:

void f() {
    MyClass obj = func();
}

The AST says this:

-FunctionDecl <line:12:1, line:14:1> line:12:6 f 'void ()'
  `-CompoundStmt <col:10, line:14:1>
    `-DeclStmt <line:13:5, col:25>
      `-VarDecl <col:5, col:24> col:13 obj 'MyClass' cinit
        `-CallExpr <col:19, col:24> 'MyClass'
          `-ImplicitCastExpr <col:19> 'MyClass (*)()' <FunctionToPointerDecay>
            `-DeclRefExpr <col:19> 'MyClass ()' lvalue Function 0x216ac150 'func' 'MyClass ()'

And the incubator generates this CIR:

  cir.func @_Z1fv() {
    %0 = cir.alloca !rec_MyClass, !cir.ptr<!rec_MyClass>, ["obj", init] {alignment = 1 : i64}
    %1 = cir.call @_Z4funcv() : () -> !rec_MyClass
    cir.store %1, %0 : !rec_MyClass, !cir.ptr<!rec_MyClass>
    cir.return
  }

Is your question whether we should do something here to explicitly show that the copy constructor was elided?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, glad to know. Sometimes the decl elements of an expr can be null in subtle situations.

As far as elidable. There is a CXXConstructExpr::isElidable that I'm surprised we don't have to express here, since the idea is that we could skip the construction.

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 see. If I add -std=c++14 to my compile line, that gets the elidable constructor in the AST, and then -fno-elide-constructors get us to emit CIR for it but not here. That's handled in emitCXXConstructExpr (which has not been upstreamed yet). I guess the cd->isTrivial() && cd->isDefaultConstructor() check prevents us from doing anything with it here.

Copy link
Member

Choose a reason for hiding this comment

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

We have been careful not to elide things too early since it might hurt static analysis - this is a big pain point and request from the static analysis community (as clang really early get rid of these). The idea is to represent them in a way that during LoweringPrepare we can either elide them out or apply specific lowering heuristics (e.g. when deciding the best way to initialize large set of constant datas).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bcardosolopes Does that mean that we should be emitting an elidable constructor in CIR even if the -fno-elide-constructors option isn't used? The incubator is eliding them the same way classic codegen does.

For the case without -std=c++14 I'm not sure we have much choice since it's not even in the AST.

// FIXME: we should probably model this more closely to C++ than
// just emitting a global with zero init (mimic what we do for trivial
// assignments and whatnots). Since this is for globals shouldn't
// be a problem for the near future.
if (cd->isTrivial() && cd->isDefaultConstructor()) {
const auto *cxxrd =
cast<CXXRecordDecl>(ty->getAs<RecordType>()->getDecl());
if (cxxrd->getNumBases() != 0) {
// There may not be anything additional to do here, but this will
// force us to pause and test this path when it is supported.
cgm.errorNYI("tryEmitPrivateForVarInit: cxx record with bases");
return {};
}
if (!cgm.getTypes().isZeroInitializable(cxxrd)) {
// To handle this case, we really need to go through
// emitNullConstant, but we need an attribute, not a value
cgm.errorNYI(
"tryEmitPrivateForVarInit: non-zero-initializable cxx record");
return {};
}
return cir::ZeroAttr::get(cgm.convertType(d.getType()));
Copy link
Contributor

Choose a reason for hiding this comment

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

OGCG calls emitNullConstant here. We have CIRGenModule::emitNullConstant partially implemented. In the incubator CIRGenModule::emitNullConstant fails with a NYi if the record type is not zero initializable and IMO that's a reasonable limitation to have for now (instead of silently breaking vtables and default ctors).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that seems reasonable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I tried to implement this suggestion, it quickly came to my attention that emitNullConstant returns a value, and we need an attribute here. I think the cd->isTrivial() && cd->isDefaultConstructor() check should prevent us from getting here in any case where the zero attribute isn't sufficient, but I will add an assert that the type is zero-initializable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Second complication, the clang AST says that CompleteS is not zero-initializable for the test case I'm adding in struct.cpp. (But classic codegen generates zeroinitializer for it). I think I may need to implement CIRGenRecordLayout::isZeroInitializableAsBase for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding the errorNYI for structs that are not "truly" zero initializable for a given ABI. LGTM now.

}
}
}
}
inConstantContext = d.hasConstantInitialization();

Expand Down
26 changes: 24 additions & 2 deletions clang/lib/CIR/CodeGen/CIRGenRecordLayout.h
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,23 @@ class CIRGenRecordLayout {
/// field no. This info is populated by the record builder.
llvm::DenseMap<const clang::FieldDecl *, unsigned> fieldIdxMap;

/// False if any direct or indirect subobject of this class, when considered
/// as a complete object, requires a non-zero bitpattern when
/// zero-initialized.
LLVM_PREFERRED_TYPE(bool)
unsigned zeroInitializable : 1;

/// False if any direct or indirect subobject of this class, when considered
/// as a base subobject, requires a non-zero bitpattern when zero-initialized.
LLVM_PREFERRED_TYPE(bool)
unsigned zeroInitializableAsBase : 1;

public:
CIRGenRecordLayout(cir::RecordType completeObjectType)
: completeObjectType(completeObjectType) {}
CIRGenRecordLayout(cir::RecordType completeObjectType, bool zeroInitializable,
bool zeroInitializableAsBase)
: completeObjectType(completeObjectType),
zeroInitializable(zeroInitializable),
zeroInitializableAsBase(zeroInitializableAsBase) {}

/// Return the "complete object" LLVM type associated with
/// this record.
Expand All @@ -47,6 +61,14 @@ class CIRGenRecordLayout {
assert(fieldIdxMap.count(fd) && "Invalid field for record!");
return fieldIdxMap.lookup(fd);
}

/// Check whether this struct can be C++ zero-initialized
/// with a zeroinitializer.
bool isZeroInitializable() const { return zeroInitializable; }

/// Check whether this struct can be C++ zero-initialized
/// with a zeroinitializer when considered as a base subobject.
bool isZeroInitializableAsBase() const { return zeroInitializableAsBase; }
};

} // namespace clang::CIRGen
Expand Down
45 changes: 37 additions & 8 deletions clang/lib/CIR/CodeGen/CIRGenRecordLayoutBuilder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,8 @@ struct CIRRecordLowering final {
return astContext.toCharUnitsFromBits(bitOffset);
}

void calculateZeroInit();

CharUnits getSize(mlir::Type Ty) {
return CharUnits::fromQuantity(dataLayout.layout.getTypeSize(Ty));
}
Expand Down Expand Up @@ -177,18 +179,26 @@ void CIRRecordLowering::lower() {
return;
}

if (isa<CXXRecordDecl>(recordDecl)) {
cirGenTypes.getCGModule().errorNYI(recordDecl->getSourceRange(),
"lower: class");
return;
}

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

CharUnits size = astRecordLayout.getSize();

accumulateFields();

if (const auto *cxxRecordDecl = dyn_cast<CXXRecordDecl>(recordDecl)) {
if (cxxRecordDecl->getNumBases() > 0) {
CIRGenModule &cgm = cirGenTypes.getCGModule();
cgm.errorNYI(recordDecl->getSourceRange(),
"CIRRecordLowering::lower: derived CXXRecordDecl");
return;
}
if (members.empty()) {
appendPaddingBytes(size);
assert(!cir::MissingFeatures::bitfields());
return;
}
}

llvm::stable_sort(members);
// TODO: implement clipTailPadding once bitfields are implemented
assert(!cir::MissingFeatures::bitfields());
Expand All @@ -199,6 +209,7 @@ void CIRRecordLowering::lower() {
insertPadding();
members.pop_back();

calculateZeroInit();
fillOutputFields();
}

Expand Down Expand Up @@ -236,6 +247,19 @@ void CIRRecordLowering::accumulateFields() {
}
}

void CIRRecordLowering::calculateZeroInit() {
for (const MemberInfo &member : members) {
if (member.kind == MemberInfo::InfoKind::Field) {
if (!member.fieldDecl || isZeroInitializable(member.fieldDecl))
continue;
zeroInitializable = zeroInitializableAsBase = false;
return;
}
// TODO(cir): handle base types
assert(!cir::MissingFeatures::cxxSupport());
}
}

void CIRRecordLowering::determinePacked() {
if (packed)
return;
Expand Down Expand Up @@ -295,7 +319,10 @@ CIRGenTypes::computeRecordLayout(const RecordDecl *rd, cir::RecordType *ty) {
// If we're in C++, compute the base subobject type.
if (llvm::isa<CXXRecordDecl>(rd) && !rd->isUnion() &&
!rd->hasAttr<FinalAttr>()) {
cgm.errorNYI(rd->getSourceRange(), "computeRecordLayout: CXXRecordDecl");
if (lowering.astRecordLayout.getNonVirtualSize() !=
lowering.astRecordLayout.getSize()) {
cgm.errorNYI(rd->getSourceRange(), "computeRecordLayout: CXXRecordDecl");
}
}

// Fill in the record *after* computing the base type. Filling in the body
Expand All @@ -304,7 +331,9 @@ CIRGenTypes::computeRecordLayout(const RecordDecl *rd, cir::RecordType *ty) {
assert(!cir::MissingFeatures::astRecordDeclAttr());
ty->complete(lowering.fieldTypes, lowering.packed, lowering.padded);

auto rl = std::make_unique<CIRGenRecordLayout>(ty ? *ty : cir::RecordType());
auto rl = std::make_unique<CIRGenRecordLayout>(
ty ? *ty : cir::RecordType(), (bool)lowering.zeroInitializable,
(bool)lowering.zeroInitializableAsBase);

assert(!cir::MissingFeatures::recordZeroInit());
assert(!cir::MissingFeatures::cxxSupport());
Expand Down
17 changes: 12 additions & 5 deletions clang/lib/CIR/CodeGen/CIRGenTypes.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -237,8 +237,11 @@ mlir::Type CIRGenTypes::convertRecordDeclType(const clang::RecordDecl *rd) {
assert(insertResult && "isSafeToCovert() should have caught this.");

// Force conversion of non-virtual base classes recursively.
if (isa<CXXRecordDecl>(rd)) {
cgm.errorNYI(rd->getSourceRange(), "CXXRecordDecl");
if (const auto *cxxRecordDecl = dyn_cast<CXXRecordDecl>(rd)) {
if (cxxRecordDecl->getNumBases() > 0) {
cgm.errorNYI(rd->getSourceRange(),
"convertRecordDeclType: derived CXXRecordDecl");
}
}

// Layout fields.
Expand Down Expand Up @@ -497,9 +500,9 @@ bool CIRGenTypes::isZeroInitializable(clang::QualType t) {
return true;
}

if (t->getAs<RecordType>()) {
cgm.errorNYI(SourceLocation(), "isZeroInitializable for RecordType", t);
return false;
if (const RecordType *rt = t->getAs<RecordType>()) {
const RecordDecl *rd = rt->getDecl();
return isZeroInitializable(rd);
}

if (t->getAs<MemberPointerType>()) {
Expand All @@ -511,6 +514,10 @@ bool CIRGenTypes::isZeroInitializable(clang::QualType t) {
return true;
}

bool CIRGenTypes::isZeroInitializable(const RecordDecl *rd) {
return getCIRGenRecordLayout(rd).isZeroInitializable();
}

const CIRGenFunctionInfo &CIRGenTypes::arrangeCIRFunctionInfo(
CanQualType returnType, llvm::ArrayRef<clang::CanQualType> argTypes) {
assert(llvm::all_of(argTypes,
Expand Down
1 change: 1 addition & 0 deletions clang/lib/CIR/CodeGen/CIRGenTypes.h
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ class CIRGenTypes {
/// Return whether a type can be zero-initialized (in the C++ sense) with an
/// LLVM zeroinitializer.
bool isZeroInitializable(clang::QualType ty);
bool isZeroInitializable(const RecordDecl *rd);

const CIRGenFunctionInfo &arrangeFreeFunctionCall(const CallArgList &args,
const FunctionType *fnType);
Expand Down
19 changes: 19 additions & 0 deletions clang/test/CIR/CodeGen/nonzeroinit-struct.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// RUN: not %clang_cc1 -triple x86_64-unknown-linux-gnu -fclangir -emit-cir %s -o - 2>&1 | FileCheck %s

struct Other {
int x;
};

struct Trivial {
int x;
double y;
decltype(&Other::x) ptr;
};

// This case has a trivial default constructor, but can't be zero-initialized.
Trivial t;

// Since the case above isn't handled yet, we want a test that verifies that
// we're failing for the right reason.

// CHECK: error: ClangIR code gen Not Yet Implemented: tryEmitPrivateForVarInit: non-zero-initializable cxx record
37 changes: 37 additions & 0 deletions clang/test/CIR/CodeGen/struct.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,17 @@ IncompleteS *p;
// LLVM: @p = dso_local global ptr null
// OGCG: @p = global ptr null, align 8

struct CompleteS {
int a;
char b;
};

CompleteS cs;

// CIR: cir.global external @cs = #cir.zero : !rec_CompleteS
// LLVM-DAG: @cs = dso_local global %struct.CompleteS zeroinitializer
// OGCG-DAG: @cs = global %struct.CompleteS zeroinitializer, align 4

void f(void) {
IncompleteS *p;
}
Expand All @@ -28,3 +39,29 @@ void f(void) {
// OGCG-NEXT: entry:
// OGCG-NEXT: %[[P:.*]] = alloca ptr, align 8
// OGCG-NEXT: ret void

char f2(CompleteS &s) {
return s.b;
}

// CIR: cir.func @_Z2f2R9CompleteS(%[[ARG_S:.*]]: !cir.ptr<!rec_CompleteS>{{.*}})
// CIR: %[[S_ADDR:.*]] = cir.alloca !cir.ptr<!rec_CompleteS>, !cir.ptr<!cir.ptr<!rec_CompleteS>>, ["s", init, const]
// CIR: cir.store %[[ARG_S]], %[[S_ADDR]]
// CIR: %[[S_REF:.*]] = cir.load %[[S_ADDR]]
// CIR: %[[S_ADDR2:.*]] = cir.get_member %[[S_REF]][1] {name = "b"}
// CIR: %[[S_B:.*]] = cir.load %[[S_ADDR2]]

// LLVM: define i8 @_Z2f2R9CompleteS(ptr %[[ARG_S:.*]])
// LLVM: %[[S_ADDR:.*]] = alloca ptr
// LLVM: store ptr %[[ARG_S]], ptr %[[S_ADDR]]
// LLVM: %[[S_REF:.*]] = load ptr, ptr %[[S_ADDR]], align 8
// LLVM: %[[S_ADDR2:.*]] = getelementptr %struct.CompleteS, ptr %[[S_REF]], i32 0, i32 1
// LLVM: %[[S_B:.*]] = load i8, ptr %[[S_ADDR2]]

// OGCG: define{{.*}} i8 @_Z2f2R9CompleteS(ptr{{.*}} %[[ARG_S:.*]])
// OGCG: entry:
// OGCG: %[[S_ADDR:.*]] = alloca ptr
// OGCG: store ptr %[[ARG_S]], ptr %[[S_ADDR]]
// OGCG: %[[S_REF:.*]] = load ptr, ptr %[[S_ADDR]]
// OGCG: %[[S_ADDR2:.*]] = getelementptr inbounds nuw %struct.CompleteS, ptr %[[S_REF]], i32 0, i32 1
// OGCG: %[[S_B:.*]] = load i8, ptr %[[S_ADDR2]]
59 changes: 59 additions & 0 deletions clang/test/CIR/CodeGen/union.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fclangir -emit-cir %s -o %t.cir
// RUN: FileCheck --check-prefix=CIR --input-file=%t.cir %s
// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fclangir -emit-llvm %s -o %t-cir.ll
// RUN: FileCheck --check-prefix=LLVM --input-file=%t-cir.ll %s
// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm %s -o %t.ll
// RUN: FileCheck --check-prefix=OGCG --input-file=%t.ll %s

// Should generate a union type with all members preserved.
union U {
bool b;
short s;
int i;
float f;
double d;
};
// CIR: !rec_U = !cir.record<union "U" {!cir.bool, !s16i, !s32i, !cir.float, !cir.double}>
// LLVM: %union.U = type { double }
// OGCG: %union.U = type { double }

void shouldGenerateUnionAccess(union U u) {
u.b = true;
u.b;
u.i = 1;
u.i;
u.f = 0.1F;
u.f;
u.d = 0.1;
u.d;
}
// CIR: cir.func {{.*}}shouldGenerateUnionAccess
// CIR: %[[#BASE:]] = cir.get_member %0[0] {name = "b"} : !cir.ptr<!rec_U> -> !cir.ptr<!cir.bool>
// CIR: cir.store %{{.+}}, %[[#BASE]] : !cir.bool, !cir.ptr<!cir.bool>
// CIR: cir.get_member %0[0] {name = "b"} : !cir.ptr<!rec_U> -> !cir.ptr<!cir.bool>
// CIR: %[[#BASE:]] = cir.get_member %0[2] {name = "i"} : !cir.ptr<!rec_U> -> !cir.ptr<!s32i>
// CIR: cir.store %{{.+}}, %[[#BASE]] : !s32i, !cir.ptr<!s32i>
// CIR: %[[#BASE:]] = cir.get_member %0[2] {name = "i"} : !cir.ptr<!rec_U> -> !cir.ptr<!s32i>
// CIR: %[[#BASE:]] = cir.get_member %0[3] {name = "f"} : !cir.ptr<!rec_U> -> !cir.ptr<!cir.float>
// CIR: cir.store %{{.+}}, %[[#BASE]] : !cir.float, !cir.ptr<!cir.float>
// CIR: %[[#BASE:]] = cir.get_member %0[3] {name = "f"} : !cir.ptr<!rec_U> -> !cir.ptr<!cir.float>
// CIR: %[[#BASE:]] = cir.get_member %0[4] {name = "d"} : !cir.ptr<!rec_U> -> !cir.ptr<!cir.double>
// CIR: cir.store %{{.+}}, %[[#BASE]] : !cir.double, !cir.ptr<!cir.double>
// CIR: %[[#BASE:]] = cir.get_member %0[4] {name = "d"} : !cir.ptr<!rec_U> -> !cir.ptr<!cir.double>

// LLVM: define {{.*}}shouldGenerateUnionAccess
// LLVM: %[[BASE:.*]] = alloca %union.U
// LLVM: store %union.U %{{.*}}, ptr %[[BASE]]
// LLVM: store i8 1, ptr %[[BASE]]
// LLVM: store i32 1, ptr %[[BASE]]
// LLVM: store float 0x3FB99999A0000000, ptr %[[BASE]]
// LLVM: store double 1.000000e-01, ptr %[[BASE]]

// OGCG: define {{.*}}shouldGenerateUnionAccess
// OGCG: %[[BASE:.*]] = alloca %union.U
// OGCG: %[[DIVE:.*]] = getelementptr inbounds nuw %union.U, ptr %[[BASE]], i32 0, i32 0
// OGCG: store i64 %{{.*}}, ptr %[[DIVE]]
// OGCG: store i8 1, ptr %[[BASE]]
// OGCG: store i32 1, ptr %[[BASE]]
// OGCG: store float 0x3FB99999A0000000, ptr %[[BASE]]
// OGCG: store double 1.000000e-01, ptr %[[BASE]]