You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2021/06/10 22:20:20 UTC
[GitHub] [tvm] mbrookhart opened a new pull request #8241: [WIP][Relay][Quantization] Extend FakeQuantizationToInteger to more ops
mbrookhart opened a new pull request #8241:
URL: https://github.com/apache/tvm/pull/8241
Adding more ops to support QAT BERT. I also refactored the tests for easier extensions.
I marked this as WIP because the result isn't matching tflite, I need to do some more comparisons over the next day or two to isolate the problem.
I'm opening the PR now because supporting ops with multiple outputs required refactoring how I handle quantization specific types. Bringing it into the make ir namespace allows me to do what I need to do, but it also opens up a question of where or not this could be more used more generally in other places for quantization?
cc @jroesch @masahi @anijain2305 @jwfromm
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on pull request #8241: [Relay][Quantization] Extend FakeQuantizationToInteger to more ops
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #8241:
URL: https://github.com/apache/tvm/pull/8241#issuecomment-891552733
Thanks @mbrookhart @jwfromm @elvin-n
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mbrookhart commented on pull request #8241: [Relay][Quantization] Extend FakeQuantizationToInteger to more ops
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on pull request #8241:
URL: https://github.com/apache/tvm/pull/8241#issuecomment-882732982
@jwfromm @masahi @jroesch @anijain2305 can you take a look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mbrookhart commented on pull request #8241: [Relay][Quantization] Extend FakeQuantizationToInteger to more ops
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on pull request #8241:
URL: https://github.com/apache/tvm/pull/8241#issuecomment-891543129
@elvin-n @jwfromm @masahi @anijain2305 This is finally green after the CI chaos last week, anyone want to take another look before merging?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] jwfromm commented on a change in pull request #8241: [Relay][Quantization] Extend FakeQuantizationToInteger to more ops
Posted by GitBox <gi...@apache.org>.
jwfromm commented on a change in pull request #8241:
URL: https://github.com/apache/tvm/pull/8241#discussion_r674992637
##########
File path: python/tvm/relay/transform/fake_quantization_to_integer.py
##########
@@ -163,4 +196,128 @@ def clip(expr, type_map):
amin = relay.op.round(relay.op.const(amin) / scale + z_p)
amax = relay.op.round(relay.op.const(amax) / scale + z_p)
out = relay.op.minimum(relay.op.maximum(arg, amin), amax)
- return [out, t.scale, t.zero_point, t.dtype]
+ return [out, t]
+
+
+@register_fake_quantization_to_integer("nn.pad")
+def pad(expr, type_map):
+ """Rewite an nn.pad op"""
+ arg = expr.args[0]
+ t = type_map[arg]
+ pad_value = expr.args[1]
+ if pad_value in type_map:
+ pad_t = type_map[pad_value]
+ if not tvm.ir.structural_equal(t, pad_t):
Review comment:
a comment explaining the logic here would be helpful.
##########
File path: src/relay/transforms/fake_quantization_to_integer.cc
##########
@@ -148,7 +93,8 @@ class SubgraphExtractor : public ExprVisitor {
const AffineTypeMap GetAffineTypes() { return affine_types_; }
void VisitExpr(const Expr& expr) override {
if (expr.as<CallNode>() == nullptr && expr.as<OpNode>() == nullptr &&
Review comment:
can you add a comment explaining this check and why it implies `fake_quantization = false`? It's a little confusing to me.
##########
File path: src/relay/transforms/fake_quantization_to_integer.cc
##########
@@ -226,18 +173,37 @@ class SubgraphMutator : public ExprMutator {
// Call the rewrite
Array<ObjectRef> vals = fqfq[op](expr, affine_types_);
// Save teh outputs of the rewrite
Review comment:
might as well fix this typo while adding comments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mbrookhart commented on a change in pull request #8241: [Relay][Quantization] Extend FakeQuantizationToInteger to more ops
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on a change in pull request #8241:
URL: https://github.com/apache/tvm/pull/8241#discussion_r672498519
##########
File path: python/tvm/relay/transform/fake_quantization_to_integer.py
##########
@@ -102,7 +104,7 @@ def bias_add(expr, type_map):
out_dtype=xt.dtype,
Review comment:
Thanks for finding this! I've fixed it and added a test case that hits it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mbrookhart commented on pull request #8241: [WIP][Relay][Quantization] Extend FakeQuantizationToInteger to more ops
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on pull request #8241:
URL: https://github.com/apache/tvm/pull/8241#issuecomment-880229186
Sorry for the delay on this. I've hit some very subtle accuracy bugs on a BERT model I was using that made me doubt this. I see slightly differences (1.5% rmse) on all comparisons on that model (tflite->onnxruntime, tflite->tvm, tflite->tvm + this, tvm->onnx, etc) I think that might a complication to that model in particular.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mbrookhart commented on a change in pull request #8241: [Relay][Quantization] Extend FakeQuantizationToInteger to more ops
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on a change in pull request #8241:
URL: https://github.com/apache/tvm/pull/8241#discussion_r672498519
##########
File path: python/tvm/relay/transform/fake_quantization_to_integer.py
##########
@@ -102,7 +104,7 @@ def bias_add(expr, type_map):
out_dtype=xt.dtype,
Review comment:
Thanks for finding this! I've fixed it and added a test case that hits it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mbrookhart commented on pull request #8241: [Relay][Quantization] Extend FakeQuantizationToInteger to more ops
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on pull request #8241:
URL: https://github.com/apache/tvm/pull/8241#issuecomment-882732982
@jwfromm @masahi @jroesch @anijain2305 can you take a look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mbrookhart commented on a change in pull request #8241: [Relay][Quantization] Extend FakeQuantizationToInteger to more ops
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on a change in pull request #8241:
URL: https://github.com/apache/tvm/pull/8241#discussion_r672498519
##########
File path: python/tvm/relay/transform/fake_quantization_to_integer.py
##########
@@ -102,7 +104,7 @@ def bias_add(expr, type_map):
out_dtype=xt.dtype,
Review comment:
Thanks for finding this! I've fixed it and added a test case that hits it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on pull request #8241: [WIP][Relay][Quantization] Extend FakeQuantizationToInteger to more ops
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #8241:
URL: https://github.com/apache/tvm/pull/8241#issuecomment-859121332
> I marked this as WIP because the result isn't matching tflite
How big is the difference? We should evaluate some accuracy metric on a whole dataset.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mbrookhart commented on pull request #8241: [Relay][Quantization] Extend FakeQuantizationToInteger to more ops
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on pull request #8241:
URL: https://github.com/apache/tvm/pull/8241#issuecomment-882732982
@jwfromm @masahi @jroesch @anijain2305 can you take a look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi merged pull request #8241: [Relay][Quantization] Extend FakeQuantizationToInteger to more ops
Posted by GitBox <gi...@apache.org>.
masahi merged pull request #8241:
URL: https://github.com/apache/tvm/pull/8241
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] elvin-n commented on a change in pull request #8241: [Relay][Quantization] Extend FakeQuantizationToInteger to more ops
Posted by GitBox <gi...@apache.org>.
elvin-n commented on a change in pull request #8241:
URL: https://github.com/apache/tvm/pull/8241#discussion_r671693301
##########
File path: python/tvm/relay/transform/fake_quantization_to_integer.py
##########
@@ -102,7 +104,7 @@ def bias_add(expr, type_map):
out_dtype=xt.dtype,
Review comment:
x_t instead of xt?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mbrookhart commented on pull request #8241: [WIP][Relay][Quantization] Extend FakeQuantizationToInteger to more ops
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on pull request #8241:
URL: https://github.com/apache/tvm/pull/8241#issuecomment-859140816
I've only tested random data so far, looking for a dataset I can test with tomorrow
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org