-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
[LLVM] Expand tvm::Type to DWARF conversion #14568
Conversation
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.
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.
98b294b
to
ee7ac4b
Compare
There was a problem hiding this 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?
src/target/llvm/codegen_cpu.cc
Outdated
std::stringstream ss; | ||
ss << name; | ||
|
||
if (!dtype.is_bool()) { | ||
ss << dtype.bits(); | ||
} | ||
if (dtype.lanes() > 1) { | ||
ss << "x" << dtype.lanes(); | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
src/target/llvm/codegen_cpu.cc
Outdated
|
||
} 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> { |
There was a problem hiding this comment.
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*
.
There was a problem hiding this comment.
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.
@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. |
There was a problem hiding this 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.
@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. |
Prior to this commit, DWARF debug symbols for
float32
,int8
, andint32
could be generated, with other datatypes resulting in an error. This commit expands the range of types with debug symbols to allow for anyDLDataTypeCode::kDLInt
,kDLUint
, orkDLFloat
, regardless of the bitsize.