This is an archive of the discontinued LLVM Phabricator instance.

[mlir][llvm] Add an explicit void type debug info attribute.
ClosedPublic

Authored by gysit on Jan 9 2023, 1:11 AM.

Details

Summary

Previously, the DISubroutineType attribute used an optional result
parameter and an optional argument types array to model the subroutine
signature. LLVM IR debug metadata, on the other hand, has one types
list whose first entry maps to the result type. That entry may be
null to model a void result type. The type list may also be entirely
empty not specifying any type information. The latter is problematic
since the current DISubroutineType attribute cannot express it.

The revision changes DISubroutineTypeAttr to closely follow the
LLVM metadata design. In particular, it uses a single types parameter
array to model the subroutine signature and introduces an explicit
DIVoidResultTypeAttr to model the null entries.

Diff Detail

Event Timeline

gysit created this revision.Jan 9 2023, 1:11 AM
Herald added a project: Restricted Project. · View Herald Transcript
gysit requested review of this revision.Jan 9 2023, 1:11 AM
definelicht added inline comments.Jan 9 2023, 1:20 AM
mlir/lib/Target/LLVMIR/DebugImporter.cpp
133–141

Can the nullptr only occur at the beginning? In that case, does it make sense to have this check for every entry, rather than just for the front entry?

gysit updated this revision to Diff 487398.Jan 9 2023, 5:46 AM

Address review comment.

gysit marked an inline comment as done.Jan 9 2023, 5:49 AM
gysit added inline comments.
mlir/lib/Target/LLVMIR/DebugImporter.cpp
133–141

I added a verifier that after completion of the import ensures that only the first element is a void result type.

gysit updated this revision to Diff 487518.Jan 9 2023, 12:02 PM
gysit marked an inline comment as done.

Move newly added verifier into LLVMAttrs.cpp where it belongs.

ftynse added inline comments.Jan 10 2023, 2:10 AM
mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td
125–134

Out of curiosity, what prevents use from using an actual type in MLIR? Do we have to partially duplicate the type system in attributes?

gysit added inline comments.Jan 10 2023, 2:37 AM
mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td
125–134

I see the beauty of this especially for the lowering from a high-level representation and it would address the problem discussed in https://discourse.llvm.org/t/handling-cyclic-dependencies-in-debug-info/67526/3. Unfortunately, these DITypeAttrs are a bit a mix of type and other information. The DISubroutineType for example carries calling convention information and the DICompositeType is also a Scope not just a type. In some cases I am also not sure if there is a useful translation, e.g., for !DIBasicType(tag: DW_TAG_unspecified_type, name: "decltype(nullptr)"). I think my concern would be that the debug metadata is much less structured and there may not be a clean mapping to an actual type system.

I would assume it could be possible to translate some type related information to LLVMDialect types and while still maintaining some wrapper attributes that compose them with the rest of the debug information? This should solve the cyclic dependency issue but the mapping of LLVM IR debug metadata nodes to actual MLIR types may be non-trivial. I can give it a shot to better understand the consequences but it definitely sounds like RFC territory?

gysit added inline comments.Jan 10 2023, 2:47 AM
mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td
125–134

@ftynse Was your idea to basically ignore the type encoded by debug metadata in LLVM IR and instead during the import just use convertType() on the LLVM IR value, function, etc.? That way we would not need to write a new conversion for the import but a translation back to LLVM IR debug metadata during the export.

gysit added inline comments.Jan 11 2023, 11:18 AM
mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td
125–134

FYI,

After iterating a bit more, I came to the conclusion that it is probably hard to reuse existing types despite the overlap.

One aspect is that the debug metadata "types" describe the types implemented by the different frontend languages. They are thus quite generic and rather type descriptions than actual types themselves (= meta types :)). As a result, there is no direct mapping to the existing MLIR/LLVM types (e.g. two C++ structs with different names may map to the same LLVM struct type). Additionally, the debug metadata "types" also carry a lot of additional descriptive information such as definition name, file, and line. This additional information is the essence of the type debug metadata since it allows the debugger to relate a value/function to the front-end language type.

One way to reduce duplication would be if type debug metadata could be attached to types (similar to operations). That way the metadata could really focus on the extra information and there would be no need to describe the structure of the types. However, this conflicts with type uniquing and seems infeasible to implement

Dinistro added inline comments.Jan 12 2023, 3:57 AM
mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp
80

NIT: llvm::any_of and ArrayRef::drop_front could help here.

gysit updated this revision to Diff 488642.Jan 12 2023, 7:09 AM

Address review comment.

Dinistro accepted this revision.Jan 12 2023, 8:25 AM
This revision is now accepted and ready to land.Jan 12 2023, 8:25 AM
This revision was landed with ongoing or failed builds.Jan 12 2023, 8:39 AM
This revision was automatically updated to reflect the committed changes.
gysit reopened this revision.Jan 12 2023, 11:33 PM
This revision is now accepted and ready to land.Jan 12 2023, 11:33 PM
gysit updated this revision to Diff 488878.Jan 12 2023, 11:35 PM

Fix flang build and drop_front on empty list.

Herald added a project: Restricted Project. · View Herald TranscriptJan 12 2023, 11:35 PM
gysit updated this revision to Diff 488880.Jan 12 2023, 11:42 PM

Remove unused headers.