[quant][graphmode][refactor] propagateQuantizationOps#39550
[quant][graphmode][refactor] propagateQuantizationOps#39550jerryzh168 wants to merge 8 commits intogh/jerryzh168/334/basefrom
Conversation
Summary: This is to prepare for next PR that fixes propagate dequantize for ops with multiple outputs Test Plan: Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]
Summary: This is to prepare for next PR that fixes propagate dequantize for ops with multiple outputs Test Plan: Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]
💊 CI failures summary and remediationsAs of commit 794734d (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. |
Summary: This is to prepare for next PR that fixes propagate dequantize for ops with multiple outputs Test Plan: Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]
Summary: This is to prepare for next PR that fixes propagate dequantize for ops with multiple outputs Test Plan: Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]
Summary: This is to prepare for next PR that fixes propagate dequantize for ops with multiple outputs Test Plan: Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]
Summary: This is to prepare for next PR that fixes propagate dequantize for ops with multiple outputs Test Plan: Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]
| void propagateDequantize(Value* output, const std::vector<Value*> inputs) { | ||
| Node* n = output->node(); | ||
| Graph* graph = n->owningGraph(); | ||
| void removeDequantizeFromInputs(const std::vector<Value*>& inputs) { |
There was a problem hiding this comment.
Can you add a test showing the functionality here before you land?
There was a problem hiding this comment.
this is a refactor, not a new functionality, it's already tested in test_quantize_script.py
Summary: This is to prepare for next PR that fixes propagate dequantize for ops with multiple outputs Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D21942063](https://our.internmc.facebook.com/intern/diff/D21942063) [ghstack-poisoned]
| for (auto* input : inputs) { | ||
| is_dequantized &= input->node()->kind() == Symbol::aten("dequantize"); | ||
| if (isSingleInputGeneralValueAtenFunction(n)) { | ||
| for (auto* output : n->outputs()) { |
There was a problem hiding this comment.
Do we need this for loop inside the if, else statements? Can it be moved outside?
There was a problem hiding this comment.
yeah, this refactor is preparing for next PR, where we do things differently for the else branch
Summary: This is to prepare for next PR that fixes propagate dequantize for ops with multiple outputs Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D21942063](https://our.internmc.facebook.com/intern/diff/D21942063) [ghstack-poisoned]
|
This pull request has been merged in c902146. |
Summary: Pull Request resolved: pytorch#39550 This is to prepare for next PR that fixes propagate dequantize for ops with multiple outputs Test Plan: Imported from OSS Differential Revision: D21942063 fbshipit-source-id: 518b3e437140bec9620988d2eb59b6aae069245e
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:
This is to prepare for next PR that fixes propagate dequantize for ops with multiple outputs
Test Plan:
Reviewers:
Subscribers:
Tasks:
Tags:
Differential Revision: D21942063