[quant][graphmode] Support propagate dequantize for nodes with multiple outputs#39551
[quant][graphmode] Support propagate dequantize for nodes with multiple outputs#39551jerryzh168 wants to merge 8 commits intogh/jerryzh168/335/basefrom
Conversation
…le outputs Summary: e.g. prim::ListUnpack Test Plan: Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]
…with multiple outputs" Summary: e.g. prim::ListUnpack Test Plan: Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]
💊 CI failures summary and remediationsAs of commit 1952a4e (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group. This comment has been revised 31 times. |
…with multiple outputs" Summary: e.g. prim::ListUnpack Test Plan: Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]
…with multiple outputs" Summary: e.g. prim::ListUnpack Test Plan: Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]
| return x + y | ||
| x = torch.randn([1, 3, 10, 10], dtype=torch.float) | ||
| x = torch.quantize_per_tensor(x, 0.5, 1, torch.quint8) | ||
| input = [x, x] |
There was a problem hiding this comment.
Should we also add a test for tracing here?
There was a problem hiding this comment.
I think we probably need people to expand all tests with tracing
There was a problem hiding this comment.
this one probably doesn't make much difference, but I can add that
| return x + y | ||
| x = torch.randn([1, 3, 10, 10], dtype=torch.float) | ||
| x = torch.quantize_per_tensor(x, 0.5, 1, torch.quint8) | ||
| input = [x, x] |
There was a problem hiding this comment.
Do both quantize_per_tensor and dequantize now support list of tensors?
There was a problem hiding this comment.
yeah that's correct: https://github.com/pytorch/pytorch/blob/master/aten/src/ATen/native/native_functions.yaml#L3580, but I think we don't really need the list version of quantize_per_tensor right now
| .check("dequantize") \ | ||
| .run(m.graph) | ||
| res = get_forward(m._c)(x) | ||
| res = m(input) |
There was a problem hiding this comment.
Wouldnt the listunpack op be needed prior to the maxpool and avgpool even after the swap dequantize pass?
There was a problem hiding this comment.
yeah, it's still there. I forgot to add the check for that
…with multiple outputs" Summary: e.g. prim::ListUnpack Test Plan: Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]
…with multiple outputs" Summary: e.g. prim::ListUnpack Test Plan: Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]
| } | ||
| } | ||
| } else { | ||
| // For this category of ops, we need to |
There was a problem hiding this comment.
What is the mechanism for warning/providing errors for quantization? In this case, it will be good to error out with a specific message.
if (n->outputs().size() > 1) {
// Factoring out dequantize for if blocks with multiple outputs // Factoring out dequantize for if blocks with multiple outputs
// is not supported right now // is not supported right now
continue; continue;
} }
There was a problem hiding this comment.
this should probably be a warning since we might not need this functionality when there are multiple outputs, and the result would be incorrect if we do need the support.
There was a problem hiding this comment.
In general when it's a clear case we don't support, we'll throw an error
| } | ||
| } | ||
| } else { | ||
| // For this category of ops, we need to |
There was a problem hiding this comment.
Also, when you mean "this category" of ops, do you mean operations that have multiple inputs or outputs?.
There was a problem hiding this comment.
no, this is for ops that we quantize by propagating dequantize ops, e.g. flatten
There was a problem hiding this comment.
no, this is for ops that we quantize by propagating dequantize ops, e.g. flatten
maybe we can include this in the comment?
…with multiple outputs" Summary: e.g. prim::ListUnpack Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D21942108](https://our.internmc.facebook.com/intern/diff/D21942108) [ghstack-poisoned]
…with multiple outputs" Summary: e.g. prim::ListUnpack Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D21942108](https://our.internmc.facebook.com/intern/diff/D21942108) [ghstack-poisoned]
|
This pull request has been merged in 2a06a69. |
…le outputs (pytorch#39551) Summary: Pull Request resolved: pytorch#39551 e.g. prim::ListUnpack Test Plan: Imported from OSS Differential Revision: D21942108 fbshipit-source-id: 6fd7c972ca70692ec52c296b6a1e858324e66c12
Stack from ghstack:
aten::append#39644 [quant][graphmode] Support quantization foraten::appendifin insert_observers #39615 [quant][graphmode] Fix a corner case in handlingifin insert_observersSummary:
e.g. prim::ListUnpack
Test Plan:
Reviewers:
Subscribers:
Tasks:
Tags:
Differential Revision: D21942108