[mlir][debuginfo] Add support for subprogram annotations #110946
Conversation
|
@llvm/pr-subscribers-flang-fir-hlfir @llvm/pr-subscribers-mlir-llvm Author: Walter Erquinigo (walter-erquinigo) ChangesLLVM already supports Full diff: https://github.com/llvm/llvm-project/pull/110946.diff 4 Files Affected:
diff --git a/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td b/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td
index 2da45eba77655b..dc711232aa9be8 100644
--- a/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td
+++ b/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td
@@ -586,7 +586,8 @@ def LLVM_DISubprogramAttr : LLVM_Attr<"DISubprogram", "di_subprogram",
OptionalParameter<"unsigned">:$scopeLine,
OptionalParameter<"DISubprogramFlags">:$subprogramFlags,
OptionalParameter<"DISubroutineTypeAttr">:$type,
- OptionalArrayRefParameter<"DINodeAttr">:$retainedNodes
+ OptionalArrayRefParameter<"DINodeAttr">:$retainedNodes,
+ OptionalArrayRefParameter<"DINodeAttr">:$annotations
);
let builders = [
AttrBuilder<(ins
@@ -594,11 +595,11 @@ def LLVM_DISubprogramAttr : LLVM_Attr<"DISubprogram", "di_subprogram",
"DIScopeAttr":$scope, "StringAttr":$name, "StringAttr":$linkageName,
"DIFileAttr":$file, "unsigned":$line, "unsigned":$scopeLine,
"DISubprogramFlags":$subprogramFlags, "DISubroutineTypeAttr":$type,
- "ArrayRef<DINodeAttr>":$retainedNodes
+ "ArrayRef<DINodeAttr>":$retainedNodes, "ArrayRef<DINodeAttr>":$annotations
), [{
return $_get($_ctxt, /*recId=*/nullptr, /*isRecSelf=*/false, id, compileUnit,
scope, name, linkageName, file, line, scopeLine,
- subprogramFlags, type, retainedNodes);
+ subprogramFlags, type, retainedNodes, annotations);
}]>
];
let assemblyFormat = "`<` struct(params) `>`";
@@ -670,6 +671,21 @@ def LLVM_DIImportedEntityAttr : LLVM_Attr<"DIImportedEntity", "di_imported_entit
let assemblyFormat = "`<` struct(params) `>`";
}
+//===----------------------------------------------------------------------===//
+// DIStringAnnotationAttr
+//===----------------------------------------------------------------------===//
+
+def LLVM_DIStringAnnotationAttr : LLVM_Attr<"DIStringAnnotation",
+ "di_string_annotation",
+ /*traits=*/[], "DINodeAttr"> {
+ let parameters = (ins
+ "StringAttr":$name,
+ "StringAttr":$value
+ );
+
+ let assemblyFormat = "`<` struct(params) `>`";
+}
+
//===----------------------------------------------------------------------===//
// DISubrangeAttr
//===----------------------------------------------------------------------===//
diff --git a/mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp b/mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp
index bd2164e640e7b8..a4304ef6668e5e 100644
--- a/mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp
+++ b/mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp
@@ -60,9 +60,9 @@ bool DINodeAttr::classof(Attribute attr) {
DIDerivedTypeAttr, DIFileAttr, DIGlobalVariableAttr,
DIImportedEntityAttr, DILabelAttr, DILexicalBlockAttr,
DILexicalBlockFileAttr, DILocalVariableAttr, DIModuleAttr,
- DINamespaceAttr, DINullTypeAttr, DIStringTypeAttr,
- DISubprogramAttr, DISubrangeAttr, DISubroutineTypeAttr>(
- attr);
+ DINamespaceAttr, DINullTypeAttr, DIStringAnnotationAttr,
+ DIStringTypeAttr, DISubprogramAttr, DISubrangeAttr,
+ DISubroutineTypeAttr>(attr);
}
//===----------------------------------------------------------------------===//
@@ -221,15 +221,16 @@ DICompositeTypeAttr::getRecSelf(DistinctAttr recId) {
//===----------------------------------------------------------------------===//
DIRecursiveTypeAttrInterface DISubprogramAttr::withRecId(DistinctAttr recId) {
- return DISubprogramAttr::get(
- getContext(), recId, getIsRecSelf(), getId(), getCompileUnit(),
- getScope(), getName(), getLinkageName(), getFile(), getLine(),
- getScopeLine(), getSubprogramFlags(), getType(), getRetainedNodes());
+ return DISubprogramAttr::get(getContext(), recId, getIsRecSelf(), getId(),
+ getCompileUnit(), getScope(), getName(),
+ getLinkageName(), getFile(), getLine(),
+ getScopeLine(), getSubprogramFlags(), getType(),
+ getRetainedNodes(), getAnnotations());
}
DIRecursiveTypeAttrInterface DISubprogramAttr::getRecSelf(DistinctAttr recId) {
return DISubprogramAttr::get(recId.getContext(), recId, /*isRecSelf=*/true,
- {}, {}, {}, {}, {}, 0, 0, {}, {}, {}, {});
+ {}, {}, {}, {}, {}, 0, 0, {}, {}, {}, {}, {});
}
//===----------------------------------------------------------------------===//
diff --git a/mlir/lib/Dialect/LLVMIR/Transforms/DIScopeForLLVMFuncOp.cpp b/mlir/lib/Dialect/LLVMIR/Transforms/DIScopeForLLVMFuncOp.cpp
index 2cfaffa7c8efce..59a4f8e9023582 100644
--- a/mlir/lib/Dialect/LLVMIR/Transforms/DIScopeForLLVMFuncOp.cpp
+++ b/mlir/lib/Dialect/LLVMIR/Transforms/DIScopeForLLVMFuncOp.cpp
@@ -78,7 +78,7 @@ static void addScopeToFunction(LLVM::LLVMFuncOp llvmFunc,
auto subprogramAttr = LLVM::DISubprogramAttr::get(
context, id, compileUnitAttr, fileAttr, funcName, funcName, fileAttr,
/*line=*/line, /*scopeline=*/col, subprogramFlags, subroutineTypeAttr,
- /*retainedNodes=*/{});
+ /*retainedNodes=*/{}, /*annotations=*/{});
llvmFunc->setLoc(FusedLoc::get(context, {loc}, subprogramAttr));
}
diff --git a/mlir/lib/Target/LLVMIR/DebugTranslation.cpp b/mlir/lib/Target/LLVMIR/DebugTranslation.cpp
index 8ca3beca6b66f7..5b7b28a761ec83 100644
--- a/mlir/lib/Target/LLVMIR/DebugTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/DebugTranslation.cpp
@@ -102,6 +102,13 @@ DebugTranslation::getMDTupleOrNull(ArrayRef<DINodeAttr> elements) {
return nullptr;
SmallVector<llvm::Metadata *> llvmElements = llvm::to_vector(
llvm::map_range(elements, [&](DINodeAttr attr) -> llvm::Metadata * {
+ if (DIStringAnnotationAttr annAttr =
+ dyn_cast<DIStringAnnotationAttr>(attr)) {
+ llvm::Metadata *ops[2] = {
+ llvm::MDString::get(llvmCtx, annAttr.getName()),
+ llvm::MDString::get(llvmCtx, annAttr.getValue())};
+ return llvm::MDNode::get(llvmCtx, ops);
+ }
return translate(attr);
}));
return llvm::MDNode::get(llvmCtx, llvmElements);
@@ -332,7 +339,8 @@ llvm::DISubprogram *DebugTranslation::translateImpl(DISubprogramAttr attr) {
/*ThisAdjustment=*/0, llvm::DINode::FlagZero,
static_cast<llvm::DISubprogram::DISPFlags>(attr.getSubprogramFlags()),
compileUnit, /*TemplateParams=*/nullptr, /*Declaration=*/nullptr,
- getMDTupleOrNull(attr.getRetainedNodes()));
+ getMDTupleOrNull(attr.getRetainedNodes()), nullptr,
+ getMDTupleOrNull(attr.getAnnotations()));
if (attr.getId())
distinctAttrToNode.try_emplace(attr.getId(), node);
return node;
@@ -361,6 +369,16 @@ DebugTranslation::translateImpl(DIImportedEntityAttr attr) {
getMDStringOrNull(attr.getName()), getMDTupleOrNull(attr.getElements()));
}
+/*
+llvm::DINodeArray *
+DebugTranslation::translateImpl(DIStringAnnotationAttr attr) {
+ llvm::DINodeArray arr;
+ arr->push_back(llvm::MDString::get(llvmCtx, attr.getName()));
+ arr->push_back(llvm::MDString::get(llvmCtx, attr.getValue()));
+ return arr;
+}
+*/
+
llvm::DISubrange *DebugTranslation::translateImpl(DISubrangeAttr attr) {
auto getMetadataOrNull = [&](Attribute attr) -> llvm::Metadata * {
if (!attr)
@@ -426,8 +444,9 @@ llvm::DINode *DebugTranslation::translate(DINodeAttr attr) {
DIImportedEntityAttr, DILabelAttr, DILexicalBlockAttr,
DILexicalBlockFileAttr, DILocalVariableAttr, DIModuleAttr,
DINamespaceAttr, DINullTypeAttr, DIStringTypeAttr,
- DISubprogramAttr, DISubrangeAttr, DISubroutineTypeAttr>(
- [&](auto attr) { return translateImpl(attr); });
+ DISubprogramAttr, DISubrangeAttr, DISubroutineTypeAttr
+ // DIStringAnnotationAttr
+ >([&](auto attr) { return translateImpl(attr); });
if (node && !node->isTemporary())
attrToNode.insert({attr, node});
|
|
@llvm/pr-subscribers-mlir Author: Walter Erquinigo (walter-erquinigo) ChangesLLVM already supports Full diff: https://github.com/llvm/llvm-project/pull/110946.diff 4 Files Affected:
diff --git a/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td b/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td
index 2da45eba77655b..dc711232aa9be8 100644
--- a/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td
+++ b/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td
@@ -586,7 +586,8 @@ def LLVM_DISubprogramAttr : LLVM_Attr<"DISubprogram", "di_subprogram",
OptionalParameter<"unsigned">:$scopeLine,
OptionalParameter<"DISubprogramFlags">:$subprogramFlags,
OptionalParameter<"DISubroutineTypeAttr">:$type,
- OptionalArrayRefParameter<"DINodeAttr">:$retainedNodes
+ OptionalArrayRefParameter<"DINodeAttr">:$retainedNodes,
+ OptionalArrayRefParameter<"DINodeAttr">:$annotations
);
let builders = [
AttrBuilder<(ins
@@ -594,11 +595,11 @@ def LLVM_DISubprogramAttr : LLVM_Attr<"DISubprogram", "di_subprogram",
"DIScopeAttr":$scope, "StringAttr":$name, "StringAttr":$linkageName,
"DIFileAttr":$file, "unsigned":$line, "unsigned":$scopeLine,
"DISubprogramFlags":$subprogramFlags, "DISubroutineTypeAttr":$type,
- "ArrayRef<DINodeAttr>":$retainedNodes
+ "ArrayRef<DINodeAttr>":$retainedNodes, "ArrayRef<DINodeAttr>":$annotations
), [{
return $_get($_ctxt, /*recId=*/nullptr, /*isRecSelf=*/false, id, compileUnit,
scope, name, linkageName, file, line, scopeLine,
- subprogramFlags, type, retainedNodes);
+ subprogramFlags, type, retainedNodes, annotations);
}]>
];
let assemblyFormat = "`<` struct(params) `>`";
@@ -670,6 +671,21 @@ def LLVM_DIImportedEntityAttr : LLVM_Attr<"DIImportedEntity", "di_imported_entit
let assemblyFormat = "`<` struct(params) `>`";
}
+//===----------------------------------------------------------------------===//
+// DIStringAnnotationAttr
+//===----------------------------------------------------------------------===//
+
+def LLVM_DIStringAnnotationAttr : LLVM_Attr<"DIStringAnnotation",
+ "di_string_annotation",
+ /*traits=*/[], "DINodeAttr"> {
+ let parameters = (ins
+ "StringAttr":$name,
+ "StringAttr":$value
+ );
+
+ let assemblyFormat = "`<` struct(params) `>`";
+}
+
//===----------------------------------------------------------------------===//
// DISubrangeAttr
//===----------------------------------------------------------------------===//
diff --git a/mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp b/mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp
index bd2164e640e7b8..a4304ef6668e5e 100644
--- a/mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp
+++ b/mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp
@@ -60,9 +60,9 @@ bool DINodeAttr::classof(Attribute attr) {
DIDerivedTypeAttr, DIFileAttr, DIGlobalVariableAttr,
DIImportedEntityAttr, DILabelAttr, DILexicalBlockAttr,
DILexicalBlockFileAttr, DILocalVariableAttr, DIModuleAttr,
- DINamespaceAttr, DINullTypeAttr, DIStringTypeAttr,
- DISubprogramAttr, DISubrangeAttr, DISubroutineTypeAttr>(
- attr);
+ DINamespaceAttr, DINullTypeAttr, DIStringAnnotationAttr,
+ DIStringTypeAttr, DISubprogramAttr, DISubrangeAttr,
+ DISubroutineTypeAttr>(attr);
}
//===----------------------------------------------------------------------===//
@@ -221,15 +221,16 @@ DICompositeTypeAttr::getRecSelf(DistinctAttr recId) {
//===----------------------------------------------------------------------===//
DIRecursiveTypeAttrInterface DISubprogramAttr::withRecId(DistinctAttr recId) {
- return DISubprogramAttr::get(
- getContext(), recId, getIsRecSelf(), getId(), getCompileUnit(),
- getScope(), getName(), getLinkageName(), getFile(), getLine(),
- getScopeLine(), getSubprogramFlags(), getType(), getRetainedNodes());
+ return DISubprogramAttr::get(getContext(), recId, getIsRecSelf(), getId(),
+ getCompileUnit(), getScope(), getName(),
+ getLinkageName(), getFile(), getLine(),
+ getScopeLine(), getSubprogramFlags(), getType(),
+ getRetainedNodes(), getAnnotations());
}
DIRecursiveTypeAttrInterface DISubprogramAttr::getRecSelf(DistinctAttr recId) {
return DISubprogramAttr::get(recId.getContext(), recId, /*isRecSelf=*/true,
- {}, {}, {}, {}, {}, 0, 0, {}, {}, {}, {});
+ {}, {}, {}, {}, {}, 0, 0, {}, {}, {}, {}, {});
}
//===----------------------------------------------------------------------===//
diff --git a/mlir/lib/Dialect/LLVMIR/Transforms/DIScopeForLLVMFuncOp.cpp b/mlir/lib/Dialect/LLVMIR/Transforms/DIScopeForLLVMFuncOp.cpp
index 2cfaffa7c8efce..59a4f8e9023582 100644
--- a/mlir/lib/Dialect/LLVMIR/Transforms/DIScopeForLLVMFuncOp.cpp
+++ b/mlir/lib/Dialect/LLVMIR/Transforms/DIScopeForLLVMFuncOp.cpp
@@ -78,7 +78,7 @@ static void addScopeToFunction(LLVM::LLVMFuncOp llvmFunc,
auto subprogramAttr = LLVM::DISubprogramAttr::get(
context, id, compileUnitAttr, fileAttr, funcName, funcName, fileAttr,
/*line=*/line, /*scopeline=*/col, subprogramFlags, subroutineTypeAttr,
- /*retainedNodes=*/{});
+ /*retainedNodes=*/{}, /*annotations=*/{});
llvmFunc->setLoc(FusedLoc::get(context, {loc}, subprogramAttr));
}
diff --git a/mlir/lib/Target/LLVMIR/DebugTranslation.cpp b/mlir/lib/Target/LLVMIR/DebugTranslation.cpp
index 8ca3beca6b66f7..5b7b28a761ec83 100644
--- a/mlir/lib/Target/LLVMIR/DebugTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/DebugTranslation.cpp
@@ -102,6 +102,13 @@ DebugTranslation::getMDTupleOrNull(ArrayRef<DINodeAttr> elements) {
return nullptr;
SmallVector<llvm::Metadata *> llvmElements = llvm::to_vector(
llvm::map_range(elements, [&](DINodeAttr attr) -> llvm::Metadata * {
+ if (DIStringAnnotationAttr annAttr =
+ dyn_cast<DIStringAnnotationAttr>(attr)) {
+ llvm::Metadata *ops[2] = {
+ llvm::MDString::get(llvmCtx, annAttr.getName()),
+ llvm::MDString::get(llvmCtx, annAttr.getValue())};
+ return llvm::MDNode::get(llvmCtx, ops);
+ }
return translate(attr);
}));
return llvm::MDNode::get(llvmCtx, llvmElements);
@@ -332,7 +339,8 @@ llvm::DISubprogram *DebugTranslation::translateImpl(DISubprogramAttr attr) {
/*ThisAdjustment=*/0, llvm::DINode::FlagZero,
static_cast<llvm::DISubprogram::DISPFlags>(attr.getSubprogramFlags()),
compileUnit, /*TemplateParams=*/nullptr, /*Declaration=*/nullptr,
- getMDTupleOrNull(attr.getRetainedNodes()));
+ getMDTupleOrNull(attr.getRetainedNodes()), nullptr,
+ getMDTupleOrNull(attr.getAnnotations()));
if (attr.getId())
distinctAttrToNode.try_emplace(attr.getId(), node);
return node;
@@ -361,6 +369,16 @@ DebugTranslation::translateImpl(DIImportedEntityAttr attr) {
getMDStringOrNull(attr.getName()), getMDTupleOrNull(attr.getElements()));
}
+/*
+llvm::DINodeArray *
+DebugTranslation::translateImpl(DIStringAnnotationAttr attr) {
+ llvm::DINodeArray arr;
+ arr->push_back(llvm::MDString::get(llvmCtx, attr.getName()));
+ arr->push_back(llvm::MDString::get(llvmCtx, attr.getValue()));
+ return arr;
+}
+*/
+
llvm::DISubrange *DebugTranslation::translateImpl(DISubrangeAttr attr) {
auto getMetadataOrNull = [&](Attribute attr) -> llvm::Metadata * {
if (!attr)
@@ -426,8 +444,9 @@ llvm::DINode *DebugTranslation::translate(DINodeAttr attr) {
DIImportedEntityAttr, DILabelAttr, DILexicalBlockAttr,
DILexicalBlockFileAttr, DILocalVariableAttr, DIModuleAttr,
DINamespaceAttr, DINullTypeAttr, DIStringTypeAttr,
- DISubprogramAttr, DISubrangeAttr, DISubroutineTypeAttr>(
- [&](auto attr) { return translateImpl(attr); });
+ DISubprogramAttr, DISubrangeAttr, DISubroutineTypeAttr
+ // DIStringAnnotationAttr
+ >([&](auto attr) { return translateImpl(attr); });
if (node && !node->isTemporary())
attrToNode.insert({attr, node});
|
walter-erquinigo
force-pushed
the
users/walter-erquinigo/annotations
branch
from
9c3c671 to
54f5b32
Compare
|
Nice. I agree re: round-trip tests. |
walter-erquinigo
force-pushed
the
users/walter-erquinigo/annotations
branch
from
54f5b32 to
e4dbd74
Compare
|
@Dinistro , I've added a few basic tests for this. Let me know if there's something I'm missing with, if possible, code pointers, because I'm not that familiar with the mlir tests. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
walter-erquinigo
changed the title
walter-erquinigo
force-pushed
the
users/walter-erquinigo/annotations
branch
2 times, most recently
from
646d4c2 to
259da6b
Compare
llvmbot
added
flang
There was a problem hiding this comment.
This should be tested somewhere in test/Target/LLMVIR/Import. Probably in the debug-info.ll file
There was a problem hiding this comment.
This should be tested in test/Target/LLVMIR/. Probably in llvmir-debug.mlir
walter-erquinigo
force-pushed
the
users/walter-erquinigo/annotations
branch
from
259da6b to
f4196fe
Compare
|
@Dinistro , I've added one more test but I'm kind of unable to implement an import test because llvm doesn't have actual C++ entities for |
I'm not too familiar with this in LLVM IR. So this can not even be written in textual IR? |
I'm also not very familiar with this, as annotations have been implemented as a bag of data in LLVM IR. Probably that's why proper declarations in LLVM IR haven't been introduced, even though it seems people are only using it to store strings. If it serves you, I was able to have my compiler define annotations at the MLIR level and generate a resultant binary whose DWARF in fact had this annotations. I don't see why importing wouldn't work, but it's just the representation used by |
|
This is information that is not represented in IR? What is the LLVM IR output of your export test? I would expect that this is somehow shown in the IR. |
I was able to figure out how to write that I just found a crash in the import and I'm on way to fix it. Thanks for pointing out to this obvious thing I should have done (reading the exported IR) |
walter-erquinigo
force-pushed
the
users/walter-erquinigo/annotations
branch
2 times, most recently
from
cdea4b0 to
6db422c
Compare
|
@Dinistro , thanks for insisting on the test. I was able to write an import test and everything is working. |
walter-erquinigo
force-pushed
the
users/walter-erquinigo/annotations
branch
from
6db422c to
5236967
Compare
|
Thanks! Everything is fully tested now :) |
| context, cast<llvm::MDString>(tuple->getOperand(0))->getString()); | ||
| const auto value = StringAttr::get( | ||
| context, cast<llvm::MDString>(tuple->getOperand(1))->getString()); | ||
| annotations.push_back(DIAnnotationAttr::get(context, name, value)); |
There was a problem hiding this comment.
You said that you only support string annotations for now, but that there might be others, right? If that's correct, this will cause crashes if we hit such cases in the import.
It might be sensible to use dyn_cast and skip this part when we aren't dealing with llvm::MDString?
Same for the number of operands, there is probably no verifier in LLVM, so we need to check this here to avoid indexing out of range.
There was a problem hiding this comment.
You said that you only support string annotations for now, but that there might be others, right? If that's correct, this will cause crashes if we hit such cases in the import.
It might be sensible to use dyn_cast and skip this part
I'm only adding support for strings because this is the only case that I've seen people using annotations with, and I don't see the need to make it more complex. At some point, if someone needs a more complex structure, they could improve these definitions.
And yes, using dyn_cast is a pretty good idea!
There was a problem hiding this comment.
Only supporting strings is fine, but crashing on legal LLVM IR is not. This should just be resilient to arbitary legal inputs and not randomly crash or segfault.
There was a problem hiding this comment.
Yeah, that makes sense. The current code would only crash if the name of the annotation is not a string. All other conditions are now flexible.
LLVM already supports `DW_TAG_LLVM_annotation` entries for subroutines, but this hasn't been surfaced to the LLVM dialect. I'm doing the minimal amount of work to support string-based annotations, which is useful for attaching metadata to functions, which is useful for debuggers to offer features beyond basic DWARF. As LLVM already supports this, this patch is not controversial.
walter-erquinigo
force-pushed
the
users/walter-erquinigo/annotations
branch
from
5236967 to
2b080eb
Compare
|
@Dinistro , please give it one more look! |
|
Thanks, man! |
LLVM already supports
DW_TAG_LLVM_annotationentries for subprograms, but this hasn't been surfaced to the LLVM dialect.I'm doing the minimal amount of work to support string-based annotations, which is useful for attaching metadata to
functions, which is useful for debuggers to offer features beyond basic DWARF.
As LLVM already supports this, this patch is not controversial.