-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
dequantize matmul and matmul_v2 Y weights in quant2_int8 #37618
dequantize matmul and matmul_v2 Y weights in quant2_int8 #37618
Conversation
Thanks for your contribution! |
@@ -336,6 +338,9 @@ def _is_int8_weights(op_node, weight_name): | |||
self._dequantize_op_weights(graph, op, "Filter", "Output") | |||
elif op.name() in self._mul_ops and _is_int8_weights(op, "Y"): | |||
self._dequantize_op_weights(graph, op, "Y", "Out") | |||
elif op.name() in self._matmul_ops and _is_int8_weights(op, "Y"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like you can combine this part into one and just check op.name() in [self._mul_ops, self._matmul_ops]
. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can apply your idea, but I will concatenate those lists before for loop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we even need matmul_ops variable separate from mul_ops
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, it looks like we are doing exactly the same things for them, so for me you can combine it.
@baoachun We have big concern about matmul_v2 and related passes. Let's discuss it when you got time
|
I'm sorry I don't understand what you mean very much. What is the problem now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…e#37618) * dequantize matmul and matmul_v2 Y weights in qat2_int8 * review fix * split conv and mul tests, add matmul test * fixup * fix ci build * remove unused variables * formatting fix * remove extra newline at end of file
PR types
Bug fixes
PR changes
Others
Describe
fix for a problem in issue: #36962.
There, matmul_v2 has weights quantized but they are not dequantized during conversion of qat model to fp32 model. later matmul_v2 fp32 is used with those quantized weights.
I separated also conv2d and matmul unit tests for quant2_int8_mkldnn_pass