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

[REFACTOR][IR] Move to runtime::String #5276

Merged
merged 4 commits into from
Apr 10, 2020
Merged

[REFACTOR][IR] Move to runtime::String #5276

merged 4 commits into from
Apr 10, 2020

Conversation

zhiics
Copy link
Member

@zhiics zhiics commented Apr 8, 2020

@zhiics
Copy link
Member Author

zhiics commented Apr 8, 2020

@tqchen One problem of replacing the default container StringImm with runtime String is that some of the IR nodes are using PrimExpr which could be both StringImm or IntImm (they are implicitly converted from std::string, integers, or floats). For example,

https://github.com/apache/incubator-tvm/blob/89da63e228eae2b0b4fe39770031a042858c52a7/include/tvm/tir/expr.h#L707

and

https://github.com/apache/incubator-tvm/blob/89da63e228eae2b0b4fe39770031a042858c52a7/include/tvm/tir/stmt.h#L147-L149
This would fail if we pass string and default it to runtime String, i.e.

https://github.com/apache/incubator-tvm/blob/89da63e228eae2b0b4fe39770031a042858c52a7/python/tvm/tir/expr.py#L977

Should we explicitly convert it from runtime String to StringImm in this case either before the constructor in Python or in the PackedFunc in the C++? This looks pretty ugly. Could you suggest some better solutions?

@tqchen
Copy link
Member

tqchen commented Apr 8, 2020

In the case of IRnodes that need StringImm, I think we can do explicit conversation for now

python/tvm/runtime/_ffi_container_api.py Outdated Show resolved Hide resolved
*/
TVM_DLL PrimExpr(std::string str); // NOLINT(*)
TVM_DLL PrimExpr(runtime::String value); // NOLINT(*)
Copy link
Member

Choose a reason for hiding this comment

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

Can we simply remove this constructor and explicily construct StringImm in the places that we need them

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I consider that as well, but it seems there are too many places using PrimExpr. Need sometime to see which one may take string.

Copy link
Member Author

@zhiics zhiics Apr 9, 2020

Choose a reason for hiding this comment

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

Considering the number of files changed so far, I'd like to keep this for now. It seems a bit more work. I can send a separate PR to handle it.

src/relay/backend/graph_runtime_codegen.cc Outdated Show resolved Hide resolved
src/relay/transforms/device_annotation.cc Outdated Show resolved Hide resolved
src/node/serialization.cc Outdated Show resolved Hide resolved
@zhiics zhiics force-pushed the string branch 3 times, most recently from 6c0ba1a to 18de508 Compare April 9, 2020 20:03
@zhiics zhiics marked this pull request as ready for review April 10, 2020 05:45
@tqchen tqchen merged commit 5da361d into apache:master Apr 10, 2020
@tqchen
Copy link
Member

tqchen commented Apr 10, 2020

Thanks @zhiics !

@zhiics zhiics deleted the string branch April 10, 2020 15:36
@tqchen tqchen mentioned this pull request Apr 10, 2020
2 tasks
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Apr 16, 2020
* Use runtime::String

* move string to tvm namespace

* add const char* constructor

* implicit cast from std::string
zhiics added a commit to neo-ai/tvm that referenced this pull request Apr 17, 2020
* Use runtime::String

* move string to tvm namespace

* add const char* constructor

* implicit cast from std::string
dpankratz pushed a commit to dpankratz/incubator-tvm that referenced this pull request Apr 24, 2020
* Use runtime::String

* move string to tvm namespace

* add const char* constructor

* implicit cast from std::string
monklof pushed a commit to monklof/incubator-tvm that referenced this pull request Jan 22, 2021
…m_data:master to master

* commit 'cd0d52daa6942bdafa9363ff6cfa3d25fcd5b8d6': (824 commits)
  [Intrinsic] Add log1p, ldexp, atan2, hypot, nextafter, copysign (apache#5312)
  [Rust][CI] Restore Rust CI (apache#5137)
  Remove PrimExpr from String (apache#5311)
  [Requantize] Cleanup and Optimize Lowering (apache#5286)
  [IR][TRANSFORM] Enable CopyOnWrite for passes. (apache#5309)
  [PYTORCH]Abs, Arange, Softplus ops (apache#5295)
  [LLVM] Fix generation of LLVM intrinsics (apache#5282)
  [BYOC] Add example of Composite + Annotate for DNNL fused op (apache#5272)
  [Frontend][TensorFlow]Improve TensorFlow Static Shape Tensor Array (apache#5243)
  [RUNTIME] Introduce RValue reference(move) support to TypedPackedFunc (apache#5271)
  [RELAY][FRONTEND][CAFFE2] add Mul and ConvTranspose operator (apache#5302)
  [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes (apache#5277)
  [CI] Fix the hexagon string (apache#5304)
  [Arith] linear system and equation solver (apache#5171)
  [PYTORCH]Repeat, Reciprocal & Reshape Op support (apache#5280)
  [FRONTEND][TENSORFLOW] Fix gather_nd indices (apache#5279)
  Update device_annotation.cc (apache#5291)
  [REFACTOR][IR] Move to runtime::String (apache#5276)
  [NDArray] Set shape_ in NDArray::FromDLPack (apache#5301)
  [RUNTIME] Initial implementation of Hexagon runtime support (apache#5252)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants