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

Use std::optional instead of dmlc::optional, NFC #12443

Merged
merged 5 commits into from
Aug 16, 2022
Merged

Use std::optional instead of dmlc::optional, NFC #12443

merged 5 commits into from
Aug 16, 2022

Conversation

kparzysz-quic
Copy link
Contributor

No description provided.

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.

LGTM. BTW, do you prefer using tvm::runtime::Optinonal instead for TVM objects?

Krzysztof Parzyszek added 2 commits August 15, 2022 17:42
Otherwise std::optional<T>::value() is "unavailable"...
@kparzysz-quic
Copy link
Contributor Author

kparzysz-quic commented Aug 15, 2022

LGTM. BTW, do you prefer using tvm::runtime::Optinonal instead for TVM objects?

I think that tvm::runtime::Optional is a part of the packed func interface, so I'm not sure if we can get rid of it.

Edit: I prefer to use existing library classes and functions in general.

@junrushao
Copy link
Member

Yeah my question is definitely a bit off the topic, so we don't have to deal with this in this PR - feel free to consider this as a discussion instead.

My point here is that tvm::runtime::Optional<ObjectRef> is a simple wrapper of ObjectRef which does not incur extra memory overhead, so it could be slightly more efficient. Therefore, when dealing with TVM objects, I usually recommend to use tvm::runtime::Optional instead

@masahi
Copy link
Member

masahi commented Aug 16, 2022

How about we also clean up this?

* default-constructed. This can be replaced with std::optional with
* C++17 if/when C++17 is required.
*/
inline std::pair<bool, runtime::DataType> GetPointerType(const Type& type) {

@kparzysz-quic
Copy link
Contributor Author

How about we also clean up this?

Yes, I'll have another PR.

@kparzysz-quic kparzysz-quic merged commit bd56231 into apache:main Aug 16, 2022
@kparzysz-quic kparzysz-quic deleted the optional branch August 16, 2022 15:43
xinetzone pushed a commit to daobook/tvm that referenced this pull request Nov 25, 2022
* Use std::optional instead of dmlc::optional, NFC

* Fix linter

* Set deployment target to macOS 10.13

Otherwise std::optional<T>::value() is "unavailable"...

* Fix linter again

* Update Hexagon apps to use C++17 as the C++ standard
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.

3 participants