Skip to content
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.

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

[LLVM] Expand tvm::Type to DWARF conversion #14568

Merged
merged 3 commits into from
Apr 11, 2023

Conversation

Lunderberg
Copy link
Contributor

Prior to this commit, DWARF debug symbols for float32, int8, and int32 could be generated, with other datatypes resulting in an error. This commit expands the range of types with debug symbols to allow for any DLDataTypeCode::kDLInt, kDLUint, or kDLFloat, regardless of the bitsize.

@tvm-bot
Copy link
Collaborator

tvm-bot commented Apr 10, 2023

Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.

  • No users to tag found in teams: llvm See #10317 for details

Generated by tvm-bot

Prior to this commit, DWARF debug symbols for `float32`, `int8`, and
`int32` could be generated, with other datatypes resulting in an
error.  This commit expands the range of types with debug symbols to
allow for any `DLDataTypeCode::kDLInt`, `kDLUint`, or `kDLFloat`,
regardless of the bitsize.
Copy link
Contributor

@echuraev echuraev left a comment

Choose a reason for hiding this comment

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

Is it necessary to add a unit test? What do you think?

Comment on lines 316 to 324
std::stringstream ss;
ss << name;

if (!dtype.is_bool()) {
ss << dtype.bits();
}
if (dtype.lanes() > 1) {
ss << "x" << dtype.lanes();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

There is already a function that gets the string from DataType here.

Please add #include <sstream> for std::stringstream if you decide to keep it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, and updated to use DLDataType2String.


} else if (auto* prim_type = ty_tir.as<PrimTypeNode>()) {
DataType dtype = prim_type->dtype;
auto [name, dwarf_type] = [&]() -> std::tuple<const char*, llvm::dwarf::TypeKind> {
Copy link
Contributor

Choose a reason for hiding this comment

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

llvm::StringRef is the "LLVM way" rather than const char*.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. With the DLDataType2String it ended up being unnecessary, but I will keep that in mind for the future.

@Lunderberg
Copy link
Contributor Author

@echuraev Good point. Probably not strictly required, as the existing DWARF handling isn't tested, but this does impact what function signatures are allowed when not using the PackedFunc API. I've added a unit test that fails on main, and passes with this change.

Copy link
Contributor

@kparzysz-quic kparzysz-quic left a comment

Choose a reason for hiding this comment

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

Your comment says that you've added a unit test for this, but it's not in this PR. Is it included in another PR? In any case, feel free to merge this now if you want it in sooner than later, and commit the testcase in another PR.

@Lunderberg
Copy link
Contributor Author

@kparzysz-quic Thank you, and thank you for the catch! Looks like I added the unit test in my local branch, then forgot to push.

@kparzysz-quic kparzysz-quic merged commit ca7c3d8 into apache:main Apr 11, 2023
@Lunderberg Lunderberg deleted the llvm_additional_dwarf_types branch April 12, 2023 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants