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

[Relay][Pass] SimplifyCastLike/Cast and ConcretizeFullLikeRewrite rewrites for SimplifyExpr #7827

Merged
merged 4 commits into from
Apr 14, 2021

Conversation

hgt312
Copy link
Contributor

@hgt312 hgt312 commented Apr 12, 2021

  • ConcretizeFullLikeRewrite: full_like -> full when known shape
  • SimplifyCastLike/Cast: cast(data, dtype) or cast_like(data, dtype_like) -> data, when the input and output have the same dtype

@yzhliu @comaniac @altanh

Copy link
Contributor

@comaniac comaniac left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

/*!
* \brief SimplifyCast matches the pattern of cast data to the same dtype.
*/
class SimplifyCast : public DFPatternRewrite {
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like these two patterns be merged to one?

pattern_ = IsOp("cast_like")({data_pat_, like_pat_}) || IsOp("cast")({data_pat_});

And both patterns can use like_ty = pre->checked_type().as<TensorTypeNode>();.

Copy link
Contributor

@altanh altanh left a comment

Choose a reason for hiding this comment

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

same comment as @comaniac , otherwise LGTM

@@ -300,6 +300,7 @@ class RelayBuildModule : public runtime::ModuleNode {
}
});
pass_seqs.push_back(transform::EliminateCommonSubexpr(fskip));
pass_seqs.push_back(transform::InferType());
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need this? InferType is already in the requirement of SimplifyExpr. Other existing patterns also refer to checked_type_ so it should be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Emmm... I dont know why Ci fails, just try it

Copy link
Contributor

Choose a reason for hiding this comment

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

From the CI it does look like the type is missing. Since InferType is executed before and after SimplifyExpr, one possibility is your data is already mutated by previous patterns in this pass and that callback didn't manually specify the type.

Copy link
Contributor

@comaniac comaniac left a comment

Choose a reason for hiding this comment

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

LGTM

@comaniac comaniac merged commit 1e9c1bf into apache:main Apr 14, 2021
@comaniac
Copy link
Contributor

Thanks @hgt312 @altanh

trevor-m pushed a commit to trevor-m/tvm that referenced this pull request May 6, 2021
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request May 6, 2021
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request May 6, 2021
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request May 11, 2021
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