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] Distinguish between void* and handle #14488

Merged
merged 2 commits into from
Apr 10, 2023

Conversation

Lunderberg
Copy link
Contributor

Prior to this PR, the type annotations PrimType(DataType::Handle()) and PointerType(PrimType(DataType::Void())) are both represented as T.handle in TVMScript, which can cause failures to round-trip between TIR and TVMScript. This PR keeps PrimType(DataType::Handle()) as T.handle, but updates the representation of PointerType(PrimType(DataType::Void())) to T.handle("void") in order to distinguish between these two cases.

This is part of changes described in #14486, to improve round-trip failures that occur in lowering.

@tvm-bot
Copy link
Collaborator

tvm-bot commented Apr 4, 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.

Generated by tvm-bot

@Lunderberg
Copy link
Contributor Author

At some point, it may make sense to have PointerType require that its value is not a DataType::Handle(), to prevent this ambiguity from occurring in the first place. However, introducing that TIR requirement was a larger change than the parser.

@Lunderberg Lunderberg force-pushed the tvmscript_handle_vs_voidptr branch from 07dfd61 to cbd086b Compare April 4, 2023 16:43
@Lunderberg Lunderberg force-pushed the tvmscript_handle_vs_voidptr branch from cbd086b to ad97955 Compare April 4, 2023 17:40
@junrushao
Copy link
Member

Yeah I made the change a while ago to unify T.Ptr and T.handle, and at that time i don't actually know a case where void* are different, so I decided to unify them anyways. I probably don't have much context. Perhaps would you like to give an example? Thanks a lot!!

@Lunderberg
Copy link
Contributor Author

Certainly! This occurs in any PrimFunc that is after tir.MakePackedAPI in the lowering flow, as the out_ret_value is annotated with a PointerType(PrimType(DataType::Void())) (link). I don't think this has an impact on the generated code, but it does mean that we're producing TIR that can't be expressed in TVMScript, and can't use TVMScript for writing unit tests for this pass.

@tqchen
Copy link
Member

tqchen commented Apr 4, 2023

The original intention there is to make the v_out_ret_value an opaque ptr. Perhaps we could simply mark it as handle as well? This would simplify our overall possibility without introducing void*

@junrushao
Copy link
Member

I do think allow possibility to express two different handle types will give developers more opportunities, so I would prefer merging this PR in, as well as updating MakePackedAPI. That would be nice if we could document a bit more about in Handle the difference between PrimType and PointerType of void :-)

@Lunderberg
Copy link
Contributor Author

From some quick tests, either representation could be removed without much difficulty. If we were getting rid of one of the two representations, I'd lean toward keeping the PointerType(...) version, since it is also needed to express a non-global scope.

That said, having a distinction between a pointer to a value and an opaque handle is probably a good one to keep around, so I'd lean toward @junrushao's suggestion.

@tqchen
Copy link
Member

tqchen commented Apr 4, 2023

happy to go with @junrushao @Lunderberg what you suggest

* \param is_unknown_type Used to distinguish between
* `PrimType(DataType::Handle())` and
* `PointerType(PrimType(DataType::Void()))`. If true, resolve dtype
* of `Void()` as `PrimType`, and if false resolve dtype of `Void()`
Copy link
Contributor

Choose a reason for hiding this comment

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

Void is defined as DataType(kHandle, 0, 0), while a legitimate handle has non-zero bits and lanes. Do we need an additional parameter here instead of passing the right thing in dtype?

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 believe so, because the dtype is providing the type of the pointed-to object, not the type of the pointer itself. While we could special-case passing of a DataType(kHandle, 0, 0) to produce PrimType(DataType::Handle()), that would give unexpected results if somebody tries to write a pointer-to-pointer as T.handle("handle") and accidentally hits the special-case.

@Lunderberg
Copy link
Contributor Author

@tvm-bot rerun

Copy link
Member

@junrushao junrushao left a comment

Choose a reason for hiding this comment

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

Let's get it in!

@Lunderberg Lunderberg merged commit 6ef73e0 into apache:main Apr 10, 2023
@Lunderberg Lunderberg deleted the tvmscript_handle_vs_voidptr branch April 10, 2023 21:15
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.

5 participants