-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[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
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 |
---|---|---|
|
@@ -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" | ||
|
@@ -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(); | ||
// 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())); | ||
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. OGCG calls 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. Yeah, that seems reasonable. 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. As I tried to implement this suggestion, it quickly came to my attention that 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. Second complication, the clang AST says that 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. Thanks for adding the |
||
} | ||
} | ||
} | ||
} | ||
inConstantContext = d.hasConstantInitialization(); | ||
|
||
|
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 |
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]] |
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.
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?
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.
Classic codegen does assume that
cd
is non-null here. I'm pretty sure that if thedyn_cast_or_null
gives us aCXXConstructorExpr
we can count on if having aCXXConstructorDecl
.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:The AST says this:
And the incubator generates this CIR:
Is your question whether we should do something here to explicitly show that the copy constructor was elided?
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.
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.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.
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 inemitCXXConstructExpr
(which has not been upstreamed yet). I guess thecd->isTrivial() && cd->isDefaultConstructor()
check prevents us from doing anything with it here.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.
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).
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.
@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.