add onnxifi quantization support #2617

zrphercule · 2019-03-28T21:54:59Z

Description:
This the onnxifi quantization support in glow side, we accept two quantized tensors, intQ32ty and int8Qty.
Testing:
Only internal test is supported now, using resnet50_quantized.

yinghai · 2019-03-28T21:56:40Z

Please fix your summary.

rdzhabarov · 2019-03-28T21:59:24Z

include/glow/Importer/CommonOperatorLoader.h

@@ -40,54 +40,66 @@ inline llvm::Error loadWeight(const onnxTensorDescriptorV1 &in, Tensor *T) {
    RETURN_ERR("Only support CPU memory tensors.");
  }

+  // This is a caffe2 offset shift.
+  const int32_t OFFSETSHIFT = 128;


would be good to have this in one place. C2ModelLoader.cpp has the same.

rdzhabarov · 2019-03-28T22:01:05Z

lib/Importer/Caffe2ModelLoader.cpp

@@ -1179,8 +1219,8 @@ llvm::Error Caffe2ModelLoader::loadWeight(const caffe2::OperatorDef &op) {
      arg {
      name: "values"
      s:
-      "\000\377\152\232\115\072\000\000\200\077\000\377\050\132\215\073\063\063\023\100\000\377\314\063\232\073\000\000\220\100"
-      }
+"\000\377\152\232\115\072\000\000\200\077\000\377\050\132\215\073\063\063\023\100\000\377\314\063\232\073\000\000\220\100"


revert this

We can be reverted by the clang format fix tool in Glow?

@zrphercule that was not reverted.

We can be reverted by the clang format fix tool in Glow?

it's inside a comment, not sure if it can be fixed by clang format.

rdzhabarov · 2019-03-28T23:10:42Z

Also, make tests passing ;)

bertmaher

Looking good to me, just needs to address @rdzhabarov's comments.

bertmaher · 2019-03-28T22:43:23Z

lib/Importer/Caffe2ModelLoader.cpp

@@ -1047,38 +1066,59 @@ llvm::Error Caffe2ModelLoader::loadOperator(const caffe2::OperatorDef &op) {

  RETURN_ERR(unexpectedNodeErrorMessage(op, "Unsupported operator."));
 }
+template <class TensorProtoType>


nitpick, add a blank line here before template to separate top-level defs.

bertmaher · 2019-03-28T23:13:50Z

Huh, what's this in the CI builds:

error: no member named 'bias' in 'onnxTensorDescriptorV1'

yinghai · 2019-03-28T23:14:57Z

Need to update Foxi tp

zrphercule · 2019-03-28T23:50:31Z

Huh, what's this in the CI builds:
error: no member named 'bias' in 'onnxTensorDescriptorV1'

Yeah because we need to update foxi first. foxi update has already been merged in OSS, we are pushing it in fbcode as well.

rdzhabarov · 2019-03-28T23:52:46Z

Yeah because we need to update foxi first. foxi update has already been merged in OSS, we are pushing it in fbcode as well.

You need to update sha of 'foxi' submodule used in glow to make it work

rdzhabarov · 2019-03-29T00:00:04Z

include/glow/Importer/CommonOperatorLoader.h

-    for (size_t i = 0; i < TH.size(); ++i) {
-      constexpr uint8_t OFFSETSHIFT = 128;
-      TH.raw(i) = static_cast<int8_t>((((uint8_t)data[i]) - OFFSETSHIFT));
+  if (in.is_quantized == 1) {


why is this not boolean?

@rdzhabarov because onnxifi is C API, and it is an uint8 instead of bool (Actually it is supposed to be char at first...)

rdzhabarov · 2019-03-29T00:07:43Z

include/glow/Importer/CommonOperatorLoader.h

+    } else if (in.dataType == ONNXIFI_DATATYPE_UINT64 ||
+               in.dataType == ONNXIFI_DATATYPE_INT64) {
+      const bool inDataSigned = in.dataType == ONNXIFI_DATATYPE_INT64;
+      (void)inDataSigned;


inDataSigned seems to be used later, why this?

hmmm that's interesting, I didnt write this, the change here is only lint problem.
But I am wondering as well, this seems can be removed, no objection?

@rdzhabarov

yes, please remove

yinghai · 2019-03-29T02:36:25Z

houseroad/foxi#9 is merged. Let's update tp in this PR.

rdzhabarov

LGTM. Please, address pending comments.

rdzhabarov · 2019-03-29T03:00:31Z

lib/Importer/Caffe2ModelLoader.cpp

+  }
+
+  if (in.data_type() == caffe2::TensorProto::UINT8) {
+    T->reset(ElemKind::Int8QTy, dim, in.scale(), in.bias() - OFFSETSHIFT);


what is the use case for what's checked in line 72?

else if (in.data_type() == caffe2::TensorProto::UINT8) { T->reset(ElemKind::Int8QTy, dim, 1.0, 0);

@rdzhabarov These are two different branches, in #72 we assume the incoming tensor is a non-quantized tensor, and only use Int8QTy to represent a int8 tensor. Here we know it is a quantized tensor (by knowing protobuf is a QTensorProto), we treated it like real quantized tensor.

linked PR seems to be unrelated.
What is the case when tensor is int8 and non quantized?

oh it is not the pr I want to link, it is the 72nd line in this file...
I think you have the point, right now glow takes all int8 input as Int8QTy. do we have normal int8ty as well?

rdzhabarov · 2019-03-29T03:03:49Z

lib/Importer/Caffe2ModelLoader.cpp

@@ -60,7 +60,6 @@ llvm::Error setTensorType(const caffe2::TensorProto &in, Tensor *T) {
    }
    dim.push_back(d);
  }
-


sorry, not your change, but
line 46 does not have a correct comment.
Could you fix [-128, 127] and <=255.

I wont bother having one more line of credit lol

zrphercule · 2019-03-29T17:56:37Z

Ready to go, anymore comments?

rdzhabarov · 2019-03-29T18:23:07Z

I'll merge once the format nit fixed.

rdzhabarov

Looks great!

add onnxifi quantization support

b204163

zrphercule requested review from yinghai, jackm321, bertmaher, nadavrot and rdzhabarov March 28, 2019 21:54

facebook-github-bot added the CLA Signed label Mar 28, 2019

rdzhabarov reviewed Mar 28, 2019

View reviewed changes

bertmaher approved these changes Mar 28, 2019

View reviewed changes

rdzhabarov reviewed Mar 29, 2019

View reviewed changes

rdzhabarov approved these changes Mar 29, 2019

View reviewed changes

zrphercule added 2 commits March 29, 2019 10:36

update foxi

1f39628

fix nits

27e8af0

format

2394244

rdzhabarov approved these changes Mar 29, 2019

View reviewed changes

rdzhabarov merged commit 036071a into pytorch:master Mar 29, 2019

zrphercule deleted the quantization_onnxifi branch March 29, 2019 22:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add onnxifi quantization support #2617

add onnxifi quantization support #2617

add onnxifi quantization support #2617

add onnxifi quantization support #2617

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment