[CIR] Upstream initial support for complete record types #135844
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
This adds basic support for populating record types. In order to keep the change small, everything non-essential was deferred to a later change set. Only non-recursive structures are handled. Structures padding is not yet implemented. Bitfields are not supported. No attempt is made to handle ABI requirements for passing structure arguments.
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 adds basic support for populating record types. In order to keep the change small, everything non-essential was deferred to a later change set. Only non-recursive structures are handled. Structures padding is not yet implemented. Bitfields are not supported. No attempt is made to handle ABI requirements for passing structure arguments. Patch is 21.64 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/135844.diff 12 Files Affected:
diff --git a/clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h b/clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h
index 68a4505ca7a5a..6b2c938b8275d 100644
--- a/clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h
+++ b/clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h
@@ -95,6 +95,8 @@ class CIRBaseBuilderTy : public mlir::OpBuilder {
return getZeroAttr(arrTy);
if (auto ptrTy = mlir::dyn_cast<cir::PointerType>(ty))
return getConstNullPtrAttr(ptrTy);
+ if (auto RecordTy = mlir::dyn_cast<cir::RecordType>(ty))
+ return getZeroAttr(RecordTy);
if (mlir::isa<cir::BoolType>(ty)) {
return getCIRBoolAttr(false);
}
diff --git a/clang/include/clang/CIR/Dialect/IR/CIRTypes.td b/clang/include/clang/CIR/Dialect/IR/CIRTypes.td
index c60af47f09def..23e20755dca95 100644
--- a/clang/include/clang/CIR/Dialect/IR/CIRTypes.td
+++ b/clang/include/clang/CIR/Dialect/IR/CIRTypes.td
@@ -499,6 +499,9 @@ def CIR_RecordType : CIR_Type<"Record", "record",
std::string getPrefixedName() {
return getKindAsStr() + "." + getName().getValue().str();
}
+
+ void complete(llvm::ArrayRef<mlir::Type> members, bool packed,
+ bool isPadded);
}];
let hasCustomAssemblyFormat = 1;
diff --git a/clang/include/clang/CIR/MissingFeatures.h b/clang/include/clang/CIR/MissingFeatures.h
index 6f2fd2cb2b3ad..cdc56edd5e409 100644
--- a/clang/include/clang/CIR/MissingFeatures.h
+++ b/clang/include/clang/CIR/MissingFeatures.h
@@ -103,6 +103,13 @@ struct MissingFeatures {
// RecordType
static bool recordTypeLayoutInfo() { return false; }
+ static bool recursiveRecordLayout() { return false; }
+ static bool skippedLayout() { return false; }
+ static bool astRecordDeclAttr() { return false; }
+ static bool cxxSupport() { return false; }
+ static bool packedRecords() { return false; }
+ static bool recordPadding() { return false; }
+ static bool recordZeroInit() { return false; }
// Misc
static bool cxxABI() { return false; }
diff --git a/clang/lib/CIR/CodeGen/CIRGenRecordLayout.h b/clang/lib/CIR/CodeGen/CIRGenRecordLayout.h
new file mode 100644
index 0000000000000..a51e0460d1074
--- /dev/null
+++ b/clang/lib/CIR/CodeGen/CIRGenRecordLayout.h
@@ -0,0 +1,54 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_LIB_CIR_CIRGENRECORDLAYOUT_H
+#define LLVM_CLANG_LIB_CIR_CIRGENRECORDLAYOUT_H
+
+#include "clang/AST/Decl.h"
+#include "clang/CIR/Dialect/IR/CIRTypes.h"
+
+namespace clang::CIRGen {
+
+/// This class handles record and union layout info while lowering AST types
+/// to CIR types.
+///
+/// These layout objects are only created on demand as CIR generation requires.
+class CIRGenRecordLayout {
+ friend class CIRGenTypes;
+
+ CIRGenRecordLayout(const CIRGenRecordLayout &) = delete;
+ void operator=(const CIRGenRecordLayout &) = delete;
+
+private:
+ /// The CIR type corresponding to this record layout; used when laying it out
+ /// as a complete object.
+ cir::RecordType completeObjectType;
+
+ /// Map from (non-bit-field) record field to the corresponding cir record type
+ /// field no. This info is populated by the record builder.
+ llvm::DenseMap<const clang::FieldDecl *, unsigned> fieldInfo;
+
+public:
+ CIRGenRecordLayout(cir::RecordType completeObjectType)
+ : completeObjectType(completeObjectType) {}
+
+ /// Return the "complete object" LLVM type associated with
+ /// this record.
+ cir::RecordType getCIRType() const { return completeObjectType; }
+
+ /// Return cir::RecordType element number that corresponds to the field FD.
+ unsigned getCIRFieldNo(const clang::FieldDecl *FD) const {
+ FD = FD->getCanonicalDecl();
+ assert(fieldInfo.count(FD) && "Invalid field for record!");
+ return fieldInfo.lookup(FD);
+ }
+};
+
+} // namespace clang::CIRGen
+
+#endif
diff --git a/clang/lib/CIR/CodeGen/CIRGenRecordLayoutBuilder.cpp b/clang/lib/CIR/CodeGen/CIRGenRecordLayoutBuilder.cpp
new file mode 100644
index 0000000000000..d14eb2f3e2f51
--- /dev/null
+++ b/clang/lib/CIR/CodeGen/CIRGenRecordLayoutBuilder.cpp
@@ -0,0 +1,212 @@
+//===----------------------------------------------------------------------===//
+//
+// 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 contains code to compute the layout of a record.
+//
+//===----------------------------------------------------------------------===//
+
+#include "CIRGenBuilder.h"
+#include "CIRGenModule.h"
+#include "CIRGenTypes.h"
+
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/RecordLayout.h"
+#include "clang/CIR/Dialect/IR/CIRAttrs.h"
+#include "llvm/Support/Casting.h"
+
+#include <memory>
+
+using namespace llvm;
+using namespace clang;
+using namespace clang::CIRGen;
+
+namespace {
+/// The CIRRecordLowering is responsible for lowering an ASTRecordLayout to an
+/// mlir::Type. Some of the lowering is straightforward, some is not.
+// TODO: Detail some of the complexities and weirdnesses?
+// (See CGRecordLayoutBuilder.cpp)
+struct CIRRecordLowering final {
+
+ // MemberInfo is a helper structure that contains information about a record
+ // member. In addition to the standard member types, there exists a sentinel
+ // member type that ensures correct rounding.
+ struct MemberInfo final {
+ CharUnits offset;
+ enum class InfoKind { Field } kind;
+ mlir::Type data;
+ union {
+ const FieldDecl *fieldDecl;
+ // CXXRecordDecl will be used here when supported.
+ };
+ MemberInfo(CharUnits offset, InfoKind kind, mlir::Type data,
+ const FieldDecl *fieldDecl = nullptr)
+ : offset(offset), kind(kind), data(data), fieldDecl(fieldDecl) {};
+ // MemberInfos are sorted so we define a < operator.
+ bool operator<(const MemberInfo &other) const {
+ return offset < other.offset;
+ }
+ };
+ // The constructor.
+ CIRRecordLowering(CIRGenTypes &cirGenTypes, const RecordDecl *recordDecl,
+ bool isPacked);
+
+ void lower();
+
+ void accumulateFields();
+
+ CharUnits bitsToCharUnits(uint64_t bitOffset) {
+ return astContext.toCharUnitsFromBits(bitOffset);
+ }
+
+ CharUnits getSize(mlir::Type Ty) {
+ assert(!cir::MissingFeatures::recordTypeLayoutInfo());
+ return CharUnits::One();
+ }
+ CharUnits getAlignment(mlir::Type Ty) {
+ assert(!cir::MissingFeatures::recordTypeLayoutInfo());
+ return CharUnits::One();
+ }
+
+ mlir::Type getStorageType(const FieldDecl *fieldDecl) {
+ mlir::Type type = cirGenTypes.convertTypeForMem(fieldDecl->getType());
+ if (fieldDecl->isBitField()) {
+ cirGenTypes.getCGModule().errorNYI(recordDecl->getSourceRange(),
+ "getStorageType for bitfields");
+ }
+ return type;
+ }
+
+ uint64_t getFieldBitOffset(const FieldDecl *fieldDecl) {
+ return astRecordLayout.getFieldOffset(fieldDecl->getFieldIndex());
+ }
+
+ /// Fills out the structures that are ultimately consumed.
+ void fillOutputFields();
+
+ CIRGenTypes &cirGenTypes;
+ CIRGenBuilderTy &builder;
+ const ASTContext &astContext;
+ const RecordDecl *recordDecl;
+ const ASTRecordLayout &astRecordLayout;
+ // Helpful intermediate data-structures
+ std::vector<MemberInfo> members;
+ // Output fields, consumed by CIRGenTypes::computeRecordLayout
+ llvm::SmallVector<mlir::Type, 16> fieldTypes;
+ llvm::DenseMap<const FieldDecl *, unsigned> fields;
+ bool zeroInitializable : 1;
+ bool packed : 1;
+ bool padded : 1;
+
+private:
+ CIRRecordLowering(const CIRRecordLowering &) = delete;
+ void operator=(const CIRRecordLowering &) = delete;
+}; // CIRRecordLowering
+} // namespace
+
+CIRRecordLowering::CIRRecordLowering(CIRGenTypes &cirGenTypes,
+ const RecordDecl *recordDecl,
+ bool isPacked)
+ : cirGenTypes(cirGenTypes), builder(cirGenTypes.getBuilder()),
+ astContext(cirGenTypes.getASTContext()), recordDecl(recordDecl),
+ astRecordLayout(
+ cirGenTypes.getASTContext().getASTRecordLayout(recordDecl)),
+ zeroInitializable(true), packed(isPacked), padded(false) {}
+
+void CIRRecordLowering::lower() {
+ if (recordDecl->isUnion()) {
+ cirGenTypes.getCGModule().errorNYI(recordDecl->getSourceRange(),
+ "lower: union");
+ return;
+ }
+
+ assert(!cir::MissingFeatures::cxxSupport());
+
+ accumulateFields();
+
+ llvm::stable_sort(members);
+ // TODO: implement clipTailPadding once bitfields are implemented
+ assert(!cir::MissingFeatures::bitfields());
+ // TODO: implemented packed records
+ assert(!cir::MissingFeatures::packedRecords());
+ // TODO: implement padding
+ assert(!cir::MissingFeatures::recordPadding());
+ // TODO: support zeroInit
+ assert(!cir::MissingFeatures::recordZeroInit());
+
+ fillOutputFields();
+}
+
+void CIRRecordLowering::fillOutputFields() {
+ for (const MemberInfo &member : members) {
+ if (member.data)
+ fieldTypes.push_back(member.data);
+ if (member.kind == MemberInfo::InfoKind::Field) {
+ if (member.fieldDecl)
+ fields[member.fieldDecl->getCanonicalDecl()] = fieldTypes.size() - 1;
+ // A field without storage must be a bitfield.
+ assert(!cir::MissingFeatures::bitfields());
+ }
+ assert(!cir::MissingFeatures::cxxSupport());
+ }
+}
+
+void CIRRecordLowering::accumulateFields() {
+ for (const FieldDecl *field : recordDecl->fields()) {
+ if (field->isBitField()) {
+ cirGenTypes.getCGModule().errorNYI(recordDecl->getSourceRange(),
+ "accumulate bitfields");
+ ++field;
+ } else if (!field->isZeroSize(astContext)) {
+ members.push_back(MemberInfo(bitsToCharUnits(getFieldBitOffset(field)),
+ MemberInfo::InfoKind::Field,
+ getStorageType(field), field));
+ ++field;
+ } else {
+ // TODO(cir): do we want to do anything special about zero size members?
+ ++field;
+ }
+ }
+}
+
+std::unique_ptr<CIRGenRecordLayout>
+CIRGenTypes::computeRecordLayout(const RecordDecl *rd, cir::RecordType *ty) {
+ CIRRecordLowering lowering(*this, rd, /*packed=*/false);
+ assert(ty->isIncomplete() && "recomputing record layout?");
+ lowering.lower();
+
+ // If we're in C++, compute the base subobject type.
+ if (llvm::isa<CXXRecordDecl>(rd) && !rd->isUnion() &&
+ !rd->hasAttr<FinalAttr>()) {
+ cgm.errorNYI(rd->getSourceRange(), "computeRecordLayout: CXXRecordDecl");
+ }
+
+ // Fill in the record *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 rd while laying D out as a base type.
+ assert(!cir::MissingFeatures::astRecordDeclAttr());
+ ty->complete(lowering.fieldTypes, lowering.packed, lowering.padded);
+
+ auto rl = std::make_unique<CIRGenRecordLayout>(ty ? *ty : cir::RecordType());
+
+ assert(!cir::MissingFeatures::recordZeroInit());
+ assert(!cir::MissingFeatures::cxxSupport());
+ assert(!cir::MissingFeatures::bitfields());
+
+ // Add all the field numbers.
+ rl->fieldInfo.swap(lowering.fields);
+
+ // Dump the layout, if requested.
+ if (getASTContext().getLangOpts().DumpRecordLayouts) {
+ cgm.errorNYI(rd->getSourceRange(), "computeRecordLayout: dump layout");
+ }
+
+ // TODO: implement verification
+ return rl;
+}
diff --git a/clang/lib/CIR/CodeGen/CIRGenTypes.cpp b/clang/lib/CIR/CodeGen/CIRGenTypes.cpp
index f625f83257859..ec77c4428d43b 100644
--- a/clang/lib/CIR/CodeGen/CIRGenTypes.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenTypes.cpp
@@ -112,6 +112,18 @@ std::string CIRGenTypes::getRecordTypeName(const clang::RecordDecl *recordDecl,
return builder.getUniqueRecordName(std::string(typeName));
}
+// 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) {
+ // If no records are being laid out, we can certainly do this one.
+ if (CGT.noRecordsBeingLaidOut())
+ return true;
+
+ assert(!cir::MissingFeatures::recursiveRecordLayout());
+ return false;
+}
+
/// 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
@@ -132,7 +144,40 @@ mlir::Type CIRGenTypes::convertRecordDeclType(const clang::RecordDecl *rd) {
if (!rd || !rd->isCompleteDefinition() || entry.isComplete())
return entry;
- cgm.errorNYI(rd->getSourceRange(), "Complete record type");
+ // If converting this type would cause us to infinitely loop, don't do it!
+ if (!isSafeToConvert(rd, *this)) {
+ cgm.errorNYI(rd->getSourceRange(), "recursive record layout");
+ return entry;
+ }
+
+ // Okay, this is a definition of a type. Compile the implementation now.
+ bool insertResult = recordsBeingLaidOut.insert(key).second;
+ (void)insertResult;
+ assert(insertResult && "isSafeToCovert() should have caught this.");
+
+ // Force conversion of non-virtual base classes recursively.
+ if (const auto *cxxRecordDecl = dyn_cast<CXXRecordDecl>(rd)) {
+ cgm.errorNYI(rd->getSourceRange(), "CXXRecordDecl");
+ }
+
+ // Layout fields.
+ std::unique_ptr<CIRGenRecordLayout> layout = computeRecordLayout(rd, &entry);
+ recordDeclTypes[key] = entry;
+ cirGenRecordLayouts[key] = std::move(layout);
+
+ // We're done laying out this record.
+ bool eraseResult = recordsBeingLaidOut.erase(key);
+ (void)eraseResult;
+ assert(eraseResult && "record not in RecordsBeingLaidOut set?");
+
+ // If this record blocked a FunctionType conversion, then recompute whatever
+ // was derived from that.
+ assert(!cir::MissingFeatures::skippedLayout());
+
+ // If we're done converting the outer-most record, then convert any deferred
+ // records as well.
+ assert(!cir::MissingFeatures::recursiveRecordLayout());
+
return entry;
}
diff --git a/clang/lib/CIR/CodeGen/CIRGenTypes.h b/clang/lib/CIR/CodeGen/CIRGenTypes.h
index fd855bf958ccb..38e4bb2f688ab 100644
--- a/clang/lib/CIR/CodeGen/CIRGenTypes.h
+++ b/clang/lib/CIR/CodeGen/CIRGenTypes.h
@@ -14,9 +14,10 @@
#define LLVM_CLANG_LIB_CODEGEN_CODEGENTYPES_H
#include "CIRGenFunctionInfo.h"
-#include "clang/CIR/Dialect/IR/CIRTypes.h"
+#include "CIRGenRecordLayout.h"
#include "clang/AST/Type.h"
+#include "clang/CIR/Dialect/IR/CIRTypes.h"
#include "llvm/ADT/SmallPtrSet.h"
@@ -45,12 +46,22 @@ class CIRGenTypes {
clang::ASTContext &astContext;
CIRGenBuilderTy &builder;
+ /// Contains the CIR type for any converted RecordDecl.
+ llvm::DenseMap<const clang::Type *, std::unique_ptr<CIRGenRecordLayout>>
+ cirGenRecordLayouts;
+
/// Contains the CIR type for any converted RecordDecl
llvm::DenseMap<const clang::Type *, cir::RecordType> recordDeclTypes;
/// Hold memoized CIRGenFunctionInfo results
llvm::FoldingSet<CIRGenFunctionInfo> functionInfos;
+ /// This set keeps track of records that we're currently converting to a CIR
+ /// type. For example, when converting:
+ /// struct A { struct B { int x; } } when processing 'x', the 'A' and 'B'
+ /// types will be in this set.
+ llvm::SmallPtrSet<const clang::Type *, 4> recordsBeingLaidOut;
+
llvm::SmallPtrSet<const CIRGenFunctionInfo *, 4> functionsBeingProcessed;
/// Heper for convertType.
mlir::Type convertFunctionTypeInternal(clang::QualType ft);
@@ -59,6 +70,9 @@ class CIRGenTypes {
CIRGenTypes(CIRGenModule &cgm);
~CIRGenTypes();
+ CIRGenBuilderTy &getBuilder() const { return builder; }
+ CIRGenModule &getCGModule() const { return cgm; }
+
/// Utility to check whether a function type can be converted to a CIR type
/// (i.e. doesn't depend on an incomplete tag type).
bool isFuncTypeConvertible(const clang::FunctionType *ft);
@@ -70,12 +84,18 @@ class CIRGenTypes {
TypeCacheTy typeCache;
mlir::MLIRContext &getMLIRContext() const;
+ clang::ASTContext &getASTContext() const { return astContext; }
+
+ bool noRecordsBeingLaidOut() const { return recordsBeingLaidOut.empty(); }
/// Convert a Clang type into a mlir::Type.
mlir::Type convertType(clang::QualType type);
mlir::Type convertRecordDeclType(const clang::RecordDecl *recordDecl);
+ std::unique_ptr<CIRGenRecordLayout>
+ computeRecordLayout(const clang::RecordDecl *rd, cir::RecordType *ty);
+
std::string getRecordTypeName(const clang::RecordDecl *,
llvm::StringRef suffix);
diff --git a/clang/lib/CIR/CodeGen/CMakeLists.txt b/clang/lib/CIR/CodeGen/CMakeLists.txt
index dc18f7f2af160..418bc2db408cb 100644
--- a/clang/lib/CIR/CodeGen/CMakeLists.txt
+++ b/clang/lib/CIR/CodeGen/CMakeLists.txt
@@ -18,6 +18,7 @@ add_clang_library(clangCIR
CIRGenExprScalar.cpp
CIRGenFunction.cpp
CIRGenModule.cpp
+ CIRGenRecordLayoutBuilder.cpp
CIRGenStmt.cpp
CIRGenStmtOpenACC.cpp
CIRGenTypes.cpp
diff --git a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
index f3e5e572653da..1f4232b9e29ec 100644
--- a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
+++ b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
@@ -212,7 +212,7 @@ static LogicalResult checkConstantTypes(mlir::Operation *op, mlir::Type opType,
}
if (isa<cir::ZeroAttr>(attrType)) {
- if (::mlir::isa<cir::ArrayType>(opType))
+ if (isa<cir::RecordType, cir::ArrayType>(opType))
return success();
return op->emitOpError("zero expects struct or array type");
}
diff --git a/clang/lib/CIR/Dialect/IR/CIRTypes.cpp b/clang/lib/CIR/Dialect/IR/CIRTypes.cpp
index 9b0177f159084..160732d9c3610 100644
--- a/clang/lib/CIR/Dialect/IR/CIRTypes.cpp
+++ b/clang/lib/CIR/Dialect/IR/CIRTypes.cpp
@@ -214,6 +214,12 @@ cir::RecordType::RecordKind RecordType::getKind() const {
return getImpl()->kind;
}
+void RecordType::complete(ArrayRef<Type> members, bool packed, bool padded) {
+ assert(!cir::MissingFeatures::astRecordDeclAttr());
+ if (mutate(members, packed, padded).failed())
+ llvm_unreachable("failed to complete record");
+}
+
//===----------------------------------------------------------------------===//
// Data Layout information for types
//===----------------------------------------------------------------------===//
diff --git a/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp b/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp
index 7159f89c93a53..cb318c88c09c5 100644
--- a/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp
+++ b/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp
@@ -1337,6 +1337,35 @@ static void prepareTypeConverter(mlir::LLVMTypeConverter &converter,
converter.addConversion([&](cir::BF16Type type) -> mlir::Type {
return mlir::BFloat16Type::get(type.getContext());
});
+ converter.addConversion([&](cir::RecordType type) -> mlir::Type {
+ // Convert struct members.
+ llvm::SmallVector<mlir::Type> llvmMembers;
+ switch (type.getKind()) {
+ case cir::RecordType::Struct:
+ for (mlir::Type ty : type.getMembers())
+ llvmMembers.push_back(convertTypeForMemory(converter, dataLayout, ty));
+ break;
+ // Unions are lowered as only the largest member.
+ case cir::RecordType::Union:
+ llvm_unreachable("Lowering of unions is NYI");
+ break;
+ }
+
+ // Record has a name: lower as an identified record.
+ mlir::LLVM::LLVMStructType llvmStruct;
+ if (type.getName()) {
+ llvmStruct = mlir::LLVM::LLVMStructType::getIdentified(
+ type.getContext(), type.getPrefixedName());
+ assert(!cir::MissingFeatures::packedRecords());
+ if (llvmStruct.setBody(llvmMembers, /*isPacked=*/true).failed())
+ llvm_unreacha...
[truncated]
|
erichkeane
reviewed
Apr 15, 2025
| mlir::Type data; | ||
| union { | ||
| const FieldDecl *fieldDecl; | ||
| // CXXRecordDecl will be used here when supported. |
There was a problem hiding this comment.
Is this because MemberInfo is also supposed to represent base types?
There was a problem hiding this comment.
Then can we change this to:
Suggested change
| // CXXRecordDecl will be used here when supported. | |
| // CXXRecordDecl will be used here when base-types are supported. |
There was a problem hiding this comment.
See this suggestion/change request here.
| // Output fields, consumed by CIRGenTypes::computeRecordLayout | ||
| llvm::SmallVector<mlir::Type, 16> fieldTypes; | ||
| llvm::DenseMap<const FieldDecl *, unsigned> fields; | ||
| bool zeroInitializable : 1; |
There was a problem hiding this comment.
These can't be bool type, they all have to be unsigned, else they get laid-out funny/inconsistently. However, you can/should use LLVM_PREFERRED_TYPE(bool) on them.
There was a problem hiding this comment.
Both the incubator and the classic codegen implementation declare them as bool. I did notice that I have to explicitly cast them as bool to access them, which seemed kind of odd. Is that a symptom of what you're talking about?
There was a problem hiding this comment.
Thats a symptom of them being bitfields. But yeah, bool-bitfields are problematic for some ABIs (only sequential bitfields of the same type can be 'joined', and at least 1 ABI will only do so for unsigned and int IIRC). So we use unsigned for them every time we are thinking about it during review. I'm guessing this is something in the classic-codegen that we never noticed.
| zeroInitializable(true), packed(isPacked), padded(false) {} | ||
|
|
||
| void CIRRecordLowering::lower() { | ||
| if (recordDecl->isUnion()) { |
There was a problem hiding this comment.
Should we also NYI on base types? Particularly with virtual base types this gets ugly.
There was a problem hiding this comment.
I cut a lot of code out from here. Anything other than unions will fail somewhere else before we get here, but I guess it wouldn't hurt to add more checks here now.
erichkeane
approved these changes
Apr 15, 2025
| @@ -126,6 +130,12 @@ void CIRRecordLowering::lower() { | |||
| return; | |||
| } | |||
|
|
|||
| if (isa<CXXRecordDecl>(recordDecl)) { | |||
| cirGenTypes.getCGModule().errorNYI(recordDecl->getSourceRange(), | |||
There was a problem hiding this comment.
Oh my! This ends up being harsher than I expected! This means no record-types in C++ at all (even ones that don't do anything). But if that is the level of support we have, I'm ok with it.
I presume a soon-to-be used patch will reduce this to '!virtual && !getNumBases` etc.
There was a problem hiding this comment.
Eventually, yes. We aren't handling CXXRecordDecl in the code that gets here yet. I don't expect there to be much more than adding a switch statement for that, but everything in C++ ends up being more involved than I realized, so who knows.
Comment request? Am I still missing something? |
erichkeane
approved these changes
Apr 15, 2025
Thanks. I missed that somehow. |
bcardosolopes
approved these changes
Apr 16, 2025
| getStorageType(field), field)); | ||
| ++field; | ||
| } else { | ||
| // TODO(cir): do we want to do anything special about zero size members? |
There was a problem hiding this comment.
maybe add an assert on missing feature here? this will lead to LLVM IR lowering differences so might be good to track.
AmrDeveloper
approved these changes
Apr 16, 2025
| @@ -95,6 +95,8 @@ class CIRBaseBuilderTy : public mlir::OpBuilder { | |||
| return getZeroAttr(arrTy); | |||
| if (auto ptrTy = mlir::dyn_cast<cir::PointerType>(ty)) | |||
| return getConstNullPtrAttr(ptrTy); | |||
| if (auto RecordTy = mlir::dyn_cast<cir::RecordType>(ty)) | |||
There was a problem hiding this comment.
NIT
if (auto recordTy = mlir::dyn_cast<cir::RecordType>(ty))
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/59/builds/16142 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 adds basic support for populating record types. In order to keep the change small, everything non-essential was deferred to a later change set. Only non-recursive structures are handled. Structures padding is not yet implemented. Bitfields are not supported. No attempt is made to handle ABI requirements for passing structure arguments.
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 adds basic support for populating record types. In order to keep the change small, everything non-essential was deferred to a later change set. Only non-recursive structures are handled. Structures padding is not yet implemented. Bitfields are not supported. No attempt is made to handle ABI requirements for passing structure arguments.