[MLIR][LLVM] Make DISubprogramAttr cyclic #106571
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
gysit
force-pushed
the
make-subprogram-recursive
branch
3 times, most recently
from
9274e39 to
fad0bda
Compare
|
@llvm/pr-subscribers-flang-fir-hlfir @llvm/pr-subscribers-mlir-llvm Author: Tobias Gysi (gysit) ChangesThis commit implements LLVM_DIRecursiveTypeAttrInterface for the DISubprogramAttr to ensure cyclic subprograms can be imported properly. In the process multiple shortcuts around the recently introduced DIImportedEntityAttr can be removed. Patch is 35.66 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/106571.diff 12 Files Affected:
diff --git a/mlir/include/mlir-c/Dialect/LLVM.h b/mlir/include/mlir-c/Dialect/LLVM.h
index 5eb96a86e472d6..561d698f722afe 100644
--- a/mlir/include/mlir-c/Dialect/LLVM.h
+++ b/mlir/include/mlir-c/Dialect/LLVM.h
@@ -234,6 +234,9 @@ MLIR_CAPI_EXPORTED MlirAttribute mlirLLVMDIBasicTypeAttrGet(
MlirContext ctx, unsigned int tag, MlirAttribute name, uint64_t sizeInBits,
MlirLLVMTypeEncoding encoding);
+/// Creates a self-referencing LLVM DICompositeType attribute.
+MlirAttribute mlirLLVMDICompositeTypeAttrGetSelfRec(MlirAttribute recId);
+
/// Creates a LLVM DICompositeType attribute.
MLIR_CAPI_EXPORTED MlirAttribute mlirLLVMDICompositeTypeAttrGet(
MlirContext ctx, unsigned int tag, MlirAttribute recId, MlirAttribute name,
@@ -311,13 +314,16 @@ MLIR_CAPI_EXPORTED MlirAttribute mlirLLVMDILocalVariableAttrGet(
MlirAttribute diFile, unsigned int line, unsigned int arg,
unsigned int alignInBits, MlirAttribute diType, int64_t flags);
+/// Creates a self-referencing LLVM DISubprogramAttr attribute.
+MlirAttribute mlirLLVMDISubprogramAttrGetSelfRec(MlirAttribute recId);
+
/// Creates a LLVM DISubprogramAttr attribute.
MLIR_CAPI_EXPORTED MlirAttribute mlirLLVMDISubprogramAttrGet(
- MlirContext ctx, MlirAttribute id, MlirAttribute compileUnit,
- MlirAttribute scope, MlirAttribute name, MlirAttribute linkageName,
- MlirAttribute file, unsigned int line, unsigned int scopeLine,
- uint64_t subprogramFlags, MlirAttribute type, intptr_t nRetainedNodes,
- MlirAttribute const *retainedNodes);
+ MlirContext ctx, MlirAttribute id, MlirAttribute recId,
+ MlirAttribute compileUnit, MlirAttribute scope, MlirAttribute name,
+ MlirAttribute linkageName, MlirAttribute file, unsigned int line,
+ unsigned int scopeLine, uint64_t subprogramFlags, MlirAttribute type,
+ intptr_t nRetainedNodes, MlirAttribute const *retainedNodes);
/// Gets the scope from this DISubprogramAttr.
MLIR_CAPI_EXPORTED MlirAttribute
@@ -356,9 +362,9 @@ MLIR_CAPI_EXPORTED MlirAttribute mlirLLVMDIModuleAttrGet(
/// Creates a LLVM DIImportedEntityAttr attribute.
MLIR_CAPI_EXPORTED MlirAttribute mlirLLVMDIImportedEntityAttrGet(
- MlirContext ctx, unsigned int tag, MlirAttribute entity, MlirAttribute file,
- unsigned int line, MlirAttribute name, intptr_t nElements,
- MlirAttribute const *elements);
+ MlirContext ctx, unsigned int tag, MlirAttribute scope,
+ MlirAttribute entity, MlirAttribute file, unsigned int line,
+ MlirAttribute name, intptr_t nElements, MlirAttribute const *elements);
/// Gets the scope of this DIModuleAttr.
MLIR_CAPI_EXPORTED MlirAttribute
diff --git a/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td b/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td
index e57be7f760d380..224e58f3216abf 100644
--- a/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td
+++ b/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td
@@ -554,14 +554,16 @@ def LLVM_DILocalVariableAttr : LLVM_Attr<"DILocalVariable", "di_local_variable",
//===----------------------------------------------------------------------===//
def LLVM_DISubprogramAttr : LLVM_Attr<"DISubprogram", "di_subprogram",
- /*traits=*/[], "DIScopeAttr"> {
+ [LLVM_DIRecursiveTypeAttrInterface],
+ "DIScopeAttr"> {
let parameters = (ins
OptionalParameter<"DistinctAttr">:$id,
+ OptionalParameter<"DistinctAttr">:$recId,
OptionalParameter<"DICompileUnitAttr">:$compileUnit,
- "DIScopeAttr":$scope,
+ OptionalParameter<"DIScopeAttr">:$scope,
OptionalParameter<"StringAttr">:$name,
OptionalParameter<"StringAttr">:$linkageName,
- "DIFileAttr":$file,
+ OptionalParameter<"DIFileAttr">:$file,
OptionalParameter<"unsigned">:$line,
OptionalParameter<"unsigned">:$scopeLine,
OptionalParameter<"DISubprogramFlags">:$subprogramFlags,
@@ -577,13 +579,28 @@ def LLVM_DISubprogramAttr : LLVM_Attr<"DISubprogram", "di_subprogram",
"ArrayRef<DINodeAttr>":$retainedNodes
), [{
MLIRContext *ctx = file.getContext();
- return $_get(ctx, id, compileUnit, scope, StringAttr::get(ctx, name),
+ return $_get(ctx, id, /*recId=*/nullptr, compileUnit, scope,
+ StringAttr::get(ctx, name),
StringAttr::get(ctx, linkageName), file, line,
scopeLine, subprogramFlags, type, retainedNodes);
}]>
];
-
let assemblyFormat = "`<` struct(params) `>`";
+ let extraClassDeclaration = [{
+ /// Requirements of DIRecursiveTypeAttrInterface.
+ /// @{
+
+ /// Get whether this attr describes a recursive self reference.
+ bool isRecSelf() { return !getScope(); }
+
+ /// Get a copy of this type attr but with the recursive ID set to `recId`.
+ DIRecursiveTypeAttrInterface withRecId(DistinctAttr recId);
+
+ /// Build a rec-self instance using the provided `recId`.
+ static DIRecursiveTypeAttrInterface getRecSelf(DistinctAttr recId);
+
+ /// @}
+ }];
}
//===----------------------------------------------------------------------===//
@@ -627,13 +644,9 @@ def LLVM_DINamespaceAttr : LLVM_Attr<"DINamespace", "di_namespace",
def LLVM_DIImportedEntityAttr : LLVM_Attr<"DIImportedEntity", "di_imported_entity",
/*traits=*/[], "DINodeAttr"> {
- /// TODO: DIImportedEntity has a 'scope' field which represents the scope where
- /// this entity is imported. Currently, we are not adding a 'scope' field in
- /// DIImportedEntityAttr to avoid cyclic dependency. As DIImportedEntityAttr
- /// entries will be contained inside a scope entity (e.g. DISubprogramAttr),
- /// the scope can easily be inferred.
let parameters = (ins
LLVM_DITagParameter:$tag,
+ "DIScopeAttr":$scope,
"DINodeAttr":$entity,
OptionalParameter<"DIFileAttr">:$file,
OptionalParameter<"unsigned">:$line,
diff --git a/mlir/lib/CAPI/Dialect/LLVM.cpp b/mlir/lib/CAPI/Dialect/LLVM.cpp
index 13341f0c4de881..ed4d028e192f53 100644
--- a/mlir/lib/CAPI/Dialect/LLVM.cpp
+++ b/mlir/lib/CAPI/Dialect/LLVM.cpp
@@ -159,6 +159,11 @@ MlirAttribute mlirLLVMDIBasicTypeAttrGet(MlirContext ctx, unsigned int tag,
unwrap(ctx), tag, cast<StringAttr>(unwrap(name)), sizeInBits, encoding));
}
+MlirAttribute mlirLLVMDICompositeTypeAttrGetSelfRec(MlirAttribute recId) {
+ return wrap(
+ DICompositeTypeAttr::getRecSelf(cast<DistinctAttr>(unwrap(recId))));
+}
+
MlirAttribute mlirLLVMDICompositeTypeAttrGet(
MlirContext ctx, unsigned int tag, MlirAttribute recId, MlirAttribute name,
MlirAttribute file, uint32_t line, MlirAttribute scope,
@@ -289,16 +294,21 @@ MlirAttribute mlirLLVMDISubroutineTypeAttrGet(MlirContext ctx,
[](Attribute a) { return cast<DITypeAttr>(a); })));
}
+MlirAttribute mlirLLVMDISubprogramAttrGetSelfRec(MlirAttribute recId) {
+ return wrap(DISubprogramAttr::getRecSelf(cast<DistinctAttr>(unwrap(recId))));
+}
+
MlirAttribute mlirLLVMDISubprogramAttrGet(
- MlirContext ctx, MlirAttribute id, MlirAttribute compileUnit,
- MlirAttribute scope, MlirAttribute name, MlirAttribute linkageName,
- MlirAttribute file, unsigned int line, unsigned int scopeLine,
- uint64_t subprogramFlags, MlirAttribute type, intptr_t nRetainedNodes,
- MlirAttribute const *retainedNodes) {
+ MlirContext ctx, MlirAttribute id, MlirAttribute recId,
+ MlirAttribute compileUnit, MlirAttribute scope, MlirAttribute name,
+ MlirAttribute linkageName, MlirAttribute file, unsigned int line,
+ unsigned int scopeLine, uint64_t subprogramFlags, MlirAttribute type,
+ intptr_t nRetainedNodes, MlirAttribute const *retainedNodes) {
SmallVector<Attribute> nodesStorage;
nodesStorage.reserve(nRetainedNodes);
return wrap(DISubprogramAttr::get(
unwrap(ctx), cast<DistinctAttr>(unwrap(id)),
+ cast<DistinctAttr>(unwrap(recId)),
cast<DICompileUnitAttr>(unwrap(compileUnit)),
cast<DIScopeAttr>(unwrap(scope)), cast<StringAttr>(unwrap(name)),
cast<StringAttr>(unwrap(linkageName)), cast<DIFileAttr>(unwrap(file)),
@@ -353,14 +363,15 @@ MlirAttribute mlirLLVMDIModuleAttrGetScope(MlirAttribute diModule) {
}
MlirAttribute mlirLLVMDIImportedEntityAttrGet(
- MlirContext ctx, unsigned int tag, MlirAttribute entity, MlirAttribute file,
- unsigned int line, MlirAttribute name, intptr_t nElements,
- MlirAttribute const *elements) {
+ MlirContext ctx, unsigned int tag, MlirAttribute scope,
+ MlirAttribute entity, MlirAttribute file, unsigned int line,
+ MlirAttribute name, intptr_t nElements, MlirAttribute const *elements) {
SmallVector<Attribute> elementsStorage;
elementsStorage.reserve(nElements);
return wrap(DIImportedEntityAttr::get(
- unwrap(ctx), tag, cast<DINodeAttr>(unwrap(entity)),
- cast<DIFileAttr>(unwrap(file)), line, cast<StringAttr>(unwrap(name)),
+ unwrap(ctx), tag, cast<DIScopeAttr>(unwrap(scope)),
+ cast<DINodeAttr>(unwrap(entity)), cast<DIFileAttr>(unwrap(file)), line,
+ cast<StringAttr>(unwrap(name)),
llvm::map_to_vector(unwrapList(nElements, elements, elementsStorage),
[](Attribute a) { return cast<DINodeAttr>(a); })));
}
diff --git a/mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp b/mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp
index 98a9659735e7e6..0ce719b9a750ca 100644
--- a/mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp
+++ b/mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp
@@ -215,6 +215,22 @@ DICompositeTypeAttr::getRecSelf(DistinctAttr recId) {
{}, DIFlags(), 0, 0, {}, {}, {}, {}, {});
}
+//===----------------------------------------------------------------------===//
+// DISubprogramAttr
+//===----------------------------------------------------------------------===//
+
+DIRecursiveTypeAttrInterface DISubprogramAttr::withRecId(DistinctAttr recId) {
+ return DISubprogramAttr::get(
+ getContext(), getId(), recId, getCompileUnit(), getScope(), getName(),
+ getLinkageName(), getFile(), getLine(), getScopeLine(),
+ getSubprogramFlags(), getType(), getRetainedNodes());
+}
+
+DIRecursiveTypeAttrInterface DISubprogramAttr::getRecSelf(DistinctAttr recId) {
+ return DISubprogramAttr::get(recId.getContext(), {}, recId, {}, {}, {}, {}, 0,
+ 0, {}, {}, {}, {});
+}
+
//===----------------------------------------------------------------------===//
// TargetFeaturesAttr
//===----------------------------------------------------------------------===//
diff --git a/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp b/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
index 3870aab52f199d..6e4a964f1fc93c 100644
--- a/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
+++ b/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
@@ -3155,9 +3155,9 @@ struct LLVMOpAsmDialectInterface : public OpAsmDialectInterface {
.Case<AccessGroupAttr, AliasScopeAttr, AliasScopeDomainAttr,
DIBasicTypeAttr, DICompileUnitAttr, DICompositeTypeAttr,
DIDerivedTypeAttr, DIFileAttr, DIGlobalVariableAttr,
- DIGlobalVariableExpressionAttr, DILabelAttr, DILexicalBlockAttr,
- DILexicalBlockFileAttr, DILocalVariableAttr, DIModuleAttr,
- DINamespaceAttr, DINullTypeAttr, DIStringTypeAttr,
+ DIGlobalVariableExpressionAttr, DIImportedEntityAttr, DILabelAttr,
+ DILexicalBlockAttr, DILexicalBlockFileAttr, DILocalVariableAttr,
+ DIModuleAttr, DINamespaceAttr, DINullTypeAttr, DIStringTypeAttr,
DISubprogramAttr, DISubroutineTypeAttr, LoopAnnotationAttr,
LoopVectorizeAttr, LoopInterleaveAttr, LoopUnrollAttr,
LoopUnrollAndJamAttr, LoopLICMAttr, LoopDistributeAttr,
diff --git a/mlir/lib/Dialect/LLVMIR/Transforms/DIScopeForLLVMFuncOp.cpp b/mlir/lib/Dialect/LLVMIR/Transforms/DIScopeForLLVMFuncOp.cpp
index c75f44bf3976a9..ad9aa7ac80cafa 100644
--- a/mlir/lib/Dialect/LLVMIR/Transforms/DIScopeForLLVMFuncOp.cpp
+++ b/mlir/lib/Dialect/LLVMIR/Transforms/DIScopeForLLVMFuncOp.cpp
@@ -76,8 +76,8 @@ static void addScopeToFunction(LLVM::LLVMFuncOp llvmFunc,
compileUnitAttr = {};
}
auto subprogramAttr = LLVM::DISubprogramAttr::get(
- context, id, compileUnitAttr, fileAttr, funcNameAttr, funcNameAttr,
- fileAttr,
+ context, id, /*recId=*/{}, compileUnitAttr, fileAttr, funcNameAttr,
+ funcNameAttr, fileAttr,
/*line=*/line,
/*scopeline=*/col, subprogramFlags, subroutineTypeAttr,
/*retainedNodes=*/{});
diff --git a/mlir/lib/Target/LLVMIR/DebugImporter.cpp b/mlir/lib/Target/LLVMIR/DebugImporter.cpp
index ce3643f513d34a..509451057dd737 100644
--- a/mlir/lib/Target/LLVMIR/DebugImporter.cpp
+++ b/mlir/lib/Target/LLVMIR/DebugImporter.cpp
@@ -217,8 +217,8 @@ DebugImporter::translateImpl(llvm::DIImportedEntity *node) {
}
return DIImportedEntityAttr::get(
- context, node->getTag(), translate(node->getEntity()),
- translate(node->getFile()), node->getLine(),
+ context, node->getTag(), translate(node->getScope()),
+ translate(node->getEntity()), translate(node->getFile()), node->getLine(),
getStringAttrOrNull(node->getRawName()), elements);
}
@@ -227,6 +227,7 @@ DISubprogramAttr DebugImporter::translateImpl(llvm::DISubprogram *node) {
mlir::DistinctAttr id;
if (node->isDistinct())
id = getOrCreateDistinctID(node);
+
// Return nullptr if the scope or type is invalid.
DIScopeAttr scope = translate(node->getScope());
if (node->getScope() && !scope)
@@ -238,16 +239,19 @@ DISubprogramAttr DebugImporter::translateImpl(llvm::DISubprogram *node) {
if (node->getType() && !type)
return nullptr;
+ // Convert the retained nodes but drop all of them if one of them is invalid.
SmallVector<DINodeAttr> retainedNodes;
for (llvm::DINode *retainedNode : node->getRetainedNodes())
retainedNodes.push_back(translate(retainedNode));
+ if (llvm::is_contained(retainedNodes, nullptr))
+ retainedNodes.clear();
- return DISubprogramAttr::get(context, id, translate(node->getUnit()), scope,
- getStringAttrOrNull(node->getRawName()),
- getStringAttrOrNull(node->getRawLinkageName()),
- translate(node->getFile()), node->getLine(),
- node->getScopeLine(), *subprogramFlags, type,
- retainedNodes);
+ return DISubprogramAttr::get(
+ context, id, /*recId=*/{}, translate(node->getUnit()), scope,
+ getStringAttrOrNull(node->getRawName()),
+ getStringAttrOrNull(node->getRawLinkageName()),
+ translate(node->getFile()), node->getLine(), node->getScopeLine(),
+ *subprogramFlags, type, retainedNodes);
}
DISubrangeAttr DebugImporter::translateImpl(llvm::DISubrange *node) {
@@ -374,6 +378,9 @@ getRecSelfConstructor(llvm::DINode *node) {
.Case([&](llvm::DICompositeType *) {
return CtorType(DICompositeTypeAttr::getRecSelf);
})
+ .Case([&](llvm::DISubprogram *) {
+ return CtorType(DISubprogramAttr::getRecSelf);
+ })
.Default(CtorType());
}
diff --git a/mlir/lib/Target/LLVMIR/DebugTranslation.cpp b/mlir/lib/Target/LLVMIR/DebugTranslation.cpp
index 042e015f107fea..55060659dd3560 100644
--- a/mlir/lib/Target/LLVMIR/DebugTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/DebugTranslation.cpp
@@ -96,6 +96,17 @@ llvm::MDString *DebugTranslation::getMDStringOrNull(StringAttr stringAttr) {
return llvm::MDString::get(llvmCtx, stringAttr);
}
+llvm::MDTuple *
+DebugTranslation::getMDTupleOrNull(ArrayRef<DINodeAttr> elements) {
+ if (elements.empty())
+ return nullptr;
+ SmallVector<llvm::Metadata *> llvmElements = llvm::to_vector(
+ llvm::map_range(elements, [&](DINodeAttr attr) -> llvm::Metadata * {
+ return translate(attr);
+ }));
+ return llvm::MDNode::get(llvmCtx, llvmElements);
+}
+
llvm::DIBasicType *DebugTranslation::translateImpl(DIBasicTypeAttr attr) {
return llvm::DIBasicType::get(
llvmCtx, attr.getTag(), getMDStringOrNull(attr.getName()),
@@ -138,6 +149,17 @@ DebugTranslation::translateTemporaryImpl(DICompositeTypeAttr attr) {
/*VTableHolder=*/nullptr);
}
+llvm::TempDISubprogram
+DebugTranslation::translateTemporaryImpl(DISubprogramAttr attr) {
+ return llvm::DISubprogram::getTemporary(
+ llvmCtx, /*Scope=*/nullptr, /*Name=*/{}, /*LinkageName=*/{},
+ /*File=*/nullptr, attr.getLine(), /*Type=*/nullptr,
+ /*ScopeLine=*/0, /*ContainingType=*/nullptr, /*VirtualIndex=*/0,
+ /*ThisAdjustment=*/0, llvm::DINode::FlagZero,
+ static_cast<llvm::DISubprogram::DISPFlags>(attr.getSubprogramFlags()),
+ /*Unit=*/nullptr);
+}
+
llvm::DICompositeType *
DebugTranslation::translateImpl(DICompositeTypeAttr attr) {
// TODO: Use distinct attributes to model this, once they have landed.
@@ -151,10 +173,6 @@ DebugTranslation::translateImpl(DICompositeTypeAttr attr) {
isDistinct = true;
}
- SmallVector<llvm::Metadata *> elements;
- for (DINodeAttr member : attr.getElements())
- elements.push_back(translate(member));
-
return getDistinctOrUnique<llvm::DICompositeType>(
isDistinct, llvmCtx, attr.getTag(), getMDStringOrNull(attr.getName()),
translate(attr.getFile()), attr.getLine(), translate(attr.getScope()),
@@ -162,7 +180,7 @@ DebugTranslation::translateImpl(DICompositeTypeAttr attr) {
attr.getAlignInBits(),
/*OffsetInBits=*/0,
/*Flags=*/static_cast<llvm::DINode::DIFlags>(attr.getFlags()),
- llvm::MDNode::get(llvmCtx, elements),
+ getMDTupleOrNull(attr.getElements()),
/*RuntimeLang=*/0, /*VTableHolder=*/nullptr,
/*TemplateParams=*/nullptr, /*Identifier=*/nullptr,
/*Discriminator=*/nullptr,
@@ -242,22 +260,21 @@ DebugTranslation::translateImpl(DIGlobalVariableAttr attr) {
attr.getIsDefined(), nullptr, nullptr, attr.getAlignInBits(), nullptr);
}
-llvm::DIType *
+llvm::DINode *
DebugTranslation::translateRecursive(DIRecursiveTypeAttrInterface attr) {
DistinctAttr recursiveId = attr.getRecId();
- if (auto *iter = recursiveTypeMap.find(recursiveId);
- iter != recursiveTypeMap.end()) {
+ if (auto *iter = recursiveNodeMap.find(recursiveId);
+ iter != recursiveNodeMap.end()) {
return iter->second;
- } else {
- assert(!attr.isRecSelf() && "unbound DI recursive self type");
}
+ assert(!attr.isRecSelf() && "unbound DI recursive self reference");
- auto setRecursivePlaceholder = [&](llvm::DIType *placeholder) {
- recursiveTypeMap.try_emplace(recursiveId, placeholder);
+ auto setRecursivePlaceholder = [&](llvm::DINode *placeholder) {
+ recursiveNodeMap.try_emplace(recursiveId, placeholder);
};
- llvm::DIType *result =
- TypeSwitch<DIRecursiveTypeAttrInterface, llvm::DIType *>(attr)
+ llvm::DINode *result =
+ TypeSwitch<DIRecursiveTypeAttrInterface, llvm::DINode *>(attr)
.Case<DICompositeTypeAttr>([&](auto attr) {
auto temporary = translateTemporaryImpl(attr);
setRecursivePlaceholder(temporary.get());
@@ -266,11 +283,20 @@ DebugTranslation::translateRecursive(DIRecursiveTypeAttrInterface attr) {
auto *concrete = translateImpl(attr);
temporary->replaceAllUsesWith(concrete);
return concrete;
+ })
+ .Case<DISubprogramAttr>([&](auto attr) {
+ auto temporary = translateTemporaryImpl(attr);
+ setRecursivePlaceholder(temporary.get());
+ // Must call `translateImpl` directly instead of `translate` to
+ // avoid handling the recursive interface again.
+ auto *concrete = translateImpl(attr);
+ temporary->replaceAllUsesWith(concrete);
+ return concrete;
});
- assert(recursiveTypeMap.back().first == recursiveId &&
+ assert(recursiveNodeMap.back().first == recursiveId &&
"internal inconsistency: unexpected recursive translation stack");
- recursiveTypeMap.pop_back();
+ recursiveNodeMap.pop_back();
return result;
}
@@ -297,6 +323,7 @@ llvm::DISubprogram *DebugTranslation::translateImpl(DISubprogramAttr attr) {
bool isDefinition = static_cast<bool>(attr.getSubprogramFlags() &
LLVM::DISubprogramFlags::Definition);
+
llvm::DISubprogram *node = getDistinctOrUnique<llvm::DI...
[truncated]
|
|
@llvm/pr-subscribers-mlir Author: Tobias Gysi (gysit) ChangesThis commit implements LLVM_DIRecursiveTypeAttrInterface for the DISubprogramAttr to ensure cyclic subprograms can be imported properly. In the process multiple shortcuts around the recently introduced DIImportedEntityAttr can be removed. Patch is 35.66 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/106571.diff 12 Files Affected:
diff --git a/mlir/include/mlir-c/Dialect/LLVM.h b/mlir/include/mlir-c/Dialect/LLVM.h
index 5eb96a86e472d6..561d698f722afe 100644
--- a/mlir/include/mlir-c/Dialect/LLVM.h
+++ b/mlir/include/mlir-c/Dialect/LLVM.h
@@ -234,6 +234,9 @@ MLIR_CAPI_EXPORTED MlirAttribute mlirLLVMDIBasicTypeAttrGet(
MlirContext ctx, unsigned int tag, MlirAttribute name, uint64_t sizeInBits,
MlirLLVMTypeEncoding encoding);
+/// Creates a self-referencing LLVM DICompositeType attribute.
+MlirAttribute mlirLLVMDICompositeTypeAttrGetSelfRec(MlirAttribute recId);
+
/// Creates a LLVM DICompositeType attribute.
MLIR_CAPI_EXPORTED MlirAttribute mlirLLVMDICompositeTypeAttrGet(
MlirContext ctx, unsigned int tag, MlirAttribute recId, MlirAttribute name,
@@ -311,13 +314,16 @@ MLIR_CAPI_EXPORTED MlirAttribute mlirLLVMDILocalVariableAttrGet(
MlirAttribute diFile, unsigned int line, unsigned int arg,
unsigned int alignInBits, MlirAttribute diType, int64_t flags);
+/// Creates a self-referencing LLVM DISubprogramAttr attribute.
+MlirAttribute mlirLLVMDISubprogramAttrGetSelfRec(MlirAttribute recId);
+
/// Creates a LLVM DISubprogramAttr attribute.
MLIR_CAPI_EXPORTED MlirAttribute mlirLLVMDISubprogramAttrGet(
- MlirContext ctx, MlirAttribute id, MlirAttribute compileUnit,
- MlirAttribute scope, MlirAttribute name, MlirAttribute linkageName,
- MlirAttribute file, unsigned int line, unsigned int scopeLine,
- uint64_t subprogramFlags, MlirAttribute type, intptr_t nRetainedNodes,
- MlirAttribute const *retainedNodes);
+ MlirContext ctx, MlirAttribute id, MlirAttribute recId,
+ MlirAttribute compileUnit, MlirAttribute scope, MlirAttribute name,
+ MlirAttribute linkageName, MlirAttribute file, unsigned int line,
+ unsigned int scopeLine, uint64_t subprogramFlags, MlirAttribute type,
+ intptr_t nRetainedNodes, MlirAttribute const *retainedNodes);
/// Gets the scope from this DISubprogramAttr.
MLIR_CAPI_EXPORTED MlirAttribute
@@ -356,9 +362,9 @@ MLIR_CAPI_EXPORTED MlirAttribute mlirLLVMDIModuleAttrGet(
/// Creates a LLVM DIImportedEntityAttr attribute.
MLIR_CAPI_EXPORTED MlirAttribute mlirLLVMDIImportedEntityAttrGet(
- MlirContext ctx, unsigned int tag, MlirAttribute entity, MlirAttribute file,
- unsigned int line, MlirAttribute name, intptr_t nElements,
- MlirAttribute const *elements);
+ MlirContext ctx, unsigned int tag, MlirAttribute scope,
+ MlirAttribute entity, MlirAttribute file, unsigned int line,
+ MlirAttribute name, intptr_t nElements, MlirAttribute const *elements);
/// Gets the scope of this DIModuleAttr.
MLIR_CAPI_EXPORTED MlirAttribute
diff --git a/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td b/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td
index e57be7f760d380..224e58f3216abf 100644
--- a/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td
+++ b/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td
@@ -554,14 +554,16 @@ def LLVM_DILocalVariableAttr : LLVM_Attr<"DILocalVariable", "di_local_variable",
//===----------------------------------------------------------------------===//
def LLVM_DISubprogramAttr : LLVM_Attr<"DISubprogram", "di_subprogram",
- /*traits=*/[], "DIScopeAttr"> {
+ [LLVM_DIRecursiveTypeAttrInterface],
+ "DIScopeAttr"> {
let parameters = (ins
OptionalParameter<"DistinctAttr">:$id,
+ OptionalParameter<"DistinctAttr">:$recId,
OptionalParameter<"DICompileUnitAttr">:$compileUnit,
- "DIScopeAttr":$scope,
+ OptionalParameter<"DIScopeAttr">:$scope,
OptionalParameter<"StringAttr">:$name,
OptionalParameter<"StringAttr">:$linkageName,
- "DIFileAttr":$file,
+ OptionalParameter<"DIFileAttr">:$file,
OptionalParameter<"unsigned">:$line,
OptionalParameter<"unsigned">:$scopeLine,
OptionalParameter<"DISubprogramFlags">:$subprogramFlags,
@@ -577,13 +579,28 @@ def LLVM_DISubprogramAttr : LLVM_Attr<"DISubprogram", "di_subprogram",
"ArrayRef<DINodeAttr>":$retainedNodes
), [{
MLIRContext *ctx = file.getContext();
- return $_get(ctx, id, compileUnit, scope, StringAttr::get(ctx, name),
+ return $_get(ctx, id, /*recId=*/nullptr, compileUnit, scope,
+ StringAttr::get(ctx, name),
StringAttr::get(ctx, linkageName), file, line,
scopeLine, subprogramFlags, type, retainedNodes);
}]>
];
-
let assemblyFormat = "`<` struct(params) `>`";
+ let extraClassDeclaration = [{
+ /// Requirements of DIRecursiveTypeAttrInterface.
+ /// @{
+
+ /// Get whether this attr describes a recursive self reference.
+ bool isRecSelf() { return !getScope(); }
+
+ /// Get a copy of this type attr but with the recursive ID set to `recId`.
+ DIRecursiveTypeAttrInterface withRecId(DistinctAttr recId);
+
+ /// Build a rec-self instance using the provided `recId`.
+ static DIRecursiveTypeAttrInterface getRecSelf(DistinctAttr recId);
+
+ /// @}
+ }];
}
//===----------------------------------------------------------------------===//
@@ -627,13 +644,9 @@ def LLVM_DINamespaceAttr : LLVM_Attr<"DINamespace", "di_namespace",
def LLVM_DIImportedEntityAttr : LLVM_Attr<"DIImportedEntity", "di_imported_entity",
/*traits=*/[], "DINodeAttr"> {
- /// TODO: DIImportedEntity has a 'scope' field which represents the scope where
- /// this entity is imported. Currently, we are not adding a 'scope' field in
- /// DIImportedEntityAttr to avoid cyclic dependency. As DIImportedEntityAttr
- /// entries will be contained inside a scope entity (e.g. DISubprogramAttr),
- /// the scope can easily be inferred.
let parameters = (ins
LLVM_DITagParameter:$tag,
+ "DIScopeAttr":$scope,
"DINodeAttr":$entity,
OptionalParameter<"DIFileAttr">:$file,
OptionalParameter<"unsigned">:$line,
diff --git a/mlir/lib/CAPI/Dialect/LLVM.cpp b/mlir/lib/CAPI/Dialect/LLVM.cpp
index 13341f0c4de881..ed4d028e192f53 100644
--- a/mlir/lib/CAPI/Dialect/LLVM.cpp
+++ b/mlir/lib/CAPI/Dialect/LLVM.cpp
@@ -159,6 +159,11 @@ MlirAttribute mlirLLVMDIBasicTypeAttrGet(MlirContext ctx, unsigned int tag,
unwrap(ctx), tag, cast<StringAttr>(unwrap(name)), sizeInBits, encoding));
}
+MlirAttribute mlirLLVMDICompositeTypeAttrGetSelfRec(MlirAttribute recId) {
+ return wrap(
+ DICompositeTypeAttr::getRecSelf(cast<DistinctAttr>(unwrap(recId))));
+}
+
MlirAttribute mlirLLVMDICompositeTypeAttrGet(
MlirContext ctx, unsigned int tag, MlirAttribute recId, MlirAttribute name,
MlirAttribute file, uint32_t line, MlirAttribute scope,
@@ -289,16 +294,21 @@ MlirAttribute mlirLLVMDISubroutineTypeAttrGet(MlirContext ctx,
[](Attribute a) { return cast<DITypeAttr>(a); })));
}
+MlirAttribute mlirLLVMDISubprogramAttrGetSelfRec(MlirAttribute recId) {
+ return wrap(DISubprogramAttr::getRecSelf(cast<DistinctAttr>(unwrap(recId))));
+}
+
MlirAttribute mlirLLVMDISubprogramAttrGet(
- MlirContext ctx, MlirAttribute id, MlirAttribute compileUnit,
- MlirAttribute scope, MlirAttribute name, MlirAttribute linkageName,
- MlirAttribute file, unsigned int line, unsigned int scopeLine,
- uint64_t subprogramFlags, MlirAttribute type, intptr_t nRetainedNodes,
- MlirAttribute const *retainedNodes) {
+ MlirContext ctx, MlirAttribute id, MlirAttribute recId,
+ MlirAttribute compileUnit, MlirAttribute scope, MlirAttribute name,
+ MlirAttribute linkageName, MlirAttribute file, unsigned int line,
+ unsigned int scopeLine, uint64_t subprogramFlags, MlirAttribute type,
+ intptr_t nRetainedNodes, MlirAttribute const *retainedNodes) {
SmallVector<Attribute> nodesStorage;
nodesStorage.reserve(nRetainedNodes);
return wrap(DISubprogramAttr::get(
unwrap(ctx), cast<DistinctAttr>(unwrap(id)),
+ cast<DistinctAttr>(unwrap(recId)),
cast<DICompileUnitAttr>(unwrap(compileUnit)),
cast<DIScopeAttr>(unwrap(scope)), cast<StringAttr>(unwrap(name)),
cast<StringAttr>(unwrap(linkageName)), cast<DIFileAttr>(unwrap(file)),
@@ -353,14 +363,15 @@ MlirAttribute mlirLLVMDIModuleAttrGetScope(MlirAttribute diModule) {
}
MlirAttribute mlirLLVMDIImportedEntityAttrGet(
- MlirContext ctx, unsigned int tag, MlirAttribute entity, MlirAttribute file,
- unsigned int line, MlirAttribute name, intptr_t nElements,
- MlirAttribute const *elements) {
+ MlirContext ctx, unsigned int tag, MlirAttribute scope,
+ MlirAttribute entity, MlirAttribute file, unsigned int line,
+ MlirAttribute name, intptr_t nElements, MlirAttribute const *elements) {
SmallVector<Attribute> elementsStorage;
elementsStorage.reserve(nElements);
return wrap(DIImportedEntityAttr::get(
- unwrap(ctx), tag, cast<DINodeAttr>(unwrap(entity)),
- cast<DIFileAttr>(unwrap(file)), line, cast<StringAttr>(unwrap(name)),
+ unwrap(ctx), tag, cast<DIScopeAttr>(unwrap(scope)),
+ cast<DINodeAttr>(unwrap(entity)), cast<DIFileAttr>(unwrap(file)), line,
+ cast<StringAttr>(unwrap(name)),
llvm::map_to_vector(unwrapList(nElements, elements, elementsStorage),
[](Attribute a) { return cast<DINodeAttr>(a); })));
}
diff --git a/mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp b/mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp
index 98a9659735e7e6..0ce719b9a750ca 100644
--- a/mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp
+++ b/mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp
@@ -215,6 +215,22 @@ DICompositeTypeAttr::getRecSelf(DistinctAttr recId) {
{}, DIFlags(), 0, 0, {}, {}, {}, {}, {});
}
+//===----------------------------------------------------------------------===//
+// DISubprogramAttr
+//===----------------------------------------------------------------------===//
+
+DIRecursiveTypeAttrInterface DISubprogramAttr::withRecId(DistinctAttr recId) {
+ return DISubprogramAttr::get(
+ getContext(), getId(), recId, getCompileUnit(), getScope(), getName(),
+ getLinkageName(), getFile(), getLine(), getScopeLine(),
+ getSubprogramFlags(), getType(), getRetainedNodes());
+}
+
+DIRecursiveTypeAttrInterface DISubprogramAttr::getRecSelf(DistinctAttr recId) {
+ return DISubprogramAttr::get(recId.getContext(), {}, recId, {}, {}, {}, {}, 0,
+ 0, {}, {}, {}, {});
+}
+
//===----------------------------------------------------------------------===//
// TargetFeaturesAttr
//===----------------------------------------------------------------------===//
diff --git a/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp b/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
index 3870aab52f199d..6e4a964f1fc93c 100644
--- a/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
+++ b/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
@@ -3155,9 +3155,9 @@ struct LLVMOpAsmDialectInterface : public OpAsmDialectInterface {
.Case<AccessGroupAttr, AliasScopeAttr, AliasScopeDomainAttr,
DIBasicTypeAttr, DICompileUnitAttr, DICompositeTypeAttr,
DIDerivedTypeAttr, DIFileAttr, DIGlobalVariableAttr,
- DIGlobalVariableExpressionAttr, DILabelAttr, DILexicalBlockAttr,
- DILexicalBlockFileAttr, DILocalVariableAttr, DIModuleAttr,
- DINamespaceAttr, DINullTypeAttr, DIStringTypeAttr,
+ DIGlobalVariableExpressionAttr, DIImportedEntityAttr, DILabelAttr,
+ DILexicalBlockAttr, DILexicalBlockFileAttr, DILocalVariableAttr,
+ DIModuleAttr, DINamespaceAttr, DINullTypeAttr, DIStringTypeAttr,
DISubprogramAttr, DISubroutineTypeAttr, LoopAnnotationAttr,
LoopVectorizeAttr, LoopInterleaveAttr, LoopUnrollAttr,
LoopUnrollAndJamAttr, LoopLICMAttr, LoopDistributeAttr,
diff --git a/mlir/lib/Dialect/LLVMIR/Transforms/DIScopeForLLVMFuncOp.cpp b/mlir/lib/Dialect/LLVMIR/Transforms/DIScopeForLLVMFuncOp.cpp
index c75f44bf3976a9..ad9aa7ac80cafa 100644
--- a/mlir/lib/Dialect/LLVMIR/Transforms/DIScopeForLLVMFuncOp.cpp
+++ b/mlir/lib/Dialect/LLVMIR/Transforms/DIScopeForLLVMFuncOp.cpp
@@ -76,8 +76,8 @@ static void addScopeToFunction(LLVM::LLVMFuncOp llvmFunc,
compileUnitAttr = {};
}
auto subprogramAttr = LLVM::DISubprogramAttr::get(
- context, id, compileUnitAttr, fileAttr, funcNameAttr, funcNameAttr,
- fileAttr,
+ context, id, /*recId=*/{}, compileUnitAttr, fileAttr, funcNameAttr,
+ funcNameAttr, fileAttr,
/*line=*/line,
/*scopeline=*/col, subprogramFlags, subroutineTypeAttr,
/*retainedNodes=*/{});
diff --git a/mlir/lib/Target/LLVMIR/DebugImporter.cpp b/mlir/lib/Target/LLVMIR/DebugImporter.cpp
index ce3643f513d34a..509451057dd737 100644
--- a/mlir/lib/Target/LLVMIR/DebugImporter.cpp
+++ b/mlir/lib/Target/LLVMIR/DebugImporter.cpp
@@ -217,8 +217,8 @@ DebugImporter::translateImpl(llvm::DIImportedEntity *node) {
}
return DIImportedEntityAttr::get(
- context, node->getTag(), translate(node->getEntity()),
- translate(node->getFile()), node->getLine(),
+ context, node->getTag(), translate(node->getScope()),
+ translate(node->getEntity()), translate(node->getFile()), node->getLine(),
getStringAttrOrNull(node->getRawName()), elements);
}
@@ -227,6 +227,7 @@ DISubprogramAttr DebugImporter::translateImpl(llvm::DISubprogram *node) {
mlir::DistinctAttr id;
if (node->isDistinct())
id = getOrCreateDistinctID(node);
+
// Return nullptr if the scope or type is invalid.
DIScopeAttr scope = translate(node->getScope());
if (node->getScope() && !scope)
@@ -238,16 +239,19 @@ DISubprogramAttr DebugImporter::translateImpl(llvm::DISubprogram *node) {
if (node->getType() && !type)
return nullptr;
+ // Convert the retained nodes but drop all of them if one of them is invalid.
SmallVector<DINodeAttr> retainedNodes;
for (llvm::DINode *retainedNode : node->getRetainedNodes())
retainedNodes.push_back(translate(retainedNode));
+ if (llvm::is_contained(retainedNodes, nullptr))
+ retainedNodes.clear();
- return DISubprogramAttr::get(context, id, translate(node->getUnit()), scope,
- getStringAttrOrNull(node->getRawName()),
- getStringAttrOrNull(node->getRawLinkageName()),
- translate(node->getFile()), node->getLine(),
- node->getScopeLine(), *subprogramFlags, type,
- retainedNodes);
+ return DISubprogramAttr::get(
+ context, id, /*recId=*/{}, translate(node->getUnit()), scope,
+ getStringAttrOrNull(node->getRawName()),
+ getStringAttrOrNull(node->getRawLinkageName()),
+ translate(node->getFile()), node->getLine(), node->getScopeLine(),
+ *subprogramFlags, type, retainedNodes);
}
DISubrangeAttr DebugImporter::translateImpl(llvm::DISubrange *node) {
@@ -374,6 +378,9 @@ getRecSelfConstructor(llvm::DINode *node) {
.Case([&](llvm::DICompositeType *) {
return CtorType(DICompositeTypeAttr::getRecSelf);
})
+ .Case([&](llvm::DISubprogram *) {
+ return CtorType(DISubprogramAttr::getRecSelf);
+ })
.Default(CtorType());
}
diff --git a/mlir/lib/Target/LLVMIR/DebugTranslation.cpp b/mlir/lib/Target/LLVMIR/DebugTranslation.cpp
index 042e015f107fea..55060659dd3560 100644
--- a/mlir/lib/Target/LLVMIR/DebugTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/DebugTranslation.cpp
@@ -96,6 +96,17 @@ llvm::MDString *DebugTranslation::getMDStringOrNull(StringAttr stringAttr) {
return llvm::MDString::get(llvmCtx, stringAttr);
}
+llvm::MDTuple *
+DebugTranslation::getMDTupleOrNull(ArrayRef<DINodeAttr> elements) {
+ if (elements.empty())
+ return nullptr;
+ SmallVector<llvm::Metadata *> llvmElements = llvm::to_vector(
+ llvm::map_range(elements, [&](DINodeAttr attr) -> llvm::Metadata * {
+ return translate(attr);
+ }));
+ return llvm::MDNode::get(llvmCtx, llvmElements);
+}
+
llvm::DIBasicType *DebugTranslation::translateImpl(DIBasicTypeAttr attr) {
return llvm::DIBasicType::get(
llvmCtx, attr.getTag(), getMDStringOrNull(attr.getName()),
@@ -138,6 +149,17 @@ DebugTranslation::translateTemporaryImpl(DICompositeTypeAttr attr) {
/*VTableHolder=*/nullptr);
}
+llvm::TempDISubprogram
+DebugTranslation::translateTemporaryImpl(DISubprogramAttr attr) {
+ return llvm::DISubprogram::getTemporary(
+ llvmCtx, /*Scope=*/nullptr, /*Name=*/{}, /*LinkageName=*/{},
+ /*File=*/nullptr, attr.getLine(), /*Type=*/nullptr,
+ /*ScopeLine=*/0, /*ContainingType=*/nullptr, /*VirtualIndex=*/0,
+ /*ThisAdjustment=*/0, llvm::DINode::FlagZero,
+ static_cast<llvm::DISubprogram::DISPFlags>(attr.getSubprogramFlags()),
+ /*Unit=*/nullptr);
+}
+
llvm::DICompositeType *
DebugTranslation::translateImpl(DICompositeTypeAttr attr) {
// TODO: Use distinct attributes to model this, once they have landed.
@@ -151,10 +173,6 @@ DebugTranslation::translateImpl(DICompositeTypeAttr attr) {
isDistinct = true;
}
- SmallVector<llvm::Metadata *> elements;
- for (DINodeAttr member : attr.getElements())
- elements.push_back(translate(member));
-
return getDistinctOrUnique<llvm::DICompositeType>(
isDistinct, llvmCtx, attr.getTag(), getMDStringOrNull(attr.getName()),
translate(attr.getFile()), attr.getLine(), translate(attr.getScope()),
@@ -162,7 +180,7 @@ DebugTranslation::translateImpl(DICompositeTypeAttr attr) {
attr.getAlignInBits(),
/*OffsetInBits=*/0,
/*Flags=*/static_cast<llvm::DINode::DIFlags>(attr.getFlags()),
- llvm::MDNode::get(llvmCtx, elements),
+ getMDTupleOrNull(attr.getElements()),
/*RuntimeLang=*/0, /*VTableHolder=*/nullptr,
/*TemplateParams=*/nullptr, /*Identifier=*/nullptr,
/*Discriminator=*/nullptr,
@@ -242,22 +260,21 @@ DebugTranslation::translateImpl(DIGlobalVariableAttr attr) {
attr.getIsDefined(), nullptr, nullptr, attr.getAlignInBits(), nullptr);
}
-llvm::DIType *
+llvm::DINode *
DebugTranslation::translateRecursive(DIRecursiveTypeAttrInterface attr) {
DistinctAttr recursiveId = attr.getRecId();
- if (auto *iter = recursiveTypeMap.find(recursiveId);
- iter != recursiveTypeMap.end()) {
+ if (auto *iter = recursiveNodeMap.find(recursiveId);
+ iter != recursiveNodeMap.end()) {
return iter->second;
- } else {
- assert(!attr.isRecSelf() && "unbound DI recursive self type");
}
+ assert(!attr.isRecSelf() && "unbound DI recursive self reference");
- auto setRecursivePlaceholder = [&](llvm::DIType *placeholder) {
- recursiveTypeMap.try_emplace(recursiveId, placeholder);
+ auto setRecursivePlaceholder = [&](llvm::DINode *placeholder) {
+ recursiveNodeMap.try_emplace(recursiveId, placeholder);
};
- llvm::DIType *result =
- TypeSwitch<DIRecursiveTypeAttrInterface, llvm::DIType *>(attr)
+ llvm::DINode *result =
+ TypeSwitch<DIRecursiveTypeAttrInterface, llvm::DINode *>(attr)
.Case<DICompositeTypeAttr>([&](auto attr) {
auto temporary = translateTemporaryImpl(attr);
setRecursivePlaceholder(temporary.get());
@@ -266,11 +283,20 @@ DebugTranslation::translateRecursive(DIRecursiveTypeAttrInterface attr) {
auto *concrete = translateImpl(attr);
temporary->replaceAllUsesWith(concrete);
return concrete;
+ })
+ .Case<DISubprogramAttr>([&](auto attr) {
+ auto temporary = translateTemporaryImpl(attr);
+ setRecursivePlaceholder(temporary.get());
+ // Must call `translateImpl` directly instead of `translate` to
+ // avoid handling the recursive interface again.
+ auto *concrete = translateImpl(attr);
+ temporary->replaceAllUsesWith(concrete);
+ return concrete;
});
- assert(recursiveTypeMap.back().first == recursiveId &&
+ assert(recursiveNodeMap.back().first == recursiveId &&
"internal inconsistency: unexpected recursive translation stack");
- recursiveTypeMap.pop_back();
+ recursiveNodeMap.pop_back();
return result;
}
@@ -297,6 +323,7 @@ llvm::DISubprogram *DebugTranslation::translateImpl(DISubprogramAttr attr) {
bool isDefinition = static_cast<bool>(attr.getSubprogramFlags() &
LLVM::DISubprogramFlags::Definition);
+
llvm::DISubprogram *node = getDistinctOrUnique<llvm::DI...
[truncated]
|
|
@abidh this tries to address the import issue that came up after #103055 by supporting cyclic subprograms. @zyx-billy would be nice if you can have a look since I may have missed something. |
zyx-billy
reviewed
Aug 29, 2024
| ; CHECK-DAG: #[[SP_OUTER:.+]] = #llvm.di_subprogram<id = [[SP_ID]], compileUnit = #{{.*}}, scope = #[[COMP]], name = "class_method", file = #{{.*}}, subprogramFlags = Definition, type = #[[SP_TYPE_OUTER]]> | ||
| ; CHECK-DAG: #[[LOC]] = loc(fused<#[[SP_OUTER]]> | ||
| ; CHECK-DAG: #[[SP:.+]] = #llvm.di_subprogram<id = [[SP_ID:.+]], recId = [[REC_ID]], compileUnit = #{{.*}}, scope = #[[COMP]], name = "class_method", file = #{{.*}}, subprogramFlags = Definition, type = #[[SP_TYPE]]> | ||
| ; CHECK-DAG: #[[LOC]] = loc(fused<#[[SP]]> |
There was a problem hiding this comment.
oh nice! I guess this should also result in smaller translated mlir sizes in general, especially if previously there were many cycles that began with subprograms?
There was a problem hiding this comment.
Yeah I would expect this further reduces ir size!
| /// @{ | ||
|
|
||
| /// Get whether this attr describes a recursive self reference. | ||
| bool isRecSelf() { return !getScope(); } |
There was a problem hiding this comment.
My only slight concern is if it's possible to see a DISubprogram without a scope naturally (it looks like it's allowed to be null here but I'm not sure if it's illegal/impractical elsewhere...). If it's not supposed to happen, can we enforce during import that all llvm's DISubprograms have a scope?
The concern is if we categorize a node as RecSelf when it isn't, we won't be able to tell the outer real node from the inner recursive reference (which triggers an assertion during export).
There was a problem hiding this comment.
Alternatively we could maybe add a tag parameter to the enum similar to the one we have on DICompositeType? It is a bit redundant though since a subprogram is a subprogram. So maybe a separate boolean makes sense.
It is indeed true that using the scope feels a bit hacky.
There was a problem hiding this comment.
Yeah looking back it was pure luck we had a tag in DICompositeTypeAttr for this 😂 I guess adding an enum tag or a bool flag probably works the same. A tag certainly increases the textual IR a lot... Or, worst case we just make recId itself a dedicated attr with DistinctAttr + bool (may be handy if we need it for more nodes later).
Thanks for doing this. The |
This commit implements LLVM_DIRecursiveTypeAttrInterface for the DISubprogramAttr to ensure cyclic subprograms can be imported properly. In the process multiple shortcuts around the recently introduced DIImportedEntityAttr can be removed.
gysit
force-pushed
the
make-subprogram-recursive
branch
from
fad0bda to
988d78e
Compare
llvmbot
added
flang
|
I went for a boolean flag but put everything at the beginning of the recursive attributes. I tried to fix flang but building it locally turns out to be difficult due to some dependencies. I may have to follow up next week... |
zyx-billy
approved these changes
Aug 30, 2024
| getContext(), getId(), recId, getCompileUnit(), getScope(), getName(), | ||
| getLinkageName(), getFile(), getLine(), getScopeLine(), | ||
| getSubprogramFlags(), getType(), getRetainedNodes()); | ||
| getContext(), recId, /*isRecSelf=*/false, getId(), getCompileUnit(), |
There was a problem hiding this comment.
minor nit, I think it's safer to also getIsRecSelf() here just in case some place called this on an existing rec-self instance to change the ID (technically they should just use getRecSelf but we don't check).
There was a problem hiding this comment.
Ah right. Makes sense!
| @@ -448,7 +448,7 @@ llvm.func @func_debug_directives() { | |||
| #di_compile_unit = #llvm.di_compile_unit<id = distinct[1]<>, sourceLanguage = DW_LANG_C, file = #di_file, isOptimized = false, emissionKind = None> | |||
|
|
|||
| // Recursive type itself. | |||
| #di_struct_self = #llvm.di_composite_type<tag = DW_TAG_null, recId = distinct[0]<>> | |||
| #di_struct_self = #llvm.di_composite_type<recId = distinct[0]<>, isRecSelf = false> | |||
There was a problem hiding this comment.
Did you mean to set it to true instead? I guess we didn't really check in the rec-self case, but might be good to set it so it's not confusing.
There was a problem hiding this comment.
That was a classic search replace thingy just saw there are more such instances. Will fix them.
gysit
force-pushed
the
make-subprogram-recursive
branch
from
af9b00b to
d22b24d
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/138/builds/2992 Here is the relevant piece of the build log for the reference |
gysit
added a commit
that referenced
this pull request
Aug 31, 2024
Reverts #106571 This commit breaks the following build bot: https://lab.llvm.org/buildbot/#/builders/138/builds/2992 It looks like there is a missing dependency in this particular setup.
|
The reason for the failure seems to be a missing MLIR_CAPI_EXPORTED prefix for the newly added functions. |
gysit
added a commit
to gysit/llvm-project
that referenced
this pull request
Sep 2, 2024
…h fixes This reverts commit fa93be4, restoring commit d884b77, with fixes that ensure the CAPI declarations are exported properly. This commit implements LLVM_DIRecursiveTypeAttrInterface for the DISubprogramAttr to ensure cyclic subprograms can be imported properly. In the process multiple shortcuts around the recently introduced DIImportedEntityAttr can be removed.
gysit
added a commit
that referenced
this pull request
Sep 2, 2024
…xes (#106947) This reverts commit fa93be4, restoring commit d884b77, with fixes that ensure the CAPI declarations are exported properly. This commit implements LLVM_DIRecursiveTypeAttrInterface for the DISubprogramAttr to ensure cyclic subprograms can be imported properly. In the process multiple shortcuts around the recently introduced DIImportedEntityAttr can be removed.
nirvedhmeshram
added a commit
to iree-org/iree
that referenced
this pull request
Sep 12, 2024
Bumps llvm to commit: iree-org/llvm-project@e268afb in branch https://github.com/iree-org/llvm-project/tree/shared/integrates_20240910 Following changes are made: 1. Fix formatv call to pass validation added by llvm/llvm-project#105745 2. API changes in DICompositeTypeAttr::get introduced by llvm/llvm-project#106571 3. Fix API call from llvm/llvm-project#100361 4. Fix chipset comparison in ROCMTarget.cpp There are two cherry-picks from upstream main as they contain fixes we need and one cheery-pick that is yet to land 1. iree-org/llvm-project@0d5d355 2. iree-org/llvm-project@650d852 3. iree-org/llvm-project@e268afb (the upstream PR for this one is llvm/llvm-project#108302 And a revert due to an outstanding torch-mlir issue iree-org/llvm-project@cf22797
josemonsalve2
pushed a commit
to josemonsalve2/iree
that referenced
this pull request
Sep 14, 2024
Bumps llvm to commit: iree-org/llvm-project@e268afb in branch https://github.com/iree-org/llvm-project/tree/shared/integrates_20240910 Following changes are made: 1. Fix formatv call to pass validation added by llvm/llvm-project#105745 2. API changes in DICompositeTypeAttr::get introduced by llvm/llvm-project#106571 3. Fix API call from llvm/llvm-project#100361 4. Fix chipset comparison in ROCMTarget.cpp There are two cherry-picks from upstream main as they contain fixes we need and one cheery-pick that is yet to land 1. iree-org/llvm-project@0d5d355 2. iree-org/llvm-project@650d852 3. iree-org/llvm-project@e268afb (the upstream PR for this one is llvm/llvm-project#108302 And a revert due to an outstanding torch-mlir issue iree-org/llvm-project@cf22797
|
Hi @gysit, @zyx-billy If
The But if I was wondering if this is a known limitation or is there a workaround this problem? |
|
If you have an LLVM IR example, it could make to run it through the LLVM IR to LLVM dialect import and see what happens (using @zyx-billy may have something like that working in their front-end? |
|
Hi @abidh, IIUC are you talking about the case of mutual recursion? The way it works is we never "expose" attributes that contain unbound self-references. In your case, the debug importer is supposed to create two results (consisting of 6 distinct attributes): Where We have some test cases starting here that might help explain how things would look (including some mutual recursions), but like @gysit says, feel free to let us know if you're seeing something that doesn't work properly though! 🙏 |
|
Hi @zyx-billy Thanks for you comment. Can you kindly explain what the relationship of My frontend was generating something like It worked if the |
|
Hi @abidh, In my example, When you say your frontend was generating those attributes, do you mean that's what they look like after importing into the llvm dialect? As long as your |
The frontend here is
I guess the problem is that |
|
Oh so the attributes aren't generated by importing, but they were created natively in MLIR? In that case I think your frontend will need to make sure to not "reuse" the |
abidh
added a commit
to abidh/llvm-project
that referenced
this pull request
Sep 25, 2024
This commit fixes an issue that can cause an assertion to fail in some circumstances at https://github.com/llvm/llvm-project/blob/26029d77a57cb4aaa1479064109e985a90d0edd8/mlir/lib/Target/LLVMIR/DebugTranslation.cpp#L270. The issue relates to the handling of recursive debug type in mlir. See the discussion at the end of follow PR for more details. llvm#106571 Problem could be explained with the following example code: type t2 type(t1), pointer :: p1 end type type t1 type(t2), pointer :: p2 end type In the description below, type_self means a temporary type that is generated as a place holder while the members of that type are being processed. If we process t1 first then we will have the following structure after it has been processed. t1 -> t2 -> t1_self This is because when we started processing t2, we did not have the complete t1 but its place holder t1_self. Now if some entity requires t2, we will already have that in cache and will return it. But this t2 refers to t1_self and not to t1. In mlir handling, only those types are allowed to have _self reference which are wrapped by entity whose reference it contains. So t1 -> t2 -> t1_self is ok because the t1_self reference can be resolved by the outer t1. But standalone t2 is not because there will be no way to resolve it. Please see DebugTranslation::translateRecursive for details on how mlir handles recursive types. The fix is not to cache the type that will fail if used standalone.
abidh
added a commit
to abidh/llvm-project
that referenced
this pull request
Jan 13, 2025
When RecordType is converted to corresponding DIType, we cache the information to avoid doing the conversion again. Our conversion of RecordType looks like this: ConvertRecordType(RecordType Ty) 1. If type Ty is already in the cache, then return the corresponding item. 2. Create a place holder DICompositeTypeAttr (called ty_self below) for Ty 3. Put Ty->ty_self in the cache 4. Convert members of Ty. This may cause ConvertRecordType to be called agin with other types. 5. Create final DICompositeTypeAttr 6. Replace the ty_self in the cache with one created in step 5 end The purpose of creating ty_self is to handle cases where a member may have reference to parent type. Now consider the code below: type t1 type(t2), pointer :: p1 end type type t2 type(t1), pointer :: p2 end type While processing t1, we could have a structure like below. t1 -> t2 -> t1_self The t2 created during handling of t1 cant be cached on its own as it contains a place holder reference. It will fail an assert in MLIR if it is processed standalone. We previously had a check in the step 6 above to not cache it. But this check was not tight enough. It just checked if a type should not have a place holder reference to another type. It missed the following case where the place holder reference can be in a type further down the line. type t1 type(t2), pointer :: p1 end type type t2 type(t3), pointer :: p2 end type type t3 type(t1), pointer :: p3 end type So while processing t1, we have to stop caching of not only t3 but also of t2. This PR improves the check and moves the logic inside convertRecordType. Please note that this limitation of why a type cant have a placeholder reference is because of how such references are resolved in the mlir. Please see the discussion at the end of the following PR. llvm#106571 Fixes llvm#122024.
abidh
added a commit
that referenced
this pull request
Jan 20, 2025
When `RecordType` is converted to corresponding `DIType`, we cache the information to avoid doing the conversion again. Our conversion of `RecordType` looks like this: `ConvertRecordType(RecordType Ty)` 1. If type `Ty` is already in the cache, then return the corresponding item. 2. Create a place holder `DICompositeTypeAttr` (called `ty_self` below) for `Ty` 3. Put `Ty->ty_self` in the cache 4. Convert members of `Ty`. This may cause `ConvertRecordType` to be called again with other types. 5. Create final `DICompositeTypeAttr` 6. Replace the `ty_self` in the cache with one created in step 5 end The purpose of creating `ty_self` is to handle cases where a member may have reference to parent type. Now consider the code below: ``` type t1 type(t2), pointer :: p1 end type type t2 type(t1), pointer :: p2 end type ``` While processing t1, we could have a structure like below. `t1 -> t2 -> t1_self` The `t2` created during handling of `t1` cant be cached on its own as it contains a place holder reference. It will fail an assert in MLIR if it is processed standalone. To avoid this problem, we have a check in the step 6 above to not cache such types. But this check was not tight enough. It just checked if a type should not have a place holder reference to another type. It missed the following case where the place holder reference can be in a type further down the line. ``` type t1 type(t2), pointer :: p1 end type type t2 type(t3), pointer :: p2 end type type t3 type(t1), pointer :: p3 end type ``` So while processing `t1`, we have to stop caching of not only `t3` but also of `t2`. This PR improves the check and moves the logic inside `convertRecordType`. Please note that this limitation of why a type cant have a placeholder reference is because of how such references are resolved in the mlir. Please see the discussion at the end of this [PR](#106571). I have to change `getDerivedType` so that it will also get the derived type for things like `type(t2), pointer :: p1` which are wrapped in `BoxType`. Happy to move it to a new function or a local helper in case this change is problematic. Fixes #122024.
github-actions bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Jan 20, 2025
…#122770) When `RecordType` is converted to corresponding `DIType`, we cache the information to avoid doing the conversion again. Our conversion of `RecordType` looks like this: `ConvertRecordType(RecordType Ty)` 1. If type `Ty` is already in the cache, then return the corresponding item. 2. Create a place holder `DICompositeTypeAttr` (called `ty_self` below) for `Ty` 3. Put `Ty->ty_self` in the cache 4. Convert members of `Ty`. This may cause `ConvertRecordType` to be called again with other types. 5. Create final `DICompositeTypeAttr` 6. Replace the `ty_self` in the cache with one created in step 5 end The purpose of creating `ty_self` is to handle cases where a member may have reference to parent type. Now consider the code below: ``` type t1 type(t2), pointer :: p1 end type type t2 type(t1), pointer :: p2 end type ``` While processing t1, we could have a structure like below. `t1 -> t2 -> t1_self` The `t2` created during handling of `t1` cant be cached on its own as it contains a place holder reference. It will fail an assert in MLIR if it is processed standalone. To avoid this problem, we have a check in the step 6 above to not cache such types. But this check was not tight enough. It just checked if a type should not have a place holder reference to another type. It missed the following case where the place holder reference can be in a type further down the line. ``` type t1 type(t2), pointer :: p1 end type type t2 type(t3), pointer :: p2 end type type t3 type(t1), pointer :: p3 end type ``` So while processing `t1`, we have to stop caching of not only `t3` but also of `t2`. This PR improves the check and moves the logic inside `convertRecordType`. Please note that this limitation of why a type cant have a placeholder reference is because of how such references are resolved in the mlir. Please see the discussion at the end of this [PR](llvm/llvm-project#106571). I have to change `getDerivedType` so that it will also get the derived type for things like `type(t2), pointer :: p1` which are wrapped in `BoxType`. Happy to move it to a new function or a local helper in case this change is problematic. Fixes #122024.
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 commit implements LLVM_DIRecursiveTypeAttrInterface for the DISubprogramAttr to ensure cyclic subprograms can be imported properly. In the process multiple shortcuts around the recently introduced DIImportedEntityAttr can be removed.