[CIR] Upstream minimal support for structure types #135105
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
andykaylor
requested review from
xlauko,
bcardosolopes,
erichkeane,
lanza,
AmrDeveloper and
dkolsen-pgi
llvmbot
added
clang
|
@llvm/pr-subscribers-clangir @llvm/pr-subscribers-clang Author: Andy Kaylor (andykaylor) ChangesThis change adds minimal support for structure types. To keep the initial change small, only incomplete declarations are being supported in this patch. More complete support will follow. Patch is 25.95 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/135105.diff 11 Files Affected:
diff --git a/clang/include/clang/CIR/Dialect/IR/CIRTypes.h b/clang/include/clang/CIR/Dialect/IR/CIRTypes.h
index 7b0fcbc7cc98f..d2c407b2fd686 100644
--- a/clang/include/clang/CIR/Dialect/IR/CIRTypes.h
+++ b/clang/include/clang/CIR/Dialect/IR/CIRTypes.h
@@ -20,6 +20,10 @@
namespace cir {
+namespace detail {
+struct StructTypeStorage;
+} // namespace detail
+
bool isAnyFloatingPointType(mlir::Type t);
bool isFPOrFPVectorTy(mlir::Type);
diff --git a/clang/include/clang/CIR/Dialect/IR/CIRTypes.td b/clang/include/clang/CIR/Dialect/IR/CIRTypes.td
index e285c0f28f113..bcdaf54bbd84f 100644
--- a/clang/include/clang/CIR/Dialect/IR/CIRTypes.td
+++ b/clang/include/clang/CIR/Dialect/IR/CIRTypes.td
@@ -400,13 +400,126 @@ def VoidPtr : Type<
"cir::VoidType::get($_builder.getContext()))"> {
}
+//===----------------------------------------------------------------------===//
+// StructType
+//
+// The base type for all RecordDecls.
+//===----------------------------------------------------------------------===//
+
+def CIR_StructType : CIR_Type<"Struct", "struct",
+ [
+ DeclareTypeInterfaceMethods<DataLayoutTypeInterface>,
+ MutableType,
+ ]> {
+ let summary = "CIR struct type";
+ let description = [{
+ Each unique clang::RecordDecl is mapped to a `cir.struct` and any object in
+ C/C++ that has a struct type will have a `cir.struct` in CIR.
+
+ There are three possible formats for this type:
+
+ - Identified and complete structs: unique name and a known body.
+ - Identified and incomplete structs: unique name and unknown body.
+ - Anonymous structs: no name and a known body.
+
+ Identified structs are uniqued by their name, and anonymous structs are
+ uniqued by their body. This means that two anonymous structs with the same
+ body will be the same type, and two identified structs with the same name
+ will be the same type. Attempting to build a struct with an existing name,
+ but a different body will result in an error.
+
+ A few examples:
+
+ ```mlir
+ !complete = !cir.struct<struct "complete" {!cir.int<u, 8>}>
+ !incomplete = !cir.struct<struct "incomplete" incomplete>
+ !anonymous = !cir.struct<struct {!cir.int<u, 8>}>
+ ```
+
+ Incomplete structs are mutable, meaning they can be later completed with a
+ body automatically updating in place every type in the code that uses the
+ incomplete struct. Mutability allows for recursive types to be represented,
+ meaning the struct can have members that refer to itself. This is useful for
+ representing recursive records and is implemented through a special syntax.
+ In the example below, the `Node` struct has a member that is a pointer to a
+ `Node` struct:
+
+ ```mlir
+ !struct = !cir.struct<struct "Node" {!cir.ptr<!cir.struct<struct
+ "Node">>}>
+ ```
+ }];
+
+ let parameters = (ins
+ OptionalArrayRefParameter<"mlir::Type">:$members,
+ OptionalParameter<"mlir::StringAttr">:$name,
+ "bool":$incomplete,
+ "bool":$packed,
+ "bool":$padded,
+ "StructType::RecordKind":$kind
+ );
+
+ // StorageClass is defined in C++ for mutability.
+ let storageClass = "StructTypeStorage";
+ let genStorageClass = 0;
+
+ let skipDefaultBuilders = 1;
+ let genVerifyDecl = 1;
+
+ let builders = [
+ // Create an identified and incomplete struct type.
+ TypeBuilder<(ins
+ "mlir::StringAttr":$name,
+ "RecordKind":$kind
+ ), [{
+ return $_get($_ctxt, /*members=*/llvm::ArrayRef<Type>{}, name,
+ /*incomplete=*/true, /*packed=*/false,
+ /*padded=*/false, kind);
+ }]>];
+
+ let extraClassDeclaration = [{
+ using Base::verifyInvariants;
+
+ enum RecordKind : uint32_t { Class, Union, Struct };
+
+ bool isClass() const { return getKind() == RecordKind::Class; };
+ bool isStruct() const { return getKind() == RecordKind::Struct; };
+ bool isUnion() const { return getKind() == RecordKind::Union; };
+ bool isComplete() const { return !isIncomplete(); };
+ bool isIncomplete() const;
+
+ size_t getNumElements() const { return getMembers().size(); };
+ std::string getKindAsStr() {
+ switch (getKind()) {
+ case RecordKind::Class:
+ return "class";
+ case RecordKind::Union:
+ return "union";
+ case RecordKind::Struct:
+ return "struct";
+ }
+ llvm_unreachable("Invalid value for StructType::getKind()");
+ }
+ std::string getPrefixedName() {
+ return getKindAsStr() + "." + getName().getValue().str();
+ }
+ }];
+
+ let hasCustomAssemblyFormat = 1;
+}
+
+// Note CIRStructType is used instead of CIR_StructType
+// because of tablegen conflicts.
+def CIRStructType : Type<
+ CPred<"::mlir::isa<::cir::StructType>($_self)">, "CIR struct type">;
+
//===----------------------------------------------------------------------===//
// Global type constraints
//===----------------------------------------------------------------------===//
def CIR_AnyType : AnyTypeOf<[
CIR_VoidType, CIR_BoolType, CIR_ArrayType, CIR_IntType, CIR_AnyFloat,
- CIR_PointerType, CIR_FuncType
+ CIR_PointerType, CIR_FuncType, CIR_StructType
]>;
#endif // MLIR_CIR_DIALECT_CIR_TYPES
diff --git a/clang/include/clang/CIR/Dialect/IR/CIRTypesDetails.h b/clang/include/clang/CIR/Dialect/IR/CIRTypesDetails.h
new file mode 100644
index 0000000000000..91bfb814a609c
--- /dev/null
+++ b/clang/include/clang/CIR/Dialect/IR/CIRTypesDetails.h
@@ -0,0 +1,116 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file contains implementation details, such as storage structures, of
+// CIR dialect types.
+//
+//===----------------------------------------------------------------------===//
+#ifndef CIR_DIALECT_IR_CIRTYPESDETAILS_H
+#define CIR_DIALECT_IR_CIRTYPESDETAILS_H
+
+#include "mlir/IR/BuiltinAttributes.h"
+#include "mlir/Support/LogicalResult.h"
+#include "clang/CIR/Dialect/IR/CIRTypes.h"
+#include "llvm/ADT/Hashing.h"
+
+namespace cir {
+namespace detail {
+
+//===----------------------------------------------------------------------===//
+// CIR StructTypeStorage
+//===----------------------------------------------------------------------===//
+
+/// Type storage for CIR record types.
+struct StructTypeStorage : public mlir::TypeStorage {
+ struct KeyTy {
+ llvm::ArrayRef<mlir::Type> members;
+ mlir::StringAttr name;
+ bool incomplete;
+ bool packed;
+ bool padded;
+ StructType::RecordKind kind;
+
+ KeyTy(llvm::ArrayRef<mlir::Type> members, mlir::StringAttr name,
+ bool incomplete, bool packed, bool padded,
+ StructType::RecordKind kind)
+ : members(members), name(name), incomplete(incomplete), packed(packed),
+ padded(padded), kind(kind) {}
+ };
+
+ llvm::ArrayRef<mlir::Type> members;
+ mlir::StringAttr name;
+ bool incomplete;
+ bool packed;
+ bool padded;
+ StructType::RecordKind kind;
+
+ StructTypeStorage(llvm::ArrayRef<mlir::Type> members, mlir::StringAttr name,
+ bool incomplete, bool packed, bool padded,
+ StructType::RecordKind kind)
+ : members(members), name(name), incomplete(incomplete), packed(packed),
+ padded(padded), kind(kind) {}
+
+ KeyTy getAsKey() const {
+ return KeyTy(members, name, incomplete, packed, padded, kind);
+ }
+
+ bool operator==(const KeyTy &key) const {
+ if (name)
+ return (name == key.name) && (kind == key.kind);
+ return (members == key.members) && (name == key.name) &&
+ (incomplete == key.incomplete) && (packed == key.packed) &&
+ (padded == key.padded) && (kind == key.kind);
+ }
+
+ static llvm::hash_code hashKey(const KeyTy &key) {
+ if (key.name)
+ return llvm::hash_combine(key.name, key.kind);
+ return llvm::hash_combine(key.members, key.incomplete, key.packed,
+ key.padded, key.kind);
+ }
+
+ static StructTypeStorage *construct(mlir::TypeStorageAllocator &allocator,
+ const KeyTy &key) {
+ return new (allocator.allocate<StructTypeStorage>())
+ StructTypeStorage(allocator.copyInto(key.members), key.name,
+ key.incomplete, key.packed, key.padded, key.kind);
+ }
+
+ /// Mutates the members and attributes an identified struct.
+ ///
+ /// Once a record is mutated, it is marked as complete, preventing further
+ /// mutations. Anonymous structs are always complete and cannot be mutated.
+ /// This method does not fail if a mutation of a complete struct does not
+ /// change the struct.
+ llvm::LogicalResult mutate(mlir::TypeStorageAllocator &allocator,
+ llvm::ArrayRef<mlir::Type> members, bool packed,
+ bool padded) {
+ // Anonymous structs cannot mutate.
+ if (!name)
+ return llvm::failure();
+
+ // Mutation of complete structs are allowed if they change nothing.
+ if (!incomplete)
+ return mlir::success((this->members == members) &&
+ (this->packed == packed) &&
+ (this->padded == padded));
+
+ // Mutate incomplete struct.
+ this->members = allocator.copyInto(members);
+ this->packed = packed;
+ this->padded = padded;
+
+ incomplete = false;
+ return llvm::success();
+ }
+};
+
+} // namespace detail
+} // namespace cir
+
+#endif // CIR_DIALECT_IR_CIRTYPESDETAILS_H
diff --git a/clang/include/clang/CIR/MissingFeatures.h b/clang/include/clang/CIR/MissingFeatures.h
index 3188429ea3b1b..bc52988dfc750 100644
--- a/clang/include/clang/CIR/MissingFeatures.h
+++ b/clang/include/clang/CIR/MissingFeatures.h
@@ -82,6 +82,9 @@ struct MissingFeatures {
static bool mayHaveIntegerOverflow() { return false; }
static bool shouldReverseUnaryCondOnBoolExpr() { return false; }
+ // StructType
+ static bool structTypeLayoutInfo() { return false; }
+
// Misc
static bool cxxABI() { return false; }
static bool tryEmitAsConstant() { return false; }
diff --git a/clang/lib/CIR/CodeGen/CIRGenBuilder.h b/clang/lib/CIR/CodeGen/CIRGenBuilder.h
index 61a747254b3d0..0a9ef5fcd5471 100644
--- a/clang/lib/CIR/CodeGen/CIRGenBuilder.h
+++ b/clang/lib/CIR/CodeGen/CIRGenBuilder.h
@@ -20,11 +20,24 @@ namespace clang::CIRGen {
class CIRGenBuilderTy : public cir::CIRBaseBuilderTy {
const CIRGenTypeCache &typeCache;
+ llvm::StringMap<unsigned> recordNames;
public:
CIRGenBuilderTy(mlir::MLIRContext &mlirContext, const CIRGenTypeCache &tc)
: CIRBaseBuilderTy(mlirContext), typeCache(tc) {}
+ std::string getUniqueAnonRecordName() { return getUniqueRecordName("anon"); }
+
+ std::string getUniqueRecordName(const std::string &baseName) {
+ auto it = recordNames.find(baseName);
+ if (it == recordNames.end()) {
+ recordNames[baseName] = 0;
+ return baseName;
+ }
+
+ return baseName + "." + std::to_string(recordNames[baseName]++);
+ }
+
cir::LongDoubleType getLongDoubleTy(const llvm::fltSemantics &format) const {
if (&format == &llvm::APFloat::IEEEdouble())
return cir::LongDoubleType::get(getContext(), typeCache.DoubleTy);
@@ -37,6 +50,32 @@ class CIRGenBuilderTy : public cir::CIRBaseBuilderTy {
llvm_unreachable("Unsupported format for long double");
}
+ /// Get a CIR record kind from a AST declaration tag.
+ cir::StructType::RecordKind getRecordKind(const clang::TagTypeKind kind) {
+ switch (kind) {
+ case clang::TagTypeKind::Struct:
+ return cir::StructType::Struct;
+ case clang::TagTypeKind::Union:
+ return cir::StructType::Union;
+ case clang::TagTypeKind::Class:
+ return cir::StructType::Class;
+ case clang::TagTypeKind::Interface:
+ llvm_unreachable("interface records are NYI");
+ case clang::TagTypeKind::Enum:
+ llvm_unreachable("enum records are NYI");
+ }
+ }
+
+ /// Get an incomplete CIR struct type.
+ cir::StructType getIncompleteStructTy(llvm::StringRef name,
+ const clang::RecordDecl *rd) {
+ const mlir::StringAttr nameAttr = getStringAttr(name);
+ cir::StructType::RecordKind kind = cir::StructType::RecordKind::Struct;
+ if (rd)
+ kind = getRecordKind(rd->getTagKind());
+ return getType<cir::StructType>(nameAttr, kind);
+ }
+
bool isSized(mlir::Type ty) {
if (mlir::isa<cir::PointerType, cir::ArrayType, cir::BoolType,
cir::IntType>(ty))
diff --git a/clang/lib/CIR/CodeGen/CIRGenModule.cpp b/clang/lib/CIR/CodeGen/CIRGenModule.cpp
index d2259a9c41d22..4bd186f68f8c9 100644
--- a/clang/lib/CIR/CodeGen/CIRGenModule.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenModule.cpp
@@ -490,6 +490,10 @@ void CIRGenModule::emitTopLevelDecl(Decl *decl) {
case Decl::OpenACCDeclare:
emitGlobalOpenACCDecl(cast<OpenACCDeclareDecl>(decl));
break;
+
+ case Decl::Record:
+ assert(!cir::MissingFeatures::generateDebugInfo());
+ break;
}
}
diff --git a/clang/lib/CIR/CodeGen/CIRGenTypes.cpp b/clang/lib/CIR/CodeGen/CIRGenTypes.cpp
index 68aee63c2b22d..8644b9a62320a 100644
--- a/clang/lib/CIR/CodeGen/CIRGenTypes.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenTypes.cpp
@@ -86,10 +86,80 @@ mlir::Type CIRGenTypes::convertFunctionTypeInternal(QualType qft) {
return cir::FuncType::get(SmallVector<mlir::Type, 1>{}, cgm.VoidTy);
}
+// This is CIR's version of CodeGenTypes::addRecordTypeName. It isn't shareable
+// because CIR has different uniquing requirements.
+std::string CIRGenTypes::getRecordTypeName(const clang::RecordDecl *recordDecl,
+ StringRef suffix) {
+ llvm::SmallString<256> typeName;
+ llvm::raw_svector_ostream outStream(typeName);
+
+ PrintingPolicy policy = recordDecl->getASTContext().getPrintingPolicy();
+ policy.SuppressInlineNamespace = false;
+
+ if (recordDecl->getIdentifier()) {
+ if (recordDecl->getDeclContext())
+ recordDecl->printQualifiedName(outStream, policy);
+ else
+ recordDecl->printName(outStream, policy);
+
+ // Ensure each template specialization has a unique name.
+ if (auto *templateSpecialization =
+ llvm::dyn_cast<ClassTemplateSpecializationDecl>(recordDecl)) {
+ outStream << '<';
+ const ArrayRef<TemplateArgument> args =
+ templateSpecialization->getTemplateArgs().asArray();
+ const auto printer = [&policy, &outStream](const TemplateArgument &arg) {
+ /// Print this template argument to the given output stream.
+ arg.print(policy, outStream, /*IncludeType=*/true);
+ };
+ llvm::interleaveComma(args, outStream, printer);
+ outStream << '>';
+ }
+ } else if (auto *typedefNameDecl = recordDecl->getTypedefNameForAnonDecl()) {
+ if (typedefNameDecl->getDeclContext())
+ typedefNameDecl->printQualifiedName(outStream, policy);
+ else
+ typedefNameDecl->printName(outStream);
+ } else {
+ outStream << builder.getUniqueAnonRecordName();
+ }
+
+ if (!suffix.empty())
+ outStream << suffix;
+
+ return builder.getUniqueRecordName(std::string(typeName));
+}
+
+/// Lay out a tagged decl type like struct or union.
+mlir::Type CIRGenTypes::convertRecordDeclType(const clang::RecordDecl *rd) {
+ // TagDecl's are not necessarily unique, instead use the (clang) type
+ // connected to the decl.
+ const Type *key = astContext.getTagDeclType(rd).getTypePtr();
+ cir::StructType entry = recordDeclTypes[key];
+
+ // Handle forward decl / incomplete types.
+ if (!entry) {
+ auto name = getRecordTypeName(rd, "");
+ entry = builder.getIncompleteStructTy(name, rd);
+ recordDeclTypes[key] = entry;
+ }
+
+ rd = rd->getDefinition();
+ if (!rd || !rd->isCompleteDefinition() || entry.isComplete())
+ return entry;
+
+ cgm.errorNYI(rd->getSourceRange(), "Complete record type");
+ return entry;
+}
+
mlir::Type CIRGenTypes::convertType(QualType type) {
type = astContext.getCanonicalType(type);
const Type *ty = type.getTypePtr();
+ // Process record types before the type cache lookup.
+ if (const auto *recordType = dyn_cast<RecordType>(type))
+ return convertRecordDeclType(recordType->getDecl());
+
// Has the type already been processed?
TypeCacheTy::iterator tci = typeCache.find(ty);
if (tci != typeCache.end())
@@ -100,9 +170,11 @@ mlir::Type CIRGenTypes::convertType(QualType type) {
mlir::Type resultType = nullptr;
switch (ty->getTypeClass()) {
+ case Type::Record:
+ llvm_unreachable("Should have been handled above");
+
case Type::Builtin: {
switch (cast<BuiltinType>(ty)->getKind()) {
-
// void
case BuiltinType::Void:
resultType = cgm.VoidTy;
@@ -236,7 +308,8 @@ mlir::Type CIRGenTypes::convertType(QualType type) {
}
default:
- cgm.errorNYI(SourceLocation(), "processing of type", type);
+ cgm.errorNYI(SourceLocation(), "processing of type",
+ type->getTypeClassName());
resultType = cgm.SInt32Ty;
break;
}
diff --git a/clang/lib/CIR/CodeGen/CIRGenTypes.h b/clang/lib/CIR/CodeGen/CIRGenTypes.h
index 4021206e979e1..a3b7eae6d0767 100644
--- a/clang/lib/CIR/CodeGen/CIRGenTypes.h
+++ b/clang/lib/CIR/CodeGen/CIRGenTypes.h
@@ -43,6 +43,9 @@ class CIRGenTypes {
clang::ASTContext &astContext;
CIRGenBuilderTy &builder;
+ /// Contains the CIR type for any converted RecordDecl
+ llvm::DenseMap<const clang::Type *, cir::StructType> recordDeclTypes;
+
/// Heper for convertType.
mlir::Type convertFunctionTypeInternal(clang::QualType ft);
@@ -65,6 +68,11 @@ class CIRGenTypes {
/// Convert a Clang type into a mlir::Type.
mlir::Type convertType(clang::QualType type);
+ mlir::Type convertRecordDeclType(const clang::RecordDecl *recordDecl);
+
+ std::string getRecordTypeName(const clang::RecordDecl *,
+ llvm::StringRef suffix);
+
/// Convert type T into an mlir::Type. This differs from convertType in that
/// it is used to convert to the memory representation for a type. For
/// example, the scalar representation for bool is i1, but the memory
diff --git a/clang/lib/CIR/Dialect/IR/CIRTypes.cpp b/clang/lib/CIR/Dialect/IR/CIRTypes.cpp
index 356f7f6244db8..4ff7e1fdbb7c8 100644
--- a/clang/lib/CIR/Dialect/IR/CIRTypes.cpp
+++ b/clang/lib/CIR/Dialect/IR/CIRTypes.cpp
@@ -14,6 +14,7 @@
#include "mlir/IR/DialectImplementation.h"
#include "clang/CIR/Dialect/IR/CIRDialect.h"
+#include "clang/CIR/Dialect/IR/CIRTypesDetails.h"
#include "clang/CIR/MissingFeatures.h"
#include "llvm/ADT/TypeSwitch.h"
@@ -53,9 +54,13 @@ Type CIRDialect::parseType(DialectAsmParser &parser) const {
if (parseResult.has_value())
return genType;
- // TODO(CIR) Attempt to parse as a raw C++ type.
- parser.emitError(typeLoc) << "unknown CIR type: " << mnemonic;
- return Type();
+ // Type is not tablegen'd: try to parse as a raw C++ type.
+ return StringSwitch<function_ref<Type()>>(mnemonic)
+ .Case("struct", [&] { return StructType::parse(parser); })
+ .Default([&] {
+ parser.emitError(typeLoc) << "unknown CIR type: " << mnemonic;
+ return Type();
+ })();
}
void CIRDialect::printType(Type type, DialectAsmPrinter &os) const {
@@ -67,6 +72,171 @@ void CIRDialect::printType(Type type, DialectAsmPrinter &os) const {
llvm::report_fatal_error("printer is missing a handler for this type");
}
+//===----------------------------------------------------------------------===//
+// StructType Definitions
+//===----------------------------------------------------------------------===//
+
+Type StructType::parse(mlir::AsmParser &parser) {
+ FailureOr<AsmParser::CyclicParseReset> cyclicParseGuard;
+ const auto loc = parser.getCurrentLocation();
+ const auto eLoc = parser.getEncodedSourceLoc(loc);
+ RecordKind kind;
+ auto *context = parser.getContext();
+
+ if (parser.parseLess())
+ return {};
+
+ // TODO(cir): in the future we should probably separate types for different
+ // source language declarations such as cir.class, cir.union, and cir.struct
+ if (parser.parseOptionalKeyword("struct").succeeded())
+ kind...
[truncated]
|
erichkeane
reviewed
Apr 10, 2025
clang/test/CIR/IR/struct.cir
Outdated
| @@ -0,0 +1,9 @@ | |||
| // RUN: cir-opt %s | FileCheck %s | |||
|
|
|||
| !s32i = !cir.int<s, 32> | |||
There was a problem hiding this comment.
what is this doing? I dont really see it in the check or definition lines.
There was a problem hiding this comment.
Oops! That was a copy-paste error.
clang/test/CIR/CodeGen/struct.c
Outdated
| // Declaration with an incomplete struct type. | ||
| struct U *p; | ||
|
|
||
| // CIR: cir.global external @p = #cir.ptr<null> : !cir.ptr<!cir.struct<struct "U" incomplete>> |
There was a problem hiding this comment.
Could we/can we do a test not in the global scope, that is, a pointer to incomplete struct at function scope? Or as function parameters?
Perhaps a 'union' test of both of these as well?
| - Anonymous structs: no name and a known body. | ||
|
|
||
| Identified structs are uniqued by their name, and anonymous structs are | ||
| uniqued by their body. This means that two anonymous structs with the same |
There was a problem hiding this comment.
This here (2 anonymous structs with the same body are the same type) is somewhat novel... it sort of models the C 'compatible types', but of course we don't have that in C++.
There was a problem hiding this comment.
This may just be stating an axiomatic fact. In practice, we seem to be giving anonymous structures a name.
https://godbolt.org/z/jfc7bT4qd
But the name isn't required by CIR, so the definition is telling you what happens if you don't have it.
| `Node` struct: | ||
|
|
||
| ```mlir | ||
| !struct = !cir.struct<struct "Node" {!cir.ptr<!cir.struct<struct |
There was a problem hiding this comment.
Curious why it is !cir.struct<struct "Node". Why the duplication of struct` there?
Presumably because it might be class or union in teh second place?
There was a problem hiding this comment.
Yes. Again, this would probably be more natural if it were !cir.record<struct ...>.
There was a problem hiding this comment.
I think I'm VERY in favor of a change to cir.record. It is more accurate and less confusing. I realize we want to model a little more closely llvm's struct type, but I don't think the confusion is worth it.
There was a problem hiding this comment.
@bcardosolopes Any objections to renaming cir.struct to cir.record?
There was a problem hiding this comment.
No strong preference on my side, but it would be nice to have that change in the incubator as well!
There was a problem hiding this comment.
I'm happy to change it both places.
| ]> { | ||
| let summary = "CIR struct type"; | ||
| let description = [{ | ||
| Each unique clang::RecordDecl is mapped to a `cir.struct` and any object in |
There was a problem hiding this comment.
So this is actually a bit confusing/awkward. A RecordDecl can represent a 'union' as well as a 'class' or 'struct' type.
So I question the name of it here.
There was a problem hiding this comment.
Yeah. It's not a great. The same thing sort of happens in LLVM IR, but there it makes a little bit more sense because unions, while represented as a structure, only contain the elements necessary to account for the memory, but that's also bad because to access any other member you end up treating it as something else.
I just looked at the incubator handling of unions, and it does represent each member of the union as a unique member of the cir.struct. That is consistent with CIR's goal of representing the logical intent of the code. I understand why it's being done the way it is, but there is definitely potential for confusion.
What would you think of renaming this to cir.record? That would introduce a clear distinction between this and llvm.struct in the LLVM dialect, which directly corresponds to llvm::StructType.
There was a problem hiding this comment.
Definitely in favor of the rename, and fixes most of my concerns on this patch TBH.
| policy.SuppressInlineNamespace = false; | ||
|
|
||
| if (recordDecl->getIdentifier()) { | ||
| if (recordDecl->getDeclContext()) |
There was a problem hiding this comment.
Er... what is going on here? A record decl is ALWAYS going to have a DeclContext.
Is this some sort of attempt to differentiate global-namespace records?
There was a problem hiding this comment.
The only thing I can figure is that someone saw that printQualifiedName dereferences the DeclContext pointer and thought a safeguard was needed. I can remove this.
|
|
||
| // Ensure each template specialization has a unique name. | ||
| if (auto *templateSpecialization = | ||
| llvm::dyn_cast<ClassTemplateSpecializationDecl>(recordDecl)) { |
There was a problem hiding this comment.
THIS block feels pretty error-prone. I find myself wondering why we can't just reuse the code that specializations use to print the RecordType entirely (like for diagnostics), instead of all this.
There was a problem hiding this comment.
Why does this feel error prone? I'm not sure which code you're suggesting we could use instead.
There was a problem hiding this comment.
The template-args can print... awkwardly sometimes and not consistently depending on placeholder syntax and sugaring. The code we already have to do such printing is already pretty onerous. See ConvertTypeToDiagnosticString for what we typically use for diagnostics (which is a little more convoluted, but interesting all the same).
Clang TypePrinter(LangOptions()).print also does similar work and does a pretty solid job of it, and has some options to make it easier to not be weird. So we might try that?
| templateSpecialization->getTemplateArgs().asArray(); | ||
| const auto printer = [&policy, &outStream](const TemplateArgument &arg) { | ||
| /// Print this template argument to the given output stream. | ||
| arg.print(policy, outStream, /*IncludeType=*/true); |
There was a problem hiding this comment.
This is likely to get weird with variadic templates or pack-expansions I think.
| outStream << '>'; | ||
| } | ||
| } else if (auto *typedefNameDecl = recordDecl->getTypedefNameForAnonDecl()) { | ||
| if (typedefNameDecl->getDeclContext()) |
There was a problem hiding this comment.
Again, a null declcontext just doesn't happen.
|
I'm going to make the change to rename |
erichkeane
reviewed
Apr 11, 2025
|
|
||
| llvm::ArrayRef<mlir::Type> members; | ||
| mlir::StringAttr name; | ||
| bool incomplete; |
There was a problem hiding this comment.
Would still, I think, like hte polarity here reversed.
|
|
||
| if (recordDecl->getIdentifier()) | ||
| astContext.getRecordType(recordDecl).print(outStream, policy); | ||
| else if (auto *typedefNameDecl = recordDecl->getTypedefNameForAnonDecl()) |
There was a problem hiding this comment.
Hmm... Note that `typedefs' (actually 'usings') can actually be templated (see https://clang.llvm.org/doxygen/classclang_1_1TypeAliasDecl.html).
Can we have a test that makes sure we print template-args in that case?
There was a problem hiding this comment.
Not until we support typedefs. This should probably by "NYI" because we can't really get here yet.
There was a problem hiding this comment.
NYI would make me happy.
erichkeane
approved these changes
Apr 11, 2025
sorry for the delay, busy week! |
bcardosolopes
approved these changes
Apr 11, 2025
This change adds minimal support for structure types. To keep the initial change small, only incomplete declarations are being supported in this patch. More complete support will follow.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
xlauko
reviewed
Apr 14, 2025
Comment on lines
81
to
82
| const auto loc = parser.getCurrentLocation(); | ||
| const auto eLoc = parser.getEncodedSourceLoc(loc); |
There was a problem hiding this comment.
Suggested change
| const auto loc = parser.getCurrentLocation(); | |
| const auto eLoc = parser.getEncodedSourceLoc(loc); | |
| const llvm::SMLoc loc = parser.getCurrentLocation(); | |
| const Location eLoc = parser.getEncodedSourceLoc(loc); |
| const auto loc = parser.getCurrentLocation(); | ||
| const auto eLoc = parser.getEncodedSourceLoc(loc); | ||
| RecordKind kind; | ||
| auto *context = parser.getContext(); |
There was a problem hiding this comment.
Suggested change
| auto *context = parser.getContext(); | |
| MLIRContext *context = parser.getContext(); |
|
|
||
| // Is a self reference: ensure referenced type was parsed. | ||
| if (name && parser.parseOptionalGreater().succeeded()) { | ||
| auto type = getChecked(eLoc, context, name, kind); |
There was a problem hiding this comment.
Suggested change
| auto type = getChecked(eLoc, context, name, kind); | |
| Type type = getChecked(eLoc, context, name, kind); |
|
|
||
| // Is a named record definition: ensure name has not been parsed yet. | ||
| if (name) { | ||
| auto type = getChecked(eLoc, context, name, kind); |
There was a problem hiding this comment.
Suggested change
| auto type = getChecked(eLoc, context, name, kind); | |
| Type type = getChecked(eLoc, context, name, kind); |
|
|
||
| // Parse record members or lack thereof. | ||
| bool incomplete = true; | ||
| llvm::SmallVector<mlir::Type> members; |
There was a problem hiding this comment.
Not sure about mlir namespace here and few other places in this function. There is top-level using namespace mlir; and usage of namespace is quite incoherent in the file.
There was a problem hiding this comment.
In general, I like the explicit use of mlir:: in the CIR code because it makes it clear which things are CIR-specific and which are coming from the shared MLIR implementation. I'd prefer to get rid of using namespace mlir; in the places where we use it.
There was a problem hiding this comment.
In general, I like the explicit use of mlir:: in the CIR code because it makes it clear which things are CIR-specific
+1
|
@erichkeane Are you happy with this in its current state? I will make changes to change the polarity of complete/incomplete and to merge RecordKind::class and RecordKind::struct in the incubator and then merge them upstream if no issues are found. |
erichkeane
approved these changes
Apr 14, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/59/builds/16012 Here is the relevant piece of the build log for the reference |
var-const
pushed a commit
to ldionne/llvm-project
that referenced
this pull request
Apr 17, 2025
This change adds minimal support for structure types. To keep the initial change small, only incomplete declarations are being supported in this patch. More complete support will follow.
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.
This change adds minimal support for structure types. To keep the initial change small, only incomplete declarations are being supported in this patch. More complete support will follow.