[MLIR][LLVM] add dwarfAddressSpace to DIDerivedType #92043
Conversation
This field is present in LLVM, but was missing from the MLIR wrapper type. This addition allows MLIR languages to add proper DWARF info for GPU programs.
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-mlir @llvm/pr-subscribers-mlir-llvm Author: William G Hatch (willghatch) ChangesThis field is present in LLVM, but was missing from the MLIR wrapper type. This addition allows MLIR languages to add proper DWARF info for GPU programs. Full diff: https://github.com/llvm/llvm-project/pull/92043.diff 7 Files Affected:
diff --git a/mlir/include/mlir-c/Dialect/LLVM.h b/mlir/include/mlir-c/Dialect/LLVM.h
index bd9b7dd26f5e9..790197070f998 100644
--- a/mlir/include/mlir-c/Dialect/LLVM.h
+++ b/mlir/include/mlir-c/Dialect/LLVM.h
@@ -238,7 +238,7 @@ MLIR_CAPI_EXPORTED MlirAttribute mlirLLVMDICompositeTypeAttrGet(
MLIR_CAPI_EXPORTED MlirAttribute mlirLLVMDIDerivedTypeAttrGet(
MlirContext ctx, unsigned int tag, MlirAttribute name,
MlirAttribute baseType, uint64_t sizeInBits, uint32_t alignInBits,
- uint64_t offsetInBits, MlirAttribute extraData);
+ uint64_t offsetInBits, unsigned dwarfAddressSpace, MlirAttribute extraData);
/// Gets the base type from a LLVM DIDerivedType attribute.
MLIR_CAPI_EXPORTED MlirAttribute
diff --git a/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td b/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td
index cc849cb7c978d..8965f4f652a20 100644
--- a/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td
+++ b/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td
@@ -418,6 +418,7 @@ def LLVM_DIDerivedTypeAttr : LLVM_Attr<"DIDerivedType", "di_derived_type",
OptionalParameter<"uint64_t">:$sizeInBits,
OptionalParameter<"uint32_t">:$alignInBits,
OptionalParameter<"uint64_t">:$offsetInBits,
+ OptionalParameter<"std::optional<unsigned>">:$dwarfAddressSpace,
OptionalParameter<"DINodeAttr">:$extraData
);
let assemblyFormat = "`<` struct(params) `>`";
diff --git a/mlir/lib/CAPI/Dialect/LLVM.cpp b/mlir/lib/CAPI/Dialect/LLVM.cpp
index 4669c40f843d9..71acb68aee786 100644
--- a/mlir/lib/CAPI/Dialect/LLVM.cpp
+++ b/mlir/lib/CAPI/Dialect/LLVM.cpp
@@ -172,11 +172,15 @@ MlirAttribute
mlirLLVMDIDerivedTypeAttrGet(MlirContext ctx, unsigned int tag,
MlirAttribute name, MlirAttribute baseType,
uint64_t sizeInBits, uint32_t alignInBits,
- uint64_t offsetInBits, MlirAttribute extraData) {
+ uint64_t offsetInBits, unsigned dwarfAddressSpace,
+ MlirAttribute extraData) {
+ std::optional<unsigned> addressSpace = std::nullopt;
+ if (dwarfAddressSpace != 0)
+ addressSpace = dwarfAddressSpace;
return wrap(DIDerivedTypeAttr::get(
unwrap(ctx), tag, cast<StringAttr>(unwrap(name)),
cast<DITypeAttr>(unwrap(baseType)), sizeInBits, alignInBits, offsetInBits,
- cast<DINodeAttr>(unwrap(extraData))));
+ addressSpace, cast<DINodeAttr>(unwrap(extraData))));
}
MlirAttribute
diff --git a/mlir/lib/Target/LLVMIR/DebugImporter.cpp b/mlir/lib/Target/LLVMIR/DebugImporter.cpp
index 4a4e1d1ecdd86..904fe7da2051d 100644
--- a/mlir/lib/Target/LLVMIR/DebugImporter.cpp
+++ b/mlir/lib/Target/LLVMIR/DebugImporter.cpp
@@ -99,7 +99,7 @@ DIDerivedTypeAttr DebugImporter::translateImpl(llvm::DIDerivedType *node) {
return DIDerivedTypeAttr::get(
context, node->getTag(), getStringAttrOrNull(node->getRawName()),
baseType, node->getSizeInBits(), node->getAlignInBits(),
- node->getOffsetInBits(), extraData);
+ node->getOffsetInBits(), node->getDWARFAddressSpace(), extraData);
}
DIFileAttr DebugImporter::translateImpl(llvm::DIFile *node) {
diff --git a/mlir/lib/Target/LLVMIR/DebugTranslation.cpp b/mlir/lib/Target/LLVMIR/DebugTranslation.cpp
index 2de5e372d88c0..652ac0039cda7 100644
--- a/mlir/lib/Target/LLVMIR/DebugTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/DebugTranslation.cpp
@@ -165,7 +165,7 @@ llvm::DIDerivedType *DebugTranslation::translateImpl(DIDerivedTypeAttr attr) {
/*File=*/nullptr, /*Line=*/0,
/*Scope=*/nullptr, translate(attr.getBaseType()), attr.getSizeInBits(),
attr.getAlignInBits(), attr.getOffsetInBits(),
- /*DWARFAddressSpace=*/std::nullopt, /*PtrAuthData=*/std::nullopt,
+ attr.getDwarfAddressSpace(), /*PtrAuthData=*/std::nullopt,
/*Flags=*/llvm::DINode::FlagZero, translate(attr.getExtraData()));
}
diff --git a/mlir/test/CAPI/llvm.c b/mlir/test/CAPI/llvm.c
index 25f900e521cf9..cc13f5d398189 100644
--- a/mlir/test/CAPI/llvm.c
+++ b/mlir/test/CAPI/llvm.c
@@ -297,7 +297,7 @@ static void testDebugInfoAttributes(MlirContext ctx) {
1, 0, 8, di_type));
// CHECK: #llvm.di_derived_type<{{.*}}>
mlirAttributeDump(
- mlirLLVMDIDerivedTypeAttrGet(ctx, 0, bar, di_type, 64, 8, 0, di_type));
+ mlirLLVMDIDerivedTypeAttrGet(ctx, 0, bar, di_type, 64, 8, 0, 0, di_type));
// CHECK: #llvm.di_composite_type<{{.*}}>
mlirAttributeDump(mlirLLVMDICompositeTypeAttrGet(
diff --git a/mlir/test/Target/LLVMIR/llvmir-debug.mlir b/mlir/test/Target/LLVMIR/llvmir-debug.mlir
index 8ab1a1b290dad..dde541db25035 100644
--- a/mlir/test/Target/LLVMIR/llvmir-debug.mlir
+++ b/mlir/test/Target/LLVMIR/llvmir-debug.mlir
@@ -35,6 +35,11 @@ llvm.func @func_no_debug() {
// Specify the name parameter.
tag = DW_TAG_pointer_type, name = "named", baseType = #si32
>
+#ptrWithAddressSpace = #llvm.di_derived_type<
+ tag = DW_TAG_pointer_type, baseType = #si32,
+ sizeInBits = 64, alignInBits = 32, offsetInBits = 8,
+ dwarfAddressSpace = 3
+>
#cu = #llvm.di_compile_unit<
id = distinct[0]<>, sourceLanguage = DW_LANG_C, file = #file,
producer = "MLIR", isOptimized = true, emissionKind = Full,
@@ -51,7 +56,7 @@ llvm.func @func_no_debug() {
elements = #llvm.di_subrange<lowerBound = 0, upperBound = 4, stride = 1>
>
#null = #llvm.di_null_type
-#spType0 = #llvm.di_subroutine_type<callingConvention = DW_CC_normal, types = #null, #si64, #ptr, #named, #composite, #vector>
+#spType0 = #llvm.di_subroutine_type<callingConvention = DW_CC_normal, types = #null, #si64, #ptr, #named, #ptrWithAddressSpace, #composite, #vector>
#toplevel_namespace = #llvm.di_namespace<
name = "toplevel", exportSymbols = true
>
@@ -135,11 +140,12 @@ llvm.func @empty_types() {
// CHECK: ![[NESTED_NAMESPACE]] = !DINamespace(name: "nested", scope: ![[TOPLEVEL_NAMESPACE:.*]])
// CHECK: ![[TOPLEVEL_NAMESPACE]] = !DINamespace(name: "toplevel", scope: null, exportSymbols: true)
// CHECK: ![[FUNC_TYPE]] = !DISubroutineType(cc: DW_CC_normal, types: ![[FUNC_ARGS:.*]])
-// CHECK: ![[FUNC_ARGS]] = !{null, ![[ARG_TYPE:.*]], ![[PTR_TYPE:.*]], ![[NAMED_TYPE:.*]], ![[COMPOSITE_TYPE:.*]], ![[VECTOR_TYPE:.*]]}
+// CHECK: ![[FUNC_ARGS]] = !{null, ![[ARG_TYPE:.*]], ![[PTR_TYPE:.*]], ![[NAMED_TYPE:.*]], ![[PTR_WITH_ADDR_TYPE:.*]], ![[COMPOSITE_TYPE:.*]], ![[VECTOR_TYPE:.*]]}
// CHECK: ![[ARG_TYPE]] = !DIBasicType(name: "si64")
// CHECK: ![[PTR_TYPE]] = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: ![[BASE_TYPE:.*]], size: 64, align: 32, offset: 8, extraData: ![[BASE_TYPE]])
// CHECK: ![[BASE_TYPE]] = !DIBasicType(name: "si32", size: 32, encoding: DW_ATE_signed)
// CHECK: ![[NAMED_TYPE]] = !DIDerivedType(tag: DW_TAG_pointer_type, name: "named", baseType: ![[BASE_TYPE:.*]])
+// CHECK: ![[PTR_WITH_ADDR_TYPE]] = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: ![[BASE_TYPE:.*]], size: 64, align: 32, offset: 8, dwarfAddressSpace: 3)
// CHECK: ![[COMPOSITE_TYPE]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "composite", file: ![[CU_FILE_LOC]], line: 42, size: 64, align: 32, elements: ![[COMPOSITE_ELEMENTS:.*]])
// CHECK: ![[COMPOSITE_ELEMENTS]] = !{![[COMPOSITE_ELEMENT:.*]]}
// CHECK: ![[COMPOSITE_ELEMENT]] = !DISubrange(count: 4)
|
|
Note that I'm not really sure what to do about the C API. One option is to use a signed type and turn negative values into |
There was a problem hiding this comment.
Thanks for your contribution 🙂
Do you know if there is a semantic difference between std::nullopt and zero? If there is none, your CAPI question would resolve itself.
Could you also add this to the LLVM dialect roundtrip test, as well as an debug metadata import test?
| @@ -418,6 +418,7 @@ def LLVM_DIDerivedTypeAttr : LLVM_Attr<"DIDerivedType", "di_derived_type", | |||
| OptionalParameter<"uint64_t">:$sizeInBits, | |||
| OptionalParameter<"uint32_t">:$alignInBits, | |||
| OptionalParameter<"uint64_t">:$offsetInBits, | |||
| OptionalParameter<"std::optional<unsigned>">:$dwarfAddressSpace, | |||
There was a problem hiding this comment.
Modeling this as an optional optional seems a bit odd. Do you know if there is a sematnic difference between zero and std::nullopt? If there is none, it would be nicer to model this as OptionalParameter<"uint32_t">.
There was a problem hiding this comment.
I think the design follows LLVM in this regard. Using an integer would solve the CAPI issue but then it is also a slight difference with regards to LLVM (i.e. when round tripping MLIR would suddenly add extra address space information, which may be ok).
There was a problem hiding this comment.
In one sense, I believe there is not a semantic difference in terms of what the debuginfo means between having no dwarf address space field and having a zero field, but the underlying LLVM implementation will do different things when it gets std::nullopt vs 0, IE omitting the field or emitting an attribute specifying DW_AT_address_class as 0. For cases where you aren't targetting a GPU, it makes more sense to omit the attribute even if there is no semantic difference between a lack of attribute and a zero value. Additionally, I'm not certain that all uses of the DW_AT_address_class attribute do ensure that zero is the same as lack of an attribute (IE there may be targets where there is a semantic difference that I'm unaware of). I mainly modeled this to capture the behaviors of the underlying LLVM implementation, but I'll do a bit of digging to try to see whether it matters.
There was a problem hiding this comment.
Yeah this is something that's entirely target-defined, so as long as there's a difference in spec between nullopt & 0, it's probably safer to also keep the optional representation in MLIR to avoid any weird bugs down the road?
There was a problem hiding this comment.
Actually, I thought it didn't, but it appears DWARF (v5, ch. 7.13) specifies a common DW_ADDR_none as 0. From ch. 2.12:
The set of permissible values is specific to each target architecture. The value DW_ADDR_none, however, is common to all encodings, and means that no address class has been specified.
I still think that if we want to adopt this, we should probably either remove the optional from the LLVM side, or at least translate 0 as nullopt into LLVM. It does mean we lose one degree of freedom, which is controlling whether the 0 address space actually gets emitted or not in DWARF (it's not clear if that should make a difference).
There was a problem hiding this comment.
I haven't found anything indicating that lack of DW_AT_address_class is necessarily assumed to be different from a zero value, but I think there is practical value to being able to not emit the attribute when not dealing with GPUs as well as explicitly emit 0 when you are.
There was a problem hiding this comment.
oh yeah I 100% agree on the part about not emitting the 0 attribute for space saving in the general case. I'm less sure if we need to emit it for certain targets though (it's possible some targets have their quirks).
Though if it was a target-based rule on whether to emit 0, we can encode that in the llvm backend for that target. The frontend should not have to worry about it.
Now are there non-target-based rules? I'm not sure.
There was a problem hiding this comment.
Oh, @zyx-billy , I hadn't even seen your comments when I made mine, github's UI refreshing is inconsistent between top-level and thread comments... Yeah, maybe we should remove the optionality entirely.
There was a problem hiding this comment.
That keeps happening to me, I need to remember to refresh the page each time before I write a comment to check if others have come in...
There was a problem hiding this comment.
haha yeah me too, it's not always consistent about auto-updating these messages 🤦. OK yeah if you also think it's ok to just use DW_ADDR_none we can do that and simplify away this c-api problem too. It would be nice to change llvm too, though of course that's out of scope for this PR 😂 we can do that later if we want.
There was a problem hiding this comment.
Thanks!
Can you add small import / export tests in:
llvm-project/mlir/test/Target/LLVMIR/llvmir-debug.mlir
llvm-project/mlir/test/Target/LLVMIR/Import/debug-info.ll
I didn't check but most likely it is sufficient to edit an existing test as long as we cover import/export without and with address space. I do not expect any issues but it still makes sense to cover this in the tests.
I am not super familiar with the C API and I didn't see any prior art modeling an std::nullopt. I guess your current solution makes sense if 0 = std::nullopt in all cases with practical relevance. If we need to make a difference, the solution with a signed integer and some constant that represents std::nullopt maybe better. |
- add another test for import - improve C API and test it more thoroughly
|
I've improved the testing, but when mentioning “round trip tests”, does that include anything other than I think using a signed integer to allow |
You can test this locally with the following command:git-clang-format --diff 14122106320b88f114301bbf8694ceac9bb7a27a f84b4f7d7bff30ffe602eda6e4ad45ecc499bddc -- mlir/include/mlir-c/Dialect/LLVM.h mlir/lib/CAPI/Dialect/LLVM.cpp mlir/lib/Target/LLVMIR/DebugImporter.cpp mlir/lib/Target/LLVMIR/DebugTranslation.cpp mlir/test/CAPI/llvm.cView the diff from clang-format here.diff --git a/mlir/lib/CAPI/Dialect/LLVM.cpp b/mlir/lib/CAPI/Dialect/LLVM.cpp
index f81226e7eb..af88d771c5 100644
--- a/mlir/lib/CAPI/Dialect/LLVM.cpp
+++ b/mlir/lib/CAPI/Dialect/LLVM.cpp
@@ -168,12 +168,10 @@ MlirAttribute mlirLLVMDICompositeTypeAttrGet(
[](Attribute a) { return a.cast<DINodeAttr>(); })));
}
-MlirAttribute
-mlirLLVMDIDerivedTypeAttrGet(MlirContext ctx, unsigned int tag,
- MlirAttribute name, MlirAttribute baseType,
- uint64_t sizeInBits, uint32_t alignInBits,
- uint64_t offsetInBits, int64_t dwarfAddressSpace,
- MlirAttribute extraData) {
+MlirAttribute mlirLLVMDIDerivedTypeAttrGet(
+ MlirContext ctx, unsigned int tag, MlirAttribute name,
+ MlirAttribute baseType, uint64_t sizeInBits, uint32_t alignInBits,
+ uint64_t offsetInBits, int64_t dwarfAddressSpace, MlirAttribute extraData) {
std::optional<unsigned> addressSpace = std::nullopt;
if (dwarfAddressSpace >= 0)
addressSpace = (unsigned)dwarfAddressSpace;
diff --git a/mlir/test/CAPI/llvm.c b/mlir/test/CAPI/llvm.c
index 53e00bc77f..29e0cc14ae 100644
--- a/mlir/test/CAPI/llvm.c
+++ b/mlir/test/CAPI/llvm.c
@@ -297,8 +297,8 @@ static void testDebugInfoAttributes(MlirContext ctx) {
1, 0, 8, di_type));
// CHECK: #llvm.di_derived_type<{{.*}}>
// CHECK-NOT: dwarfAddressSpace
- mlirAttributeDump(
- mlirLLVMDIDerivedTypeAttrGet(ctx, 0, bar, di_type, 64, 8, 0, -1, di_type));
+ mlirAttributeDump(mlirLLVMDIDerivedTypeAttrGet(ctx, 0, bar, di_type, 64, 8, 0,
+ -1, di_type));
// CHECK: #llvm.di_derived_type<{{.*}} dwarfAddressSpace = 3{{.*}}>
mlirAttributeDump(
|
At least for me when updating DI attrs, those two are usually the ones I update, so I think it's ok. |
|
The roundtrip testing is meant a simple LLVM dialect roundtrip, in this specific case: https://github.com/llvm/llvm-project/blob/main/mlir/test/Dialect/LLVMIR/debuginfo.mlir
I guess this is fine for the CAPI. |
|
@Dinistro thank you for pointing out the test file, I was looking for a test like that in the wrong place initially. I think this should be good to go now. |
|
I don't have commit access, so if any of you want to merge it, that would be great. Thanks! |
|
@willghatch Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested Please check whether problems have been caused by your change specifically, as How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
|
Thanks for the patch, I merged it now. |
|
Thanks! |
This field is present in LLVM, but was missing from the MLIR wrapper type. This addition allows MLIR languages to add proper DWARF info for GPU programs.