Skip to content

[RFC][Quantization] Designing and lowering of quantized ops - #3512

Closed
shoubhik wants to merge 9 commits into
apache:masterfrom
anijain2305:quantize-dequantize
Closed

[RFC][Quantization] Designing and lowering of quantized ops#3512
shoubhik wants to merge 9 commits into
apache:masterfrom
anijain2305:quantize-dequantize

Conversation

@shoubhik

@shoubhikshoubhik commented Jul 8, 2019

Copy link
Copy Markdown
Contributor

[Relay] [Quantization] WIP - This is the continuation of pull request #3367

In this PR I want to discuss the design and implementation of the

  • Quantize op -> FP32 to i8/u8
  • Dequantize Op -> i8/u8 -> fp32

I have added test cases to verify the correctness of the ops.

shoubhikbhatti@gmail.com added 5 commits July 8, 2019 12:12
shoubhikbhatti@gmail.com
[Relay] [Quantization] WIP - Common files for the qauntization work.
8d9e317
shoubhikbhatti@gmail.com
[Relay] [Quantization] WIP - Adding the tests file.
7081694
shoubhikbhatti@gmail.com
[Relay] [Quantization] WIP - This is the continuation of pull request […
c089ebc
…apache#3367](apache#3367)
In this PR I want to discuss the design and implementation of the
- Quantize op -> FP32 to i8/u8
- Dequantize Op -> i8/u8 -> fp32
I have added test cases to verify the correctness of the ops.
shoubhikbhatti@gmail.com
[Relay] [Quantization] Removing redundant code.
bcf003b
shoubhikbhatti@gmail.com
[Relay] [Quantization]
6766af9
1. Correcting docs
2. Reordering Clip and Cast in the dequantize op for stability.
@shoubhikshoubhik changed the title Quantize dequantize op[RFC][Quantization] Designing and lowering of quantized opsJul 8, 2019
@shoubhikshoubhik mentioned this pull request Jul 8, 2019
Closed
@shoubhik

shoubhik commented Jul 8, 2019

Copy link
Copy Markdown
ContributorAuthor

@tqchen@FrozenGene@ZihengJiang@zhiics@wweic@eqy

@zhiics

zhiics commented Jul 8, 2019

Copy link
Copy Markdown
Member

Is this ready for review? Have we been converged on the design in the quantization RFC?

@anijain2305

anijain2305 commented Jul 8, 2019

Copy link
Copy Markdown
Contributor

We have made good progress on the Quantization RFC, achieving clarity and convergence on many points.
For this PR specifically, @tqchen and @FrozenGene, can you please comment if this looks in line with our quantization RFC.

FrozenGene
FrozenGene reviewed Jul 9, 2019
View reviewed changes
Comment threadsrc/relay/pass/quantize_rewrite.cc
const auto scale = MakeConstantScalar(Float(32), attrs->output_scale);
const int32_t min_val = get_qmin(out_dtype);
const int32_t max_val = get_qmax(out_dtype);
auto scale_data = Cast(Round(Divide(data, scale)), Int(32));

@FrozenGeneFrozenGeneJul 9, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As discussed in the RFC, this should be round_away_from_zero, i.e. std::round in C++. How about this Round here? I don't see definition of this Round.

@anijain2305anijain2305Jul 9, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is Relay Round operator which translates to LLVM round intrinsic

https://llvm.org/docs/LangRef.html#llvm-round-intrinsic

The comment on the above link says that it has same behavior as LIBM round function which is defined here
https://sourceware.org/newlib/libm.html#lround

This is round-away-from-zero.

@FrozenGeneFrozenGeneJul 12, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ok. That's make sense.

@shoubhik

shoubhik commented Jul 11, 2019

Copy link
Copy Markdown
ContributorAuthor

@FrozenGene and @tqchen, any other major comments for the PR?

liangfu
liangfu reviewed Jul 12, 2019
View reviewed changes
Comment threadpython/tvm/relay/quantize/rewrite.py Outdated
liangfu
liangfu reviewed Jul 12, 2019
View reviewed changes
Comment threadsrc/relay/op/nn/quantize_op.cc Outdated
liangfu
liangfu reviewed Jul 12, 2019
View reviewed changes
Comment threadinclude/tvm/relay/quantize_util.h Outdated
liangfu
liangfu reviewed Jul 12, 2019
View reviewed changes
Comment threadpython/tvm/relay/op/qnn/qnn.py Outdated
liangfu
liangfu reviewed Jul 12, 2019
View reviewed changes
Comment threadinclude/tvm/relay/quantize_util.h Outdated
liangfu
liangfu reviewed Jul 12, 2019
View reviewed changes
Comment threadpython/tvm/relay/op/qnn/__init__.py Outdated
liangfu
liangfu reviewed Jul 12, 2019
View reviewed changes
Comment threadsrc/relay/op/nn/quantize_op.cc Outdated
@tqchen

tqchen commented Jul 12, 2019

Copy link
Copy Markdown
Member

Mainly organizational issues, please make things consistent with what was discussed in #3531

shoubhikbhatti@gmail.com added 3 commits July 12, 2019 11:34
shoubhikbhatti@gmail.com
[Relay] [Quantization]
186af5a
1. Correcting the file paths as suggested in the reviews.
shoubhikbhatti@gmail.com
Adding the common python files.
e83704b
shoubhikbhatti@gmail.com
1. Fixing the namespace for the qnn dialect.
3e4ad90
2. Fixing lint issues.
@shoubhik

shoubhik commented Jul 12, 2019

Copy link
Copy Markdown
ContributorAuthor

Mainly organizational issues, please make things consistent with what was discussed in #3531

I have addressed the namespace issues and have followed the same convetion as #3531 in the new commit.

@shoubhikshoubhik mentioned this pull request Jul 12, 2019
Merged
liangfu
liangfu requested changes Jul 15, 2019
View reviewed changes
Comment threadpython/tvm/relay/qnn/ir_pass.py Outdated
Comment threadpython/tvm/relay/qnn/op/__init__.py Outdated
Comment threadpython/tvm/relay/qnn/__init__.py Outdated
Comment threadpython/tvm/relay/qnn/_qnn.py Outdated
Comment threadpython/tvm/relay/qnn/op/_make.py Outdated
Comment threadpython/tvm/relay/qnn/op/qnn.py Outdated
@shoubhik

shoubhik commented Jul 17, 2019

Copy link
Copy Markdown
ContributorAuthor

@liangfu made the changes you suggested.

liangfu
liangfu approved these changes Jul 18, 2019
View reviewed changes

@liangfuliangfu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@anijain2305anijain2305 mentioned this pull request Jul 19, 2019
Closed
zhenhuaw-me
zhenhuaw-me suggested changes Jul 24, 2019
View reviewed changes

@zhenhuaw-mezhenhuaw-me left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Have not check lowering code. Basically two things:

  1. Is Quantize and Dequantize handle data type of 16 bits, regarding PR #3531 ?
  2. This PR review may need to suspend until PR #3531 were merged, as there is so many rebase task to do.

Comment threadinclude/tvm/relay/qnn/attrs.h
DataType out_dtype;

TVM_DECLARE_ATTRS(QuantizeAttrs, "relay.attrs.QuantizeAttrs") {
TVM_ATTR_FIELD(out_dtype)

@zhenhuaw-mezhenhuaw-meJul 24, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I have seen that PR #3531 accepts quantized tensor data type of 8/16 bit, are we going to align?

Comment threadsrc/relay/qnn/util.h
namespace tvm {
namespace relay {

inline bool IsInt8(const DataType& dtype) {

@zhenhuaw-mezhenhuaw-meJul 24, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

hmmmmmm, do we really need such utils?

Comment threadsrc/relay/qnn/util.h
return dtype == Float(32);
}

inline bool IsQuantizedType(const DataType& dtype) {

@zhenhuaw-mezhenhuaw-meJul 24, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Conflict naming against PR #3531 (also in many other files). Should we pending this review until #3531 is merged?

@shoubhikshoubhikJul 25, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

yes i am waiting for #3531 to get merged. it seems to be in flux for now. once it is merged i will make the changes in this one.

Comment threadsrc/relay/qnn/util.h
case QuantizeOpType::Quantize:
return IsFloat32(in_dtype);
case QuantizeOpType ::Dequantize:
return IsQuantizedType(in_dtype);

@zhenhuaw-mezhenhuaw-meJul 24, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The type check is against your api definition.

Comment threadsrc/relay/qnn/util.h
const DataType& in_dtype) {
switch (op_type) {
case QuantizeOpType::Quantize:
return IsQuantizedType(in_dtype);

@zhenhuaw-mezhenhuaw-meJul 24, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The type check is against your api definition. Alos @anijain2305 , do we need to align?

I think, if someone defines an OpType and conversions, it his responsible to include that code in one unified PR. Otherwise, don't include the OpType definition or conversions.

@shoubhikshoubhikJul 25, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

i agree, lets finish #3531 and then i will update this one.

Comment threadsrc/relay/qnn/util.h
}
}

inline const int32_t GetQmin(const DataType& dtype) {

@zhenhuaw-mezhenhuaw-meJul 24, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The implementation seems against PR #3531, of which the code is more simple.

Comment threadsrc/relay/qnn/util.h
} else if (IsUint32(dtype)) {
return std::numeric_limits<uint32_t>::min();
}
LOG(FATAL) << "Type not supported\n";

@zhenhuaw-mezhenhuaw-meJul 24, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe include in an else section.

Comment threadsrc/relay/qnn/util.h
return -1;
}

inline const int32_t GetQmax(const DataType& dtype) {

@zhenhuaw-mezhenhuaw-meJul 24, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same as GetQmin().

@zhenhuaw-me

zhenhuaw-me commented Jul 25, 2019

Copy link
Copy Markdown
Contributor

Regarding 16/32 bits quantization, I have a discussion in RFC #3591 .

@shoubhik

shoubhik commented Jul 25, 2019

Copy link
Copy Markdown
ContributorAuthor

There are quite a lot of changes here that are depndent on #3531 . I am closing the PR for now. I will reopen this once #3531 is pushed.

@anijain2305
anijain2305 deleted the quantize-dequantize branch November 13, 2019 00:30
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@FrozenGeneFrozenGeneFrozenGene left review comments
@anijain2305anijain2305anijain2305 left review comments
@liangfuliangfuliangfu approved these changes
+1 more reviewer
@zhenhuaw-mezhenhuaw-mezhenhuaw-me requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@shoubhik@zhiics@anijain2305@tqchen@zhenhuaw-me@liangfu@FrozenGene