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
20 changes: 18 additions & 2 deletions clang/lib/CIR/CodeGen/CIRGenExprConst.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1717,17 +1717,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 (Ty->isRecordType()) {
if (const CXXConstructExpr *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())
if (CD->isTrivial() && CD->isDefaultConstructor()) {
const auto *cxxrd =
cast<CXXRecordDecl>(Ty->getAs<RecordType>()->getDecl());
// Some cases, such as member pointer members, can't be zero
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you clarify that this is an ABI thing? Data member pointers are zero initialized in the standard sense. It's just that most popular ABIs including Itanium have a non-zero bit representation of a null data member pointer

// initialized. These are "zero-initialized" in the language standard
// sense, but the target ABI may require that a literal value other
// than zero be used in the initializer to make clear that a pointer
// with the value zero is not what is intended. The classic codegen
// goes through emitNullConstant for those cases but generates a
// non-zero constant. We can't quite do that here because we need an
// attribute and not a value, but something like that can be
// implemented.
if (!CGM.getTypes().isZeroInitializable(cxxrd)) {
llvm_unreachable("NYI");
}
return cir::ZeroAttr::get(CGM.convertType(D.getType()));
}
}
}
}
InConstantContext = D.hasConstantInitialization();

Expand Down
19 changes: 19 additions & 0 deletions clang/lib/CIR/CodeGen/CIRRecordLayoutBuilder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,7 @@ void CIRRecordLowering::lower(bool nonVirtualBaseType) {
insertPadding();
members.pop_back();

calculateZeroInit();
fillOutputFields();
computeVolatileBitfields();
}
Expand Down Expand Up @@ -628,6 +629,24 @@ void CIRRecordLowering::accumulateFields() {
}
}

void CIRRecordLowering::calculateZeroInit() {
for (const MemberInfo &member : members) {
if (member.kind == MemberInfo::InfoKind::Field) {
if (!member.fieldDecl || isZeroInitializable(member.fieldDecl))
continue;
IsZeroInitializable = IsZeroInitializableAsBase = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think IsZeroInitializable and IsZeroInitializableAsBase are a bit odd. They're only really used within CIRGenTypes::computeRecordLayout. I wonder if we can remove these two member variables and replace them with a function returning a bool so there's no/less partially initialized state in this class?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They are accessed using a function that returns bool. You don't see that here because it was already present in the code and just always returned true because that's what these values were initialized to. They should probably have been initialized to false until this function is called, but I don't think there's any way to access the information outside of this CIRRecordLowering code until the CIRRecordLayout is created, which is done using these variables.

return;
} else if (member.kind == MemberInfo::InfoKind::Base ||
member.kind == MemberInfo::InfoKind::VBase) {
if (isZeroInitializable(member.cxxRecordDecl))
continue;
IsZeroInitializable = false;
if (member.kind == MemberInfo::InfoKind::Base)
IsZeroInitializableAsBase = false;
}
}
}

void CIRRecordLowering::determinePacked(bool NVBaseType) {
if (isPacked)
return;
Expand Down
22 changes: 22 additions & 0 deletions clang/test/CIR/CodeGen/nonzeroinit-struct.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fclangir -emit-cir %s -o %t.cir
// RUN: FileCheck --input-file=%t.cir %s

// This case is not yet implemented.
// XFAIL: *

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;

// The type is wrong on the last member here. It needs to be initialized to -1,
// but I don't know what that will look like.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe you can CHECK-NOT for zeroinitializer in the LLVM IR here instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When this test runs, it hits the llvm_unreachable("NYI"); and crashes. The check here is basically just a placeholder for what will be needed when the proper initialization is implemented. So, neither CHECK or CHECK-NOT will be reached.

// CHECK: cir.global {{.*}} @t = #cir.const_record<{#cir.int<0> : !s32i, #cir.fp<0.000000e+00> : !cir.double, #cir.int<-1> : !s64i}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is a little weird to model function pointers/member function pointers as a type other than a pointer. IMO, we need to have CIR model member function pointers in a way that IT handles the 'assigned to zero actually means -1'.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. I just didn't really know what that would be. When this gets implemented the check will be updated.

Copy link
Member

Choose a reason for hiding this comment

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

we need to have CIR model member function pointers in a way that IT handles the 'assigned to zero actually means -1'

Yea, the -1 probably only needs to show up during lowering

Loading