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] Concat - Refactoring to C++ #3819

Merged
merged 1 commit into from Aug 31, 2019
Merged

Conversation

anijain2305
Copy link
Contributor

@anijain2305 anijain2305 commented Aug 22, 2019

There are 2 reasons for moving to C++

  • With python interface earlier, we had to call infer_type in the op python definition. This broke the nice abstraction that we have with Relay and QNN ops.
  • Working with different targets, I realized that we need to write QNN passes, that will work on the abstraction of QNN ops. For example, Intel needs the conv to be uint8 x int8. However, TFLite graphs can be uint8 x uint8. This can resolved by writing a legalize pass for QNN conv2d op for Intel machines, that will insert a requantize on the weight matrix to go from uint8 x int8. With earlier python interface, the ops were already getting lowered to relay ops, before I could even run this pass.

Please review @yzhliu @jackwish @FrozenGene @vinx13 @tqchen @zhiics

@anijain2305
Copy link
Contributor Author

@vinx13 @zhiics @yzhliu Can you please review this?

Refactoring to C++ removes InferType dependency. Relevant comment - #3730 (comment)

@@ -415,6 +415,13 @@ static inline Expr Full(Expr fill_value,
return CallNode::make(op, {fill_value}, Attrs(attrs), {});
}

static inline Expr Concatenate(Expr data, int axis) {
Copy link
Member

Choose a reason for hiding this comment

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

this is actually the same as MakeConcatenate below isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup. Nice observation. Made the changes to use that.

src/relay/qnn/util.h Show resolved Hide resolved
Array<tvm::Expr> input_zero_points, double output_scale,
int32_t output_zero_point, int axis) {
auto attrs = make_node<QnnConcatenateAttrs>();
attrs->input_scales = input_scales;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
attrs->input_scales = input_scales;
attrs->input_scales = std::move(input_scales);

int32_t output_zero_point, int axis) {
auto attrs = make_node<QnnConcatenateAttrs>();
attrs->input_scales = input_scales;
attrs->input_zero_points = input_zero_points;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
attrs->input_zero_points = input_zero_points;
attrs->input_zero_points = std::move(input_zero_points);


/*
* \brief Canonicalizes the QNN concatenate op.
* \param ref_call The original call that will be lowered.
Copy link
Member

Choose a reason for hiding this comment

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

Names in comment mismatch with the arguments.

const Array<IndexExpr>& input_shape, const DataType& out_dtype);

static inline Expr Requantize(const Expr& data, const Array<IndexExpr>& input_shape,
const double& input_scale, const int32_t& input_zero_point,
Copy link
Member

Choose a reason for hiding this comment

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

no need to have const xx& for built-in types, just double input_scale, int32_t input_zero_point etc.

@anijain2305
Copy link
Contributor Author

Thanks @zhiics and @vinx13 for the comments. The changes are in.

Copy link
Member

@zhiics zhiics left a comment

Choose a reason for hiding this comment

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

lgtm

@zhiics zhiics merged commit ec7790e into apache:master Aug 31, 2019
@zhenhuaw-me zhenhuaw-me mentioned this pull request Sep 2, 2019
wweic pushed a commit to wweic/tvm that referenced this pull request Sep 16, 2019
wweic pushed a commit to wweic/tvm that referenced this pull request Sep 16, 2019
wweic pushed a commit to neo-ai/tvm that referenced this pull request Sep 16, 2019
@anijain2305 anijain2305 deleted the qnn_concat 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.

None yet

3 participants