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

[QNN] Concatenate operator #3730

Merged
merged 1 commit into from
Aug 14, 2019
Merged

Conversation

anijain2305
Copy link
Contributor

@FrozenGene @u99127 @jackwish @tqchen

Pinging for review. The code changes will be very small after we have requantize merged in.

@zhenhuaw-me
Copy link
Contributor

Thank you @anijain2305 , please remind me when this PR rebased to a merged requantize head :)

@anijain2305 anijain2305 force-pushed the qnn_concatenate branch 2 times, most recently from 43be829 to b232102 Compare August 8, 2019 23:46
@anijain2305
Copy link
Contributor Author

@u99127 @jackwish This is now ready for review.

Copy link
Contributor

@zhenhuaw-me zhenhuaw-me left a comment

Choose a reason for hiding this comment

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

LGTM.

As I am not community reviewer, you may need someone else too approve, my message is only comment. :)

@anijain2305
Copy link
Contributor Author

@u99127 @tqchen Please review as you get time.

@anijain2305
Copy link
Contributor Author

@u99127 @ZihengJiang @vinx13 Please review. This (hopefully) should be quick.

@vinx13 vinx13 merged commit f06ef4f into apache:master Aug 14, 2019

# Find the dtype of the input expr. This is required for the requantize op. Since, this is
# concatenate op, the dtype of the input is same as dtype of the output.
data0 = relay.transform.infer_type(data[0])
Copy link
Member

@vinx13 vinx13 Aug 15, 2019

Choose a reason for hiding this comment

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

@anijain2305 This API has been removed in favor of InferType, please update and send another PR. Also I'm considering that requantize can by default use same output dtype if null is provided, so that we can avoid type inference here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just added a PR for fixing InferType. I think it missed CI because CI passed 3 days ago and InferType changes happened after that.

Regarding requantize, it makes sense. Let me think little more tomorrow and finalize this.

@ZihengJiang
Copy link
Contributor

ZihengJiang commented Aug 15, 2019

It seems the new added tests will raise error.

http://ci.tvm.ai:8080/blue/organizations/jenkins/tvm/detail/PR-3762/5/pipeline
http://ci.tvm.ai:8080/blue/organizations/jenkins/tvm/detail/master/1448/pipeline

Since this breaks CI for other PRs, could you raise a PR to fix it soon? @anijain2305

ZihengJiang added a commit that referenced this pull request Aug 15, 2019
@anijain2305
Copy link
Contributor Author

anijain2305 commented Aug 15, 2019

@ZihengJiang @vinx13 Added the following quick fix for InferType

#3779

wweic pushed a commit to neo-ai/tvm that referenced this pull request Aug 16, 2019
wweic pushed a commit to neo-ai/tvm that referenced this pull request Sep 6, 2019
@anijain2305 anijain2305 deleted the qnn_concatenate branch November 13, 2019 00:30
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.

4 participants