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

[Frontend][TFlite] Add parser support for relu6, leaky_relu, relu_n1_to_1, log_softmax #4805

Merged
merged 3 commits into from
Jun 16, 2020

Conversation

inadob
Copy link
Contributor

@inadob inadob commented Feb 3, 2020

This patch is made on top of #4789.

  • Add helper functions in the parser to quantize and dequantize with Relay
  • Add qnn tests for all 4 activation functions

@inadob inadob changed the title [Frontend][TFlite] Add parser support for relu6, leeaky_relu, relu_n1_to_1, log_softmax [Frontend][TFlite] Add parser support for relu6, leaky_relu, relu_n1_to_1, log_softmax Feb 3, 2020
Copy link
Contributor

@anijain2305 anijain2305 left a comment

Choose a reason for hiding this comment

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

Relu and Clip implementation does not look right.

We can keep the computation in integer domain. The way to do that is to subtract the input zero point, and then call Relu, then requantize to the output scale (only if output scale/zero point are different from input scale/zero point).

return out

def convert_leaky_relu(self, op):
"""Convert TFLite LEAKY_RELU"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Not important, I think it should be """Convert TFLite Leaky_ReLU""" to align with """One iteration of Leaky_ReLU""".

return out

def convert_relu_n1_to_1(self, op):
"""Convert TFLite RELU_N1_TO_1"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Not important, I think it should be """Convert TFLite ReLU_n1_to_1""" to align with """One iteration of ReLU_n1_to_1""".

@tqchen tqchen added the status: need update need update based on feedbacks label Feb 12, 2020
@tqchen
Copy link
Member

tqchen commented Feb 26, 2020

@inadob please followup to address the review comments :)

@inadob
Copy link
Contributor Author

inadob commented Feb 28, 2020

Relu and Clip implementation does not look right.

We can keep the computation in integer domain. The way to do that is to subtract the input zero point, and then call Relu, then requantize to the output scale (only if output scale/zero point are different from input scale/zero point).

That's fine for the standard ReLU op but I am not entirely sure whether we can follow this logic when we apply ReLU6 and ReLU1. Is there a way to recreate them in Relay without clip?.

What I did, in this case, was to shift the data by subtracting the zero point, do clip, shift back and finally requantize if needed. The problem here was that Clip() from RELAY_PASS_PATTERN needs the clipping range to be in double and I couldn't just fix this by casting to float64 (it seems double is not supported in TVM).

@anijain @u99127

@anijain2305
Copy link
Contributor

The problem here was that Clip() from RELAY_PASS_PATTERN needs the clipping range to be in double and I couldn't just fix this by casting to float64 (it seems double is not supported in TVM).

Can you show me code example here? I have typically used float numbers like 1.0 or 6.0 to work with a_min or a_max

@inadob
Copy link
Contributor Author

inadob commented Mar 5, 2020

Can you show me code example here? I have typically used float numbers like 1.0 or 6.0 to work with a_min or a_max

Expr ClipQnnCanonicalize(const Attrs& attrs, const Array<Expr>& new_args,
                               const Array<tvm::relay::Type>& arg_types) {
  CHECK_EQ(new_args.size(), 7);
  auto& input_tensor = new_args[0];
  auto& input_scale = new_args[1]; // in fp
  auto& input_zero_point = new_args[2]; // in int32
  auto& clip_min = new_args[3];  // value is in fp
  auto& clip_max = new_args[4];  // value is in fp
  auto& output_scale = new_args[5];
  auto& output_zero_point = new_args[6];

  // Get the input dtype and shape.
  CHECK_EQ(arg_types.size(), 8);
  auto tensor_type = arg_types[0].as<TensorTypeNode>();
  CHECK(tensor_type != nullptr);
  auto input_dtype = tensor_type->dtype;
  auto input_shape = tensor_type->shape;


  // shift the input by subtracting the input zero_point
  auto shifted_input = Subtract(Cast(input_tensor, DataType::Int(32)), input_zero_point);

  // do the clipping in int32
  // auto clipped_tensor = Clip(shifted_input, clip_min, clip_max)
  auto clipped_tensor = Clip(shifted_input, Cast(clip_min, DataType::Float(64)), Cast(clip_max, DataType::Float(64)))
  // shift the input back by adding the zero_point
  clipped_tensor = Add(clipped_tensor, input_zero_point);

  // requantize the output if needed
  auto requantized_output = clipped_tensor;
  if (!IsEqualScalar(input_scale, output_scale) ||
      !IsEqualScalar(input_zero_point, output_zero_point)) {
     requantized_output = Requantize(clipped_tensor, input_shape, input_scale, input_zero_point, output_scale,
                                     output_zero_point, DataType::Int(32));
    }

  // Go back to lower precision.
  auto q_min = GetQmin(input_dtype);
  auto q_max = GetQmax(input_dtype);
  requantized_output = Clip(requantized_output, q_min, q_max); 
  return Cast(requantized_output, input_dtype);
}

And I am getting a complaint about double dtype...

/workspace/src/relay/qnn/op/clip.cc:61:117: error: cannot convert 'tvm::relay::Expr {aka tvm::RelayExpr}' to 'double' for argument '2' to 'tvm::relay::Expr tvm::relay::Clip(tvm::relay::Expr, double, double)'
   auto clipped_tensor = Clip(shifted_input, Cast(clip_min, DataType::Float(64)), Cast(clip_max, DataType::Float(64)))

The commented out line where I directly do clip() without casting to float64 didn't work too.

@tqchen
Copy link
Member

tqchen commented Mar 30, 2020

@anijain2305 @inadob please followup :)

@tqchen
Copy link
Member

tqchen commented Apr 24, 2020

ping

@inadob
Copy link
Contributor Author

inadob commented Apr 28, 2020

@anijain can you please help me with the quantized operators

@anijain2305
Copy link
Contributor

anijain2305 commented May 5, 2020

Sorry for the delay. @inadob Can you take a look at this PR - #5479

I have added support for fused quantized activations there. We dont need to create a new op in C++ from scratch. It is pretty easy to do the same thing in python. Let me know what you think

@anijain2305
Copy link
Contributor

Ping @inadob Let us know if you are working on this. Or else I can take a chance at this.

@inadob
Copy link
Contributor Author

inadob commented Jun 1, 2020

Ping @inadob Let us know if you are working on this. Or else I can take a chance at this.

@anijain2305 Yes, I am working on it - I will be ready in the next few days. Sorry for the delay

@anijain2305
Copy link
Contributor

@inadob Your changes look better now. Can you please rebase? (git submodule update --init --recursive)

@anijain2305
Copy link
Contributor

It seems you still have old `3rdparty/dmlc-core'. You can check that by clicking on "Files changed" tab.

@inadob
Copy link
Contributor Author

inadob commented Jun 5, 2020

It seems you still have old `3rdparty/dmlc-core'. You can check that by clicking on "Files changed" tab.

It's fixed now.

@anijain2305
Copy link
Contributor

@inadob Can you please fix the CI error?

@anijain2305
Copy link
Contributor

Ping @inadob :)

@tqchen
Copy link
Member

tqchen commented Jun 12, 2020

Glad that it is moving, @anijain2305 please manage this PR

…g_softmax

* add implementation in parser
* add qnn tests for each operator
* add 'clip' as in the quantized fused operations
* remove redundant assertions and imports
@inadob inadob force-pushed the activation branch 2 times, most recently from 4781b9e to 126361c Compare June 13, 2020 16:45
@inadob
Copy link
Contributor Author

inadob commented Jun 16, 2020

@anijain2305
CI has been fixed :)

@anijain2305 anijain2305 merged commit 52bf113 into apache:master Jun 16, 2020
@anijain2305
Copy link
Contributor

Thanks @inadob This is merged!

@tqchen tqchen added status: accepted and removed status: need update need update based on feedbacks labels Jun 16, 2020
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Jun 30, 2020
…to_1, log_softmax (apache#4805)

* [Frontend][TFLite]Add support for relu6, leaky_relu, relu_n1_to_1, log_softmax

* add implementation in parser
* add qnn tests for each operator

* Implement clip operation for quantized relu6, relu1

* add 'clip' as in the quantized fused operations
* remove redundant assertions and imports

* Fix floating value quantization for RELU6 and RELU1
zhiics pushed a commit to neo-ai/tvm that referenced this pull request Jul 2, 2020
…to_1, log_softmax (apache#4805)

* [Frontend][TFLite]Add support for relu6, leaky_relu, relu_n1_to_1, log_softmax

* add implementation in parser
* add qnn tests for each operator

* Implement clip operation for quantized relu6, relu1

* add 'clip' as in the quantized fused operations
* remove redundant assertions and imports

* Fix floating value quantization for RELU6 and RELU1
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.

5 participants