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

[TVMScript][TIR] Pretty print TIR LLVM function name #15953

Merged
merged 1 commit into from Oct 25, 2023

Conversation

cbalint13
Copy link
Contributor

@cbalint13 cbalint13 commented Oct 19, 2023

This allows printing of the LLVM function real name in TIR printer.
Prior to this a counter-intuitive T.int32() value was printed instead of the real name.


Changes

Before: T.call_llvm_pure_intrin("int32x4", T.uint32(62), T.uint32(0))
After: T.call_llvm_pure_intrin("int32x4", "llvm.donothing", T.uint32(0))

Notes

* This preserves the llvm IR emitter to still accept IntImm along with StringImm argument.
* Python tir interface now sends StringImm instead of IntImm making it printing friendly.
* There are subarches that still uses IntImm for custom things, those will just keep working fine.
* It is not possible to teach the tir-printer instead, CallNode context is lost away at the IntImm handling point.

This is part of #15918 .

@cbalint13 cbalint13 force-pushed the tir-printer branch 2 times, most recently from aec0876 to 22a08ed Compare October 20, 2023 08:29
Copy link
Contributor

@lhutton1 lhutton1 left a comment

Choose a reason for hiding this comment

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

This has been bugging me for a while, thanks for fixing it @cbalint13! I'm conscious this is still marked as draft, I'd love to take a closer look when you're ready :)

@cbalint13 cbalint13 marked this pull request as ready for review October 20, 2023 18:17
@cbalint13
Copy link
Contributor Author

cbalint13 commented Oct 20, 2023

This has been bugging me for a while, thanks for fixing it @cbalint13! I'm conscious this is still marked as draft, I'd love to take a closer look when you're ready :)

@lhutton1

Ready for review.

I kept it as Draft, avoiding to "spam" people until all-in green CI status.
Also thanks for @ekalda for his review patience and his previous suggestions.

Adding to Cc: @Lunderberg , @junrushao , @masahi , @vinx13, @quic-sanirudh , @kparzysz-quic

Copy link
Contributor

@quic-sanirudh quic-sanirudh left a comment

Choose a reason for hiding this comment

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

This has been in my todo list for a while too, so thanks a lot for this. I just mentioned one small comment about using ICHECK in place of assertion, but otherwise it looks good to me.

src/target/llvm/codegen_arm.cc Outdated Show resolved Hide resolved
src/target/llvm/codegen_llvm.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@lhutton1 lhutton1 left a comment

Choose a reason for hiding this comment

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

LGTM!

python/tvm/tir/op.py Show resolved Hide resolved
@tqchen
Copy link
Member

tqchen commented Oct 23, 2023

cc @junrushao @Hzfengsy

@cbalint13 cbalint13 force-pushed the tir-printer branch 2 times, most recently from d6e0f57 to 300a592 Compare October 23, 2023 14:08
Copy link
Contributor

@quic-sanirudh quic-sanirudh left a comment

Choose a reason for hiding this comment

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

Thanks

@Lunderberg
Copy link
Contributor

It looks like this changes the underlying IR on the C++ side, which is a wider change than I expected given the name of the PR. Using StringImm for the LLVM intrinsic requires updated the LLVM codegen, and breaks IDE support for "find all references" when applied to the intrinsic's enum value.

I like the user-friendliness of this change, and thing it should be implemented on the TVMScript side instead. When printing out a tir::Call node (here in the TVMScript printer), we could check whether we are calling an LLVM intrinsic, and convert the intrinsic's integer value to string at that point.

@cbalint13
Copy link
Contributor Author

I like the user-friendliness of this change, and thing it should be implemented on the TVMScript side instead. When printing out a tir::Call node (here in the TVMScript printer), we could check whether we are calling an LLVM intrinsic, and convert the intrinsic's integer value to string at that point.

@Lunderberg ,

Thanks a lot for the feedback !
Now it is reimplemented via docsifier.

Copy link
Contributor

@Lunderberg Lunderberg left a comment

Choose a reason for hiding this comment

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

Thank you for making the changes, and LGTM! If we have more TVM intrinsics that should have custom print methods, we may want to add some per-intrinsic function with signature Array<PrimExpr>(Array<PrimExpr>). That way, each TVM intrinsic could mutate the printed arguments prior to printing.

@cbalint13
Copy link
Contributor Author

Thanks @lhutton1 , @quic-sanirudh and @Lunderberg !

Copy link
Contributor

@quic-sanirudh quic-sanirudh left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks

@ekalda ekalda merged commit 885fc27 into apache:main Oct 25, 2023
19 checks passed
@ekalda
Copy link
Contributor

ekalda commented Oct 25, 2023

Thanks @cbalint13 for the great work on this!

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.

None yet

6 participants