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

add onnxifi quantization support #2617

Merged
merged 4 commits into from Mar 29, 2019

Conversation

Projects
None yet
5 participants
@zrphercule
Copy link
Member

zrphercule commented Mar 28, 2019

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

This comment has been minimized.

Copy link
Contributor

yinghai commented Mar 28, 2019

Please fix your summary.

@@ -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;

This comment has been minimized.

Copy link
@rdzhabarov

rdzhabarov Mar 28, 2019

Contributor

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

@@ -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"

This comment has been minimized.

Copy link
@rdzhabarov

rdzhabarov Mar 28, 2019

Contributor

revert this

This comment has been minimized.

Copy link
@yinghai

yinghai Mar 29, 2019

Contributor

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

This comment has been minimized.

Copy link
@rdzhabarov

rdzhabarov Mar 29, 2019

Contributor

@zrphercule that was not reverted.

This comment has been minimized.

Copy link
@rdzhabarov

rdzhabarov Mar 29, 2019

Contributor

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

This comment has been minimized.

Copy link
Contributor

rdzhabarov commented Mar 28, 2019

Also, make tests passing ;)

@bertmaher
Copy link
Contributor

bertmaher left a comment

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

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

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

This comment has been minimized.

Copy link
@bertmaher

bertmaher Mar 28, 2019

Contributor

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

@bertmaher

This comment has been minimized.

Copy link
Contributor

bertmaher commented Mar 28, 2019

Huh, what's this in the CI builds:

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

This comment has been minimized.

Copy link
Contributor

yinghai commented Mar 28, 2019

Need to update Foxi tp

@zrphercule

This comment has been minimized.

Copy link
Member Author

zrphercule commented Mar 28, 2019

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

This comment has been minimized.

Copy link
Contributor

rdzhabarov commented Mar 28, 2019

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

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) {

This comment has been minimized.

Copy link
@rdzhabarov

rdzhabarov Mar 29, 2019

Contributor

why is this not boolean?

This comment has been minimized.

Copy link
@zrphercule

zrphercule Mar 29, 2019

Author Member

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

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

This comment has been minimized.

Copy link
@rdzhabarov

rdzhabarov Mar 29, 2019

Contributor

inDataSigned seems to be used later, why this?

This comment has been minimized.

Copy link
@zrphercule

zrphercule Mar 29, 2019

Author Member

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?

This comment has been minimized.

Copy link
@zrphercule

This comment has been minimized.

Copy link
@rdzhabarov

rdzhabarov Mar 29, 2019

Contributor

yes, please remove

@yinghai

This comment has been minimized.

Copy link
Contributor

yinghai commented Mar 29, 2019

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

@rdzhabarov
Copy link
Contributor

rdzhabarov left a comment

LGTM. Please, address pending comments.

}

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

This comment has been minimized.

Copy link
@rdzhabarov

rdzhabarov Mar 29, 2019

Contributor

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);

This comment has been minimized.

Copy link
@zrphercule

zrphercule Mar 29, 2019

Author Member

@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.

This comment has been minimized.

Copy link
@rdzhabarov

rdzhabarov Mar 29, 2019

Contributor

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

This comment has been minimized.

Copy link
@zrphercule

zrphercule Mar 29, 2019

Author Member

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?

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

This comment has been minimized.

Copy link
@rdzhabarov

rdzhabarov Mar 29, 2019

Contributor

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

This comment has been minimized.

Copy link
@zrphercule

zrphercule Mar 29, 2019

Author Member

I wont bother having one more line of credit lol

zrphercule added some commits Mar 29, 2019

@zrphercule

This comment has been minimized.

Copy link
Member Author

zrphercule commented Mar 29, 2019

Ready to go, anymore comments?

@rdzhabarov

This comment has been minimized.

Copy link
Contributor

rdzhabarov commented Mar 29, 2019

I'll merge once the format nit fixed.

@rdzhabarov
Copy link
Contributor

rdzhabarov left a comment

Looks great!

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

6 checks passed

ci/circleci: ASAN Your tests passed on CircleCI!
Details
ci/circleci: DEBUG Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_WITH_EXPENSIVE_TESTS Your tests passed on CircleCI!
Details
ci/circleci: SHARED Your tests passed on CircleCI!
Details
ci/circleci: TSAN Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@zrphercule zrphercule deleted the zrphercule:quantization_onnxifi branch Mar 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.