Skip to content

[MLIR][LLVM] add dwarfAddressSpace to DIDerivedType #92043

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
May 16, 2024
Merged

[MLIR][LLVM] add dwarfAddressSpace to DIDerivedType #92043

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a8c0b74
[MLIR][LLVM] add dwarfAddressSpace to DIDerivedType
willghatch May 10, 2024
da85cfd
review edits
willghatch May 14, 2024
f84b4f7
extend C-API doc string
willghatch May 14, 2024
945c474
formatting...
willghatch May 14, 2024
328e19b
oops, fix test that I was uncareful in writing
willghatch May 14, 2024
01af344
add round-trip test
willghatch May 15, 2024
e128b05
use constant in C API test
willghatch May 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions mlir/include/mlir-c/Dialect/LLVM.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,11 +234,16 @@ MLIR_CAPI_EXPORTED MlirAttribute mlirLLVMDICompositeTypeAttrGet(
MlirAttribute baseType, int64_t flags, uint64_t sizeInBits,
uint64_t alignInBits, intptr_t nElements, MlirAttribute const *elements);

/// Creates a LLVM DIDerivedType attribute.
/// Creates a LLVM DIDerivedType attribute. Note that `dwarfAddressSpace` is an
/// optional field, where `MLIR_CAPI_DWARF_ADDRESS_SPACE_NULL` indicates null
/// and non-negative values indicate a value present.
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, int64_t dwarfAddressSpace, MlirAttribute extraData);

/// Constant to represent std::nullopt for dwarfAddressSpace to omit the field.
#define MLIR_CAPI_DWARF_ADDRESS_SPACE_NULL -1

/// Gets the base type from a LLVM DIDerivedType attribute.
MLIR_CAPI_EXPORTED MlirAttribute
Expand Down
1 change: 1 addition & 0 deletions mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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">.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

@willghatch willghatch May 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

OptionalParameter<"DINodeAttr">:$extraData
);
let assemblyFormat = "`<` struct(params) `>`";
Expand Down
14 changes: 8 additions & 6 deletions mlir/lib/CAPI/Dialect/LLVM.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,15 +168,17 @@ 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, 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;
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
Expand Down
2 changes: 1 addition & 1 deletion mlir/lib/Target/LLVMIR/DebugImporter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion mlir/lib/Target/LLVMIR/DebugTranslation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
}

Expand Down
8 changes: 7 additions & 1 deletion mlir/test/CAPI/llvm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,8 +296,14 @@ static void testDebugInfoAttributes(MlirContext ctx) {
mlirAttributeDump(mlirLLVMDILocalVariableAttrGet(ctx, compile_unit, foo, file,
1, 0, 8, di_type));
// CHECK: #llvm.di_derived_type<{{.*}}>
// CHECK-NOT: dwarfAddressSpace
mlirAttributeDump(mlirLLVMDIDerivedTypeAttrGet(
ctx, 0, bar, di_type, 64, 8, 0, MLIR_CAPI_DWARF_ADDRESS_SPACE_NULL,
di_type));

// CHECK: #llvm.di_derived_type<{{.*}} dwarfAddressSpace = 3{{.*}}>
mlirAttributeDump(
mlirLLVMDIDerivedTypeAttrGet(ctx, 0, bar, di_type, 64, 8, 0, di_type));
mlirLLVMDIDerivedTypeAttrGet(ctx, 0, bar, di_type, 64, 8, 0, 3, di_type));

// CHECK: #llvm.di_composite_type<{{.*}}>
mlirAttributeDump(mlirLLVMDICompositeTypeAttrGet(
Expand Down
11 changes: 9 additions & 2 deletions mlir/test/Dialect/LLVMIR/debuginfo.mlir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,13 @@
tag = DW_TAG_pointer_type, name = "ptr1"
>

// CHECK-DAG: #[[PTR2:.*]] = #llvm.di_derived_type<tag = DW_TAG_pointer_type, baseType = #[[INT0]], sizeInBits = 64, alignInBits = 32, offsetInBits = 4, dwarfAddressSpace = 3, extraData = #[[INT1]]>
#ptr2 = #llvm.di_derived_type<
tag = DW_TAG_pointer_type, baseType = #int0,
sizeInBits = 64, alignInBits = 32, offsetInBits = 4,
dwarfAddressSpace = 3, extraData = #int1
>

// CHECK-DAG: #[[COMP0:.*]] = #llvm.di_composite_type<tag = DW_TAG_array_type, name = "array0", line = 10, sizeInBits = 128, alignInBits = 32>
#comp0 = #llvm.di_composite_type<
tag = DW_TAG_array_type, name = "array0",
Expand Down Expand Up @@ -73,9 +80,9 @@
flags = "TypePassByReference|NonTrivial"
>

// CHECK-DAG: #[[SPTYPE0:.*]] = #llvm.di_subroutine_type<callingConvention = DW_CC_normal, types = #[[NULL]], #[[INT0]], #[[PTR0]], #[[PTR1]], #[[COMP0:.*]], #[[COMP1:.*]], #[[COMP2:.*]]>
// CHECK-DAG: #[[SPTYPE0:.*]] = #llvm.di_subroutine_type<callingConvention = DW_CC_normal, types = #[[NULL]], #[[INT0]], #[[PTR0]], #[[PTR1]], #[[PTR2]], #[[COMP0:.*]], #[[COMP1:.*]], #[[COMP2:.*]]>
#spType0 = #llvm.di_subroutine_type<
callingConvention = DW_CC_normal, types = #null, #int0, #ptr0, #ptr1, #comp0, #comp1, #comp2
callingConvention = DW_CC_normal, types = #null, #int0, #ptr0, #ptr1, #ptr2, #comp0, #comp1, #comp2
>

// CHECK-DAG: #[[SPTYPE1:.*]] = #llvm.di_subroutine_type<types = #[[INT1]], #[[INT1]]>
Expand Down
6 changes: 4 additions & 2 deletions mlir/test/Target/LLVMIR/Import/debug-info.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,8 @@ define void @basic_type() !dbg !3 {
; CHECK: #[[INT:.+]] = #llvm.di_basic_type<tag = DW_TAG_base_type, name = "int">
; CHECK: #[[PTR1:.+]] = #llvm.di_derived_type<tag = DW_TAG_pointer_type, baseType = #[[INT]]>
; CHECK: #[[PTR2:.+]] = #llvm.di_derived_type<tag = DW_TAG_pointer_type, name = "mypointer", baseType = #[[INT]], sizeInBits = 64, alignInBits = 32, offsetInBits = 4, extraData = #[[INT]]>
; CHECK: #llvm.di_subroutine_type<types = #[[PTR1]], #[[PTR2]]>
; CHECK: #[[PTR3:.+]] = #llvm.di_derived_type<tag = DW_TAG_pointer_type, baseType = #[[INT]], dwarfAddressSpace = 3>
; CHECK: #llvm.di_subroutine_type<types = #[[PTR1]], #[[PTR2]], #[[PTR3]]>

define void @derived_type() !dbg !3 {
ret void
Expand All @@ -150,10 +151,11 @@ define void @derived_type() !dbg !3 {
!2 = !DIFile(filename: "debug-info.ll", directory: "/")
!3 = distinct !DISubprogram(name: "derived_type", scope: !2, file: !2, spFlags: DISPFlagDefinition, unit: !1, type: !4)
!4 = !DISubroutineType(types: !5)
!5 = !{!7, !8}
!5 = !{!7, !8, !9}
!6 = !DIBasicType(name: "int")
!7 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !6)
!8 = !DIDerivedType(name: "mypointer", tag: DW_TAG_pointer_type, baseType: !6, size: 64, align: 32, offset: 4, extraData: !6)
!9 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !6, dwarfAddressSpace: 3)

; // -----

Expand Down
10 changes: 8 additions & 2 deletions mlir/test/Target/LLVMIR/llvmir-debug.mlir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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
>
Expand Down Expand Up @@ -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_SPACE:.*]], ![[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_SPACE]] = !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)
Expand Down
Loading