From 4cec9d3ba329a5331d6d8ceb1726d1112368b164 Mon Sep 17 00:00:00 2001 From: Vinicius Couto Espindola <34522047+sitio-couto@users.noreply.github.com> Date: Wed, 1 Nov 2023 20:13:53 -0300 Subject: [PATCH] [CIR][CIRGen] Refactor StructType builders (#294) Instead of using a single builder for every possible StructType, we now have three builders: identified complete, identified incomplete, and anonymous struct types. This allows us to enforce correctness and to explicitly show the intent when creating a StructType. This patch also adds support for anonymous structs type aliases. When a StructType has no name, it will generate a `ty_anon_` alias. Conflicts are automatically resolved by MLIR. --- .../include/clang/CIR/Dialect/IR/CIRTypes.td | 44 ++++++++++++++++--- clang/lib/CIR/CodeGen/CIRGenBuilder.h | 43 ++++++++++++------ clang/lib/CIR/CodeGen/CIRGenTypes.cpp | 3 +- .../CIR/CodeGen/CIRRecordLayoutBuilder.cpp | 10 +++-- clang/lib/CIR/Dialect/IR/CIRDialect.cpp | 7 +-- clang/lib/CIR/Dialect/IR/CIRTypes.cpp | 17 ++++++- clang/test/CIR/CodeGen/struct.c | 2 +- clang/test/CIR/CodeGen/vbase.cpp | 1 + clang/test/CIR/CodeGen/vtable-rtti.cpp | 6 +-- clang/test/CIR/IR/aliases.cir | 11 +++-- clang/test/CIR/IR/invalid.cir | 7 +++ 11 files changed, 114 insertions(+), 37 deletions(-) diff --git a/clang/include/clang/CIR/Dialect/IR/CIRTypes.td b/clang/include/clang/CIR/Dialect/IR/CIRTypes.td index 96078c8bbeb5..7b0c060cabe9 100644 --- a/clang/include/clang/CIR/Dialect/IR/CIRTypes.td +++ b/clang/include/clang/CIR/Dialect/IR/CIRTypes.td @@ -115,6 +115,37 @@ def CIR_StructType : CIR_Type<"Struct", "struct", "ASTRecordDeclInterface":$ast ); + let skipDefaultBuilders = 1; + let builders = [ + // Build an identified and complete struct. + TypeBuilder<(ins + "ArrayRef":$members, + "StringAttr":$name, + "bool":$packed, + "RecordKind":$kind, + CArg<"ASTRecordDeclInterface", "nullptr">:$ast), [{ + return $_get(context, members, name, /*incomplete=*/false, + packed, kind, ast); + }]>, + // Build an incomplete struct. + TypeBuilder<(ins + "StringAttr":$name, + "RecordKind":$kind), [{ + return $_get(context, /*members=*/ArrayRef{}, name, + /*incomplete=*/true, /*packed=*/false, kind, + /*ast=*/nullptr); + }]>, + // Build an anonymous struct. + TypeBuilder<(ins + "ArrayRef":$members, + "bool":$packed, + "RecordKind":$kind, + CArg<"ASTRecordDeclInterface", "nullptr">:$ast), [{ + return $_get(context, members, /*name=*/nullptr, + /*incomplete=*/false, packed, kind, ast); + }]> + ]; + let hasCustomAssemblyFormat = 1; let extraClassDeclaration = [{ @@ -138,18 +169,21 @@ def CIR_StructType : CIR_Type<"Struct", "struct", bool isComplete() const { return !getIncomplete(); } bool isPadded(const ::mlir::DataLayout &dataLayout) const; - std::string getPrefixedName() { - const auto name = getName().getValue().str(); + std::string getKindAsStr() { switch (getKind()) { case RecordKind::Class: - return "class." + name; + return "class"; case RecordKind::Union: - return "union." + name; + return "union"; case RecordKind::Struct: - return "struct." + name; + return "struct"; } } + std::string getPrefixedName() { + return getKindAsStr() + "." + getName().getValue().str(); + } + /// Return the member with the largest bit-length. mlir::Type getLargestMember(const ::mlir::DataLayout &dataLayout) const; diff --git a/clang/lib/CIR/CodeGen/CIRGenBuilder.h b/clang/lib/CIR/CodeGen/CIRGenBuilder.h index 6b0e0f5401a2..fa93e0405013 100644 --- a/clang/lib/CIR/CodeGen/CIRGenBuilder.h +++ b/clang/lib/CIR/CodeGen/CIRGenBuilder.h @@ -175,12 +175,11 @@ class CIRGenBuilderTy : public CIRBaseBuilderTy { isZero &= isNullValue(typedAttr); } - // Struct type not specified: create type from members. + // Struct type not specified: create anon struct type from members. if (!structTy) - structTy = getType( - members, mlir::StringAttr::get(getContext()), - /*incomplete=*/false, packed, mlir::cir::StructType::Struct, - /*ast=*/nullptr); + structTy = getType(members, packed, + mlir::cir::StructType::Struct, + /*ast=*/nullptr); // Return zero or anonymous constant struct. if (isZero) @@ -200,7 +199,7 @@ class CIRGenBuilderTy : public CIRBaseBuilderTy { } if (!ty) - ty = getAnonStructTy(members, /*incomplete=*/false, packed); + ty = getAnonStructTy(members, packed); auto sTy = ty.dyn_cast(); assert(sTy && "expected struct type"); @@ -397,9 +396,15 @@ class CIRGenBuilderTy : public CIRBaseBuilderTy { /// Get a CIR anonymous struct type. mlir::cir::StructType - getAnonStructTy(llvm::ArrayRef members, bool incomplete, - bool packed = false, const clang::RecordDecl *ast = nullptr) { - return getStructTy(members, "", incomplete, packed, ast); + getAnonStructTy(llvm::ArrayRef members, bool packed = false, + const clang::RecordDecl *ast = nullptr) { + mlir::cir::ASTRecordDeclAttr astAttr = nullptr; + auto kind = mlir::cir::StructType::RecordKind::Struct; + if (ast) { + astAttr = getAttr(ast); + kind = getRecordKind(ast->getTagKind()); + } + return getType(members, packed, kind, astAttr); } /// Get a CIR record kind from a AST declaration tag. @@ -419,10 +424,20 @@ class CIRGenBuilderTy : public CIRBaseBuilderTy { } } + /// Get a incomplete CIR struct type. + mlir::cir::StructType getIncompleteStructTy(llvm::StringRef name, + const clang::RecordDecl *ast) { + const auto nameAttr = getStringAttr(name); + auto kind = mlir::cir::StructType::RecordKind::Struct; + if (ast) + kind = getRecordKind(ast->getTagKind()); + return getType(nameAttr, kind); + } + /// Get a CIR named struct type. - mlir::cir::StructType getStructTy(llvm::ArrayRef members, - llvm::StringRef name, bool incomplete, - bool packed, const clang::RecordDecl *ast) { + mlir::cir::StructType getCompleteStructTy(llvm::ArrayRef members, + llvm::StringRef name, bool packed, + const clang::RecordDecl *ast) { const auto nameAttr = getStringAttr(name); mlir::cir::ASTRecordDeclAttr astAttr = nullptr; auto kind = mlir::cir::StructType::RecordKind::Struct; @@ -430,8 +445,8 @@ class CIRGenBuilderTy : public CIRBaseBuilderTy { astAttr = getAttr(ast); kind = getRecordKind(ast->getTagKind()); } - return mlir::cir::StructType::get(getContext(), members, nameAttr, - incomplete, packed, kind, astAttr); + return getType(members, nameAttr, packed, kind, + astAttr); } // diff --git a/clang/lib/CIR/CodeGen/CIRGenTypes.cpp b/clang/lib/CIR/CodeGen/CIRGenTypes.cpp index aa5bbfc262d2..6240539377a7 100644 --- a/clang/lib/CIR/CodeGen/CIRGenTypes.cpp +++ b/clang/lib/CIR/CodeGen/CIRGenTypes.cpp @@ -182,8 +182,7 @@ mlir::Type CIRGenTypes::convertRecordDeclType(const clang::RecordDecl *RD) { // Handle forward decl / incomplete types. if (!entry) { auto name = getRecordTypeName(RD, ""); - entry = Builder.getStructTy({}, name, /*incomplete=*/true, /*packed=*/false, - RD); + entry = Builder.getIncompleteStructTy(name, RD); recordDeclTypes[key] = entry; } diff --git a/clang/lib/CIR/CodeGen/CIRRecordLayoutBuilder.cpp b/clang/lib/CIR/CodeGen/CIRRecordLayoutBuilder.cpp index fb17c7dbc3ff..9e75af34c333 100644 --- a/clang/lib/CIR/CodeGen/CIRRecordLayoutBuilder.cpp +++ b/clang/lib/CIR/CodeGen/CIRRecordLayoutBuilder.cpp @@ -608,8 +608,9 @@ CIRGenTypes::computeRecordLayout(const RecordDecl *D, builder.astRecordLayout.getSize()) { CIRRecordLowering baseBuilder(*this, D, /*Packed=*/builder.isPacked); auto baseIdentifier = getRecordTypeName(D, ".base"); - *BaseTy = Builder.getStructTy(baseBuilder.fieldTypes, baseIdentifier, - /*incomplete=*/false, /*packed=*/false, D); + *BaseTy = + Builder.getCompleteStructTy(baseBuilder.fieldTypes, baseIdentifier, + /*packed=*/false, D); // TODO(cir): add something like addRecordTypeName // BaseTy and Ty must agree on their packedness for getCIRFieldNo to work @@ -622,8 +623,9 @@ CIRGenTypes::computeRecordLayout(const RecordDecl *D, // Fill in the struct *after* computing the base type. Filling in the body // signifies that the type is no longer opaque and record layout is complete, // but we may need to recursively layout D while laying D out as a base type. - *Ty = Builder.getStructTy(builder.fieldTypes, getRecordTypeName(D, ""), - /*incomplete=*/false, /*packed=*/false, D); + *Ty = + Builder.getCompleteStructTy(builder.fieldTypes, getRecordTypeName(D, ""), + /*packed=*/false, D); auto RL = std::make_unique( Ty ? *Ty : mlir::cir::StructType{}, diff --git a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp index 2e6a2f2db798..daf5b71d5502 100644 --- a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp +++ b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp @@ -53,9 +53,10 @@ struct CIROpAsmDialectInterface : public OpAsmDialectInterface { AliasResult getAlias(Type type, raw_ostream &os) const final { if (auto structType = type.dyn_cast()) { - // TODO(cir): generate unique alias names for anonymous records. - if (!structType.getName()) - return AliasResult::NoAlias; + if (!structType.getName()) { + os << "ty_anon_" << structType.getKindAsStr(); + return AliasResult::OverridableAlias; + } os << "ty_" << structType.getName(); return AliasResult::OverridableAlias; } diff --git a/clang/lib/CIR/Dialect/IR/CIRTypes.cpp b/clang/lib/CIR/Dialect/IR/CIRTypes.cpp index 2efa39d3adac..f7443ac0b92a 100644 --- a/clang/lib/CIR/Dialect/IR/CIRTypes.cpp +++ b/clang/lib/CIR/Dialect/IR/CIRTypes.cpp @@ -108,6 +108,7 @@ Type StructType::parse(mlir::AsmParser &parser) { const auto loc = parser.getCurrentLocation(); bool packed = false; RecordKind kind; + auto *context = parser.getContext(); if (parser.parseLess()) return {}; @@ -152,8 +153,20 @@ Type StructType::parse(mlir::AsmParser &parser) { if (parser.parseGreater()) return {}; - return StructType::get(parser.getContext(), members, name, incomplete, packed, - kind, nullptr); + // Try to create the proper type. + mlir::Type type = {}; + if (name && incomplete) { // Identified & incomplete + type = StructType::get(context, name, kind); + } else if (name && !incomplete) { // Identified & complete + type = StructType::get(context, members, name, packed, kind); + } else if (!name && !incomplete) { // anonymous + type = StructType::get(context, members, packed, kind); + } else { + parser.emitError(loc, "anonymous structs must be complete"); + return {}; + } + + return type; } void StructType::print(mlir::AsmPrinter &printer) const { diff --git a/clang/test/CIR/CodeGen/struct.c b/clang/test/CIR/CodeGen/struct.c index e3ed1ac15759..d19335f0e3c5 100644 --- a/clang/test/CIR/CodeGen/struct.c +++ b/clang/test/CIR/CodeGen/struct.c @@ -22,7 +22,7 @@ void baz(void) { struct Foo f; } -// CHECK-DAG: !ty_22Node22 = !cir.struct +// CHECK-DAG: !ty_22Node22 = !cir.struct // CHECK-DAG: !ty_22Node221 = !cir.struct} #cir.record.decl.ast> // CHECK-DAG: !ty_22Bar22 = !cir.struct // CHECK-DAG: !ty_22Foo22 = !cir.struct diff --git a/clang/test/CIR/CodeGen/vbase.cpp b/clang/test/CIR/CodeGen/vbase.cpp index 30fd93973047..698c82b4ff2e 100644 --- a/clang/test/CIR/CodeGen/vbase.cpp +++ b/clang/test/CIR/CodeGen/vbase.cpp @@ -2,6 +2,7 @@ // RUN: FileCheck --input-file=%t.cir %s --check-prefix=CIR // RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu -fclangir-enable -emit-llvm %s -o %t.ll // RUN: FileCheck --input-file=%t.ll %s --check-prefix=LLVM +// XFAIL: * struct A { int a; diff --git a/clang/test/CIR/CodeGen/vtable-rtti.cpp b/clang/test/CIR/CodeGen/vtable-rtti.cpp index 6da37c786d2b..e1e0cc437d42 100644 --- a/clang/test/CIR/CodeGen/vtable-rtti.cpp +++ b/clang/test/CIR/CodeGen/vtable-rtti.cpp @@ -18,10 +18,10 @@ class B : public A }; // Type info B. -// CHECK: ![[TypeInfoB:ty_.*]] = !cir.struct, !cir.ptr, !cir.ptr}> +// CHECK: ![[TypeInfoB:ty_.*]] = !cir.struct, !cir.ptr, !cir.ptr}> // vtable for A type -// CHECK: ![[VTableTypeA:ty_.*]] = !cir.struct x 5>}> +// CHECK: ![[VTableTypeA:ty_.*]] = !cir.struct x 5>}> // Class A // CHECK: ![[ClassA:ty_.*]] = !cir.struct>>} #cir.record.decl.ast> @@ -57,7 +57,7 @@ class B : public A // CHECK: } // Vtable definition for A -// cir.global "private" external @_ZTV1A : ![[VTableTypeA]] {alignment = 8 : i64} +// CHECK: cir.global "private" external @_ZTV1A : ![[VTableTypeA]] {alignment = 8 : i64} // A ctor => @A::A() // Calls @A::A() and initialize __vptr with address of A's vtable diff --git a/clang/test/CIR/IR/aliases.cir b/clang/test/CIR/IR/aliases.cir index a22c5dba4bcc..8d6cbd04c7a2 100644 --- a/clang/test/CIR/IR/aliases.cir +++ b/clang/test/CIR/IR/aliases.cir @@ -1,10 +1,15 @@ // RUN: cir-opt %s -o %t.cir // RUN: FileCheck --input-file=%t.cir %s -!s32i = !cir.int module { - // CHECK: cir.func @shouldNotUseAliasWithAnonStruct(%arg0: !cir.struct) - cir.func @shouldNotUseAliasWithAnonStruct(%arg0 : !cir.struct) { + // CHECK: @testAnonRecordsAlias + cir.func @testAnonRecordsAlias() { + // CHECK: cir.alloca !ty_anon_struct, cir.ptr + %0 = cir.alloca !cir.struct}>, cir.ptr }>>, ["A"] + // CHECK: cir.alloca !ty_anon_struct1, cir.ptr + %1 = cir.alloca !cir.struct}>, cir.ptr }>>, ["B"] + // CHECK: cir.alloca !ty_anon_union, cir.ptr + %2 = cir.alloca !cir.struct}>, cir.ptr }>>, ["C"] cir.return } } diff --git a/clang/test/CIR/IR/invalid.cir b/clang/test/CIR/IR/invalid.cir index ce7eafd6a1e8..95b70193bf4c 100644 --- a/clang/test/CIR/IR/invalid.cir +++ b/clang/test/CIR/IR/invalid.cir @@ -548,3 +548,10 @@ module { cir.return } } + + +// ----- + +!u16i = !cir.int +// expected-error@+1 {{anonymous structs must be complete}} +!struct = !cir.struct