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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
483efe5
[CIR] Unblock simple C++ structure support
andykaylor Apr 29, 2025
a160ec9
Add check for base class and test for unions
andykaylor May 5, 2025
84f01c9
Add error and test for non-zero-init structs
andykaylor May 6, 2025
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
View file
Open in desktop
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
13 changes: 9 additions & 4 deletions clang/lib/CIR/CodeGen/CIRGenExprConstant.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,10 +365,15 @@ mlir::Attribute ConstantEmitter::tryEmitPrivateForVarInit(const VarDecl &d) {
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 (const CXXConstructExpr *e =
dyn_cast_or_null<CXXConstructExpr>(d.getInit())) {
const CXXConstructorDecl *cd = e->getConstructor();
Copy link
Collaborator

@erichkeane erichkeane May 5, 2025

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

@andykaylor andykaylor May 5, 2025

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

@erichkeane erichkeane May 5, 2025

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

@andykaylor andykaylor May 5, 2025

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

@bcardosolopes bcardosolopes May 6, 2025

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

@andykaylor andykaylor May 6, 2025

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())
return cir::ZeroAttr::get(cgm.convertType(d.getType()));
Copy link
Contributor

@mmha mmha May 3, 2025

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

@andykaylor andykaylor May 5, 2025

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

@andykaylor andykaylor May 5, 2025

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

@andykaylor andykaylor May 5, 2025

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

@mmha mmha May 6, 2025

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
25 changes: 18 additions & 7 deletions clang/lib/CIR/CodeGen/CIRGenRecordLayoutBuilder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,18 +177,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 (auto const *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 Down Expand Up @@ -295,7 +303,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 Down
7 changes: 5 additions & 2 deletions clang/lib/CIR/CodeGen/CIRGenTypes.cpp
View file
Open in desktop
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
37 changes: 37 additions & 0 deletions clang/test/CIR/CodeGen/struct.cpp
View file
Open in desktop
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]]
Loading