[CIR] Upstream limited support for nested structures #136331
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously when we checked to see if it was safe to create CIR for a structure type, we were conservatively saying no if any structure was in the process of being converted. That prevented handling nested structures, even when it would have actually been safe to handle them. Handling structures which recursively reference structures currently being processed requires deferring the handling of the recursively referenced structure, and that still isn't implemented after this change. This change adds the less conservative check that allows handling of non-recursive nested structures.
andykaylor
requested review from
bcardosolopes,
erichkeane,
lanza and
dkolsen-pgi
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Andy Kaylor (andykaylor) ChangesPreviously when we checked to see if it was safe to create CIR for a structure type, we were conservatively saying no if any structure was in the process of being converted. That prevented handling nested structures, even when it would have actually been safe to handle them. Handling structures which recursively reference structures currently being processed requires deferring the handling of the recursively referenced structure, and that still isn't implemented after this change. This change adds the less conservative check that allows handling of non-recursive nested structures. Full diff: https://github.com/llvm/llvm-project/pull/136331.diff 3 Files Affected:
diff --git a/clang/lib/CIR/CodeGen/CIRGenTypes.cpp b/clang/lib/CIR/CodeGen/CIRGenTypes.cpp
index a896b9dce14d6..7bd86cf0c7bcd 100644
--- a/clang/lib/CIR/CodeGen/CIRGenTypes.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenTypes.cpp
@@ -116,16 +116,93 @@ std::string CIRGenTypes::getRecordTypeName(const clang::RecordDecl *recordDecl,
return builder.getUniqueRecordName(std::string(typeName));
}
+/// Return true if the specified type is already completely laid out.
+bool CIRGenTypes::isRecordLayoutComplete(const Type *ty) const {
+ const auto it = recordDeclTypes.find(ty);
+ return it != recordDeclTypes.end() && it->second.isComplete();
+}
+
+// We have multiple forms of this function that call each other, so we need to
+// declare one in advance.
+static bool
+isSafeToConvert(QualType qt, CIRGenTypes &cgt,
+ llvm::SmallPtrSetImpl<const RecordDecl *> &alreadyChecked);
+
+/// Return true if it is safe to convert the specified record decl to CIR and
+/// lay it out, false if doing so would cause us to get into a recursive
+/// compilation mess.
+static bool
+isSafeToConvert(const RecordDecl *rd, CIRGenTypes &cgt,
+ llvm::SmallPtrSetImpl<const RecordDecl *> &alreadyChecked) {
+ // If we have already checked this type (maybe the same type is used by-value
+ // multiple times in multiple record fields, don't check again.
+ if (!alreadyChecked.insert(rd).second)
+ return true;
+
+ const Type *key = cgt.getASTContext().getTagDeclType(rd).getTypePtr();
+
+ // If this type is already laid out, converting it is a noop.
+ if (cgt.isRecordLayoutComplete(key))
+ return true;
+
+ // If this type is currently being laid out, we can't recursively compile it.
+ if (cgt.isRecordBeingLaidOut(key))
+ return false;
+
+ // If this type would require laying out bases that are currently being laid
+ // out, don't do it. This includes virtual base classes which get laid out
+ // when a class is translated, even though they aren't embedded by-value into
+ // the class.
+ if (const CXXRecordDecl *crd = dyn_cast<CXXRecordDecl>(rd)) {
+ assert(!cir::MissingFeatures::cxxSupport());
+ cgt.getCGModule().errorNYI(rd->getSourceRange(),
+ "isSafeToConvert: CXXRecordDecl");
+ return false;
+ }
+
+ // If this type would require laying out members that are currently being laid
+ // out, don't do it.
+ for (const FieldDecl *field : rd->fields())
+ if (!isSafeToConvert(field->getType(), cgt, alreadyChecked))
+ return false;
+
+ // If there are no problems, lets do it.
+ return true;
+}
+
+/// Return true if it is safe to convert this field type, which requires the
+/// record elements contained by-value to all be recursively safe to convert.
+static bool
+isSafeToConvert(QualType qt, CIRGenTypes &cgt,
+ llvm::SmallPtrSetImpl<const RecordDecl *> &alreadyChecked) {
+ // Strip off atomic type sugar.
+ if (const auto *at = qt->getAs<AtomicType>())
+ qt = at->getValueType();
+
+ // If this is a record, check it.
+ if (const auto *rt = qt->getAs<RecordType>())
+ return isSafeToConvert(rt->getDecl(), cgt, alreadyChecked);
+
+ // If this is an array, check the elements, which are embedded inline.
+ if (const auto *at = cgt.getASTContext().getAsArrayType(qt))
+ return isSafeToConvert(at->getElementType(), cgt, alreadyChecked);
+
+ // Otherwise, there is no concern about transforming this. We only care about
+ // things that are contained by-value in a record that can have another
+ // record as a member.
+ return true;
+}
+
// Return true if it is safe to convert the specified record decl to CIR and lay
// it out, false if doing so would cause us to get into a recursive compilation
// mess.
-static bool isSafeToConvert(const RecordDecl *RD, CIRGenTypes &CGT) {
+static bool isSafeToConvert(const RecordDecl *rd, CIRGenTypes &cgt) {
// If no records are being laid out, we can certainly do this one.
- if (CGT.noRecordsBeingLaidOut())
+ if (cgt.noRecordsBeingLaidOut())
return true;
- assert(!cir::MissingFeatures::recursiveRecordLayout());
- return false;
+ llvm::SmallPtrSet<const RecordDecl *, 16> alreadyChecked;
+ return isSafeToConvert(rd, cgt, alreadyChecked);
}
/// Lay out a tagged decl type like struct or union.
diff --git a/clang/lib/CIR/CodeGen/CIRGenTypes.h b/clang/lib/CIR/CodeGen/CIRGenTypes.h
index fc6e9cf621cb3..2bb78420700f8 100644
--- a/clang/lib/CIR/CodeGen/CIRGenTypes.h
+++ b/clang/lib/CIR/CodeGen/CIRGenTypes.h
@@ -89,7 +89,11 @@ class CIRGenTypes {
mlir::MLIRContext &getMLIRContext() const;
clang::ASTContext &getASTContext() const { return astContext; }
+ bool isRecordLayoutComplete(const clang::Type *ty) const;
bool noRecordsBeingLaidOut() const { return recordsBeingLaidOut.empty(); }
+ bool isRecordBeingLaidOut(const clang::Type *ty) const {
+ return recordsBeingLaidOut.count(ty);
+ }
const ABIInfo &getABIInfo() const { return theABIInfo; }
diff --git a/clang/test/CIR/CodeGen/struct.c b/clang/test/CIR/CodeGen/struct.c
index 55f316f27eeb1..3dc1655e15d2c 100644
--- a/clang/test/CIR/CodeGen/struct.c
+++ b/clang/test/CIR/CodeGen/struct.c
@@ -8,9 +8,13 @@
// For LLVM IR checks, the structs are defined before the variables, so these
// checks are at the top.
// LLVM: %struct.CompleteS = type { i32, i8 }
+// LLVM: %struct.OuterS = type { %struct.InnerS, i32 }
+// LLVM: %struct.InnerS = type { i32, i8 }
// LLVM: %struct.PackedS = type <{ i32, i8 }>
// LLVM: %struct.PackedAndPaddedS = type <{ i32, i8, i8 }>
// OGCG: %struct.CompleteS = type { i32, i8 }
+// OGCG: %struct.OuterS = type { %struct.InnerS, i32 }
+// OGCG: %struct.InnerS = type { i32, i8 }
// OGCG: %struct.PackedS = type <{ i32, i8 }>
// OGCG: %struct.PackedAndPaddedS = type <{ i32, i8, i8 }>
@@ -31,6 +35,23 @@ struct CompleteS {
// LLVM: @cs = dso_local global %struct.CompleteS zeroinitializer
// OGCG: @cs = global %struct.CompleteS zeroinitializer, align 4
+struct InnerS {
+ int a;
+ char b;
+};
+
+struct OuterS {
+ struct InnerS is;
+ int c;
+};
+
+struct OuterS os;
+
+// CIR: cir.global external @os = #cir.zero : !cir.record<struct
+// CIR-SAME: "OuterS" {!cir.record<struct "InnerS" {!s32i, !s8i}>, !s32i}>
+// LLVM: @os = dso_local global %struct.OuterS zeroinitializer
+// OGCG: @os = global %struct.OuterS zeroinitializer, align 4
+
#pragma pack(push)
#pragma pack(1)
|
|
@llvm/pr-subscribers-clangir Author: Andy Kaylor (andykaylor) ChangesPreviously when we checked to see if it was safe to create CIR for a structure type, we were conservatively saying no if any structure was in the process of being converted. That prevented handling nested structures, even when it would have actually been safe to handle them. Handling structures which recursively reference structures currently being processed requires deferring the handling of the recursively referenced structure, and that still isn't implemented after this change. This change adds the less conservative check that allows handling of non-recursive nested structures. Full diff: https://github.com/llvm/llvm-project/pull/136331.diff 3 Files Affected:
diff --git a/clang/lib/CIR/CodeGen/CIRGenTypes.cpp b/clang/lib/CIR/CodeGen/CIRGenTypes.cpp
index a896b9dce14d6..7bd86cf0c7bcd 100644
--- a/clang/lib/CIR/CodeGen/CIRGenTypes.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenTypes.cpp
@@ -116,16 +116,93 @@ std::string CIRGenTypes::getRecordTypeName(const clang::RecordDecl *recordDecl,
return builder.getUniqueRecordName(std::string(typeName));
}
+/// Return true if the specified type is already completely laid out.
+bool CIRGenTypes::isRecordLayoutComplete(const Type *ty) const {
+ const auto it = recordDeclTypes.find(ty);
+ return it != recordDeclTypes.end() && it->second.isComplete();
+}
+
+// We have multiple forms of this function that call each other, so we need to
+// declare one in advance.
+static bool
+isSafeToConvert(QualType qt, CIRGenTypes &cgt,
+ llvm::SmallPtrSetImpl<const RecordDecl *> &alreadyChecked);
+
+/// Return true if it is safe to convert the specified record decl to CIR and
+/// lay it out, false if doing so would cause us to get into a recursive
+/// compilation mess.
+static bool
+isSafeToConvert(const RecordDecl *rd, CIRGenTypes &cgt,
+ llvm::SmallPtrSetImpl<const RecordDecl *> &alreadyChecked) {
+ // If we have already checked this type (maybe the same type is used by-value
+ // multiple times in multiple record fields, don't check again.
+ if (!alreadyChecked.insert(rd).second)
+ return true;
+
+ const Type *key = cgt.getASTContext().getTagDeclType(rd).getTypePtr();
+
+ // If this type is already laid out, converting it is a noop.
+ if (cgt.isRecordLayoutComplete(key))
+ return true;
+
+ // If this type is currently being laid out, we can't recursively compile it.
+ if (cgt.isRecordBeingLaidOut(key))
+ return false;
+
+ // If this type would require laying out bases that are currently being laid
+ // out, don't do it. This includes virtual base classes which get laid out
+ // when a class is translated, even though they aren't embedded by-value into
+ // the class.
+ if (const CXXRecordDecl *crd = dyn_cast<CXXRecordDecl>(rd)) {
+ assert(!cir::MissingFeatures::cxxSupport());
+ cgt.getCGModule().errorNYI(rd->getSourceRange(),
+ "isSafeToConvert: CXXRecordDecl");
+ return false;
+ }
+
+ // If this type would require laying out members that are currently being laid
+ // out, don't do it.
+ for (const FieldDecl *field : rd->fields())
+ if (!isSafeToConvert(field->getType(), cgt, alreadyChecked))
+ return false;
+
+ // If there are no problems, lets do it.
+ return true;
+}
+
+/// Return true if it is safe to convert this field type, which requires the
+/// record elements contained by-value to all be recursively safe to convert.
+static bool
+isSafeToConvert(QualType qt, CIRGenTypes &cgt,
+ llvm::SmallPtrSetImpl<const RecordDecl *> &alreadyChecked) {
+ // Strip off atomic type sugar.
+ if (const auto *at = qt->getAs<AtomicType>())
+ qt = at->getValueType();
+
+ // If this is a record, check it.
+ if (const auto *rt = qt->getAs<RecordType>())
+ return isSafeToConvert(rt->getDecl(), cgt, alreadyChecked);
+
+ // If this is an array, check the elements, which are embedded inline.
+ if (const auto *at = cgt.getASTContext().getAsArrayType(qt))
+ return isSafeToConvert(at->getElementType(), cgt, alreadyChecked);
+
+ // Otherwise, there is no concern about transforming this. We only care about
+ // things that are contained by-value in a record that can have another
+ // record as a member.
+ return true;
+}
+
// Return true if it is safe to convert the specified record decl to CIR and lay
// it out, false if doing so would cause us to get into a recursive compilation
// mess.
-static bool isSafeToConvert(const RecordDecl *RD, CIRGenTypes &CGT) {
+static bool isSafeToConvert(const RecordDecl *rd, CIRGenTypes &cgt) {
// If no records are being laid out, we can certainly do this one.
- if (CGT.noRecordsBeingLaidOut())
+ if (cgt.noRecordsBeingLaidOut())
return true;
- assert(!cir::MissingFeatures::recursiveRecordLayout());
- return false;
+ llvm::SmallPtrSet<const RecordDecl *, 16> alreadyChecked;
+ return isSafeToConvert(rd, cgt, alreadyChecked);
}
/// Lay out a tagged decl type like struct or union.
diff --git a/clang/lib/CIR/CodeGen/CIRGenTypes.h b/clang/lib/CIR/CodeGen/CIRGenTypes.h
index fc6e9cf621cb3..2bb78420700f8 100644
--- a/clang/lib/CIR/CodeGen/CIRGenTypes.h
+++ b/clang/lib/CIR/CodeGen/CIRGenTypes.h
@@ -89,7 +89,11 @@ class CIRGenTypes {
mlir::MLIRContext &getMLIRContext() const;
clang::ASTContext &getASTContext() const { return astContext; }
+ bool isRecordLayoutComplete(const clang::Type *ty) const;
bool noRecordsBeingLaidOut() const { return recordsBeingLaidOut.empty(); }
+ bool isRecordBeingLaidOut(const clang::Type *ty) const {
+ return recordsBeingLaidOut.count(ty);
+ }
const ABIInfo &getABIInfo() const { return theABIInfo; }
diff --git a/clang/test/CIR/CodeGen/struct.c b/clang/test/CIR/CodeGen/struct.c
index 55f316f27eeb1..3dc1655e15d2c 100644
--- a/clang/test/CIR/CodeGen/struct.c
+++ b/clang/test/CIR/CodeGen/struct.c
@@ -8,9 +8,13 @@
// For LLVM IR checks, the structs are defined before the variables, so these
// checks are at the top.
// LLVM: %struct.CompleteS = type { i32, i8 }
+// LLVM: %struct.OuterS = type { %struct.InnerS, i32 }
+// LLVM: %struct.InnerS = type { i32, i8 }
// LLVM: %struct.PackedS = type <{ i32, i8 }>
// LLVM: %struct.PackedAndPaddedS = type <{ i32, i8, i8 }>
// OGCG: %struct.CompleteS = type { i32, i8 }
+// OGCG: %struct.OuterS = type { %struct.InnerS, i32 }
+// OGCG: %struct.InnerS = type { i32, i8 }
// OGCG: %struct.PackedS = type <{ i32, i8 }>
// OGCG: %struct.PackedAndPaddedS = type <{ i32, i8, i8 }>
@@ -31,6 +35,23 @@ struct CompleteS {
// LLVM: @cs = dso_local global %struct.CompleteS zeroinitializer
// OGCG: @cs = global %struct.CompleteS zeroinitializer, align 4
+struct InnerS {
+ int a;
+ char b;
+};
+
+struct OuterS {
+ struct InnerS is;
+ int c;
+};
+
+struct OuterS os;
+
+// CIR: cir.global external @os = #cir.zero : !cir.record<struct
+// CIR-SAME: "OuterS" {!cir.record<struct "InnerS" {!s32i, !s8i}>, !s32i}>
+// LLVM: @os = dso_local global %struct.OuterS zeroinitializer
+// OGCG: @os = global %struct.OuterS zeroinitializer, align 4
+
#pragma pack(push)
#pragma pack(1)
|
erichkeane
approved these changes
Apr 18, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously when we checked to see if it was safe to create CIR for a structure type, we were conservatively saying no if any structure was in the process of being converted. That prevented handling nested structures, even when it would have actually been safe to handle them. Handling structures which recursively reference structures currently being processed requires deferring the handling of the recursively referenced structure, and that still isn't implemented after this change.
This change adds the less conservative check that allows handling of non-recursive nested structures.