Search graph for quantization nodes #6452
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/6452
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit bb1dd12 with merge base 97a4600 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
facebook-github-bot
added
the
CLA Signed
Erik-Lundell
added
partner: arm
|
Can you keep the PR in Draft stage until the tests are passing? :p |
|
Good point! (rushed this a bit before heading to a conference, so sorry for late response) |
Generalizes the search for quantization parameters.
The idea is to make a graph like this a valid quantized graph:
dq -> view -> transpose -> some_op
^
/
dq ------> expand -------/
For a subset of operations 'passable_op' it is is allowed to "pass
through" the op when searching for qparams. If multiple qparams
are encounterd in one search, they are asserted to be equal.
Signed-off-by: Erik Lundell <[email protected]>
Change-Id: I6dbb82fb39164c246ea74a9642d907dba22ab2c3
Signed-off-by: Erik Lundell <[email protected]> Change-Id: I029d95ecdfa85f5cdc63997ad1eb7515a016bae4
Erik-Lundell
force-pushed
the
search-graph
branch
from
2f7d886 to
6df6a43
Compare
| else: | ||
| consumer_qargs: list[QuantArgs] = [] | ||
| for input in consumer_nodes: | ||
| quant_args = search_quant_arg_downstream(input) |
There was a problem hiding this comment.
can you add a test for this?
There was a problem hiding this comment.
I would say that it's rather well tested through all op and model tests, but it could be a good idea to make an extra tricky graph.
| q_op = exir_ops.edge.quantized_decomposed.quantize_per_tensor.default | ||
| dq_op = exir_ops.edge.quantized_decomposed.dequantize_per_tensor.default | ||
| dq_q_ops = [q_op, dq_op] | ||
| dq_q_ops = (q_op, dq_op) | ||
| passable_ops = [ |
There was a problem hiding this comment.
nit: should these be a decorator for each of these ops?
There was a problem hiding this comment.
Avoiding maintaining a list seems like a good idea, could you go more into detail about how/ where to add such a decorator?
| else: | ||
| quant_node = input_node | ||
| input_zp = get_quant_node_args(quant_node).zp | ||
| input_zp = search_quant_arg_upstream(input_node).zp |
There was a problem hiding this comment.
Nit: though we are checking this for a quant case, but this function can return None. I feel at least we should have some logging in this upstream / downstream search function for easier debugging?
There was a problem hiding this comment.
My reasoning about (the lack of) error handling here was to match the earlier behavior with the hope to solve some additional cases. Since we have checked is_quant_node = True, we have assumed before that the node is embedded with dq/ q nodes as it should. It's possible that there is some case where we don't have this for some weird reason but that would have failed earlier as well.
|
@digantdesai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Thanks! The hope was to clearly define what a correctly quantized graph should look like, instead of having exceptions for cases such as view before addmm like we had before. We also wanted to make sure that we don't have operators with different qparams at different outputs/ inputs that can't handle it which could cause numerical issues. Such cases should end up as assertion errors instead now. |
|
@digantdesai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
This reverts commit 63017e4.
|
Found type errors! |
|
Whelp, at least that gave me the motivation to install pyre locally |
Generalizes the search for quantization parameters. The idea is to make a graph like this (part of) a valid quantized graph:
dq -> view -> transpose -> some_op
dq ------> expand -----------/^
For a subset of operations 'passable_op' that don't modify the values of the tensors it is is allowed to "pass through" the op when searching for qparams. If multiple qparams are encounterd in one search, they are asserted to be equal.
The reason for this is to unify what a quantized graph looks like, removing the need for some ad hoc exceptions (for example checking if a view is before an addmm etc.) that were in place before and in general cleaning up the handling of quantization in the ArmBackend. In particular, some decompositions in the to_edge step seem to not insert q/ dq ops which breaks the pattern of dq -> op -> q.
With this change, Arm internal passes can also in some cases skip inserting quantize ops, simplifying the passes.
Change-Id: I6dbb82fb39164c246ea74a9642d907dba22ab2c3