Bound the dynamic-qdq traceback in XNNPACK ChannelsLastTaggedReshapePass - #21637
Bound the dynamic-qdq traceback in XNNPACK ChannelsLastTaggedReshapePass#21637Hyungkeun-Park-Nota wants to merge 4 commits into
Conversation
input_to_nhwc steps back over the dynamic q/dq wrapper so the NHWC copy is inserted ahead of the quantize. The loop stopped only once args[0] was not a Node, so it did not stop at the quantized tensor and ran on into ordinary compute. The rewrite that follows is a blanket replace_all_uses_with from wherever the walk landed, so overshooting either feeds an intermediate op NHWC while leaving that op's own output NCHW, which XNNPACK reports as xnn_status_invalid_parameter when propagating input shapes at execute(), or lands on a non-4D constant and raises "required rank 4 tensor to use channels_last format" in _to_copy. Restrict the walk to q/dq nodes. dq -> q -> source is two hops and the source is not a q/dq node, so it stops there. On a w8a8-dynamic detection model 69 of 83 tracebacks had been overshooting, by up to 26 hops; bounding them leaves the delegate count unchanged and drops 16 now-redundant transposes.
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/21637
Note: Links to docs will display an error until the docs builds have been completed.
|
|
@pytorchbot label 'module: xnnpack' 'release notes: xnnpack' |
|
Can you add a test for a producer with multiple consumers?
|
| # lands ahead of the quantize. Only q/dq nodes may be stepped over: | ||
| # walking further reaches ordinary compute, which the blanket | ||
| # replace_all_uses_with below has no business rewriting. | ||
| while (is_quant(input_node) or is_dequant(input_node)) and isinstance( |
There was a problem hiding this comment.
Should this also be is_dynamic_qdq instead?
Two follow-ups from review. Use is_dynamic_qdq for the walk guard instead of is_quant/is_dequant. The branch is already inside "if is_dynamic_input", so the wrapper being stepped over is the dynamic one; a static q/dq pair between the dequantize and its source is a boundary the walk has no reason to cross either. On the graphs in this test file both predicates land on the same node, since the quantize in a dynamic chain is itself dynamic. Adding the multi-consumer test that was asked for turned up a case the walk change breaks. Once the walk stops at the source instead of running back to a placeholder, that source can be a graph output, and the following replace_all_uses_with then points the output at the freshly created copy. The copy has no meta["val"] until the pass retraces, so call() raises KeyError on out_node.meta["val"]. It is also wrong on its own terms: the output has to keep returning the source in its original memory format. Redirect the source's consumers individually and skip the output node. The new test is a SiLU stem ahead of the first convolution, which is where the original YOLOX and SAM failures sat. It has two producers with more than one consumer: the input activation feeds the sigmoid and the mul, and the SiLU output feeds the convolution and the graph output. It fails on main by leaving the mul reading a channels-last copy, fails on the previous revision of this branch with the KeyError above, and passes here.
|
@JakeStevens Switched the walk guard to That test turned up one more thing: the graph output was being redirected to the channels-last copy, so a fix for that is in here too. |
|
The helper can still rewrite every ordinary sibling consumer. For: the pass rewrites tanh to consume the NHWC copy after tanh was classified as NCHW |
|
Thanks. To pass that case as well, changed the helper to redirect only the quantize wrapper (and the choose_qparams feeding it) to the NHWC copy. So, the other consumers keep reading the source. Added the sigmoid/tanh case as a test. Please take a look. |
Follow-up from review: redirect_dynamic_uses_to_nhwc moved every consumer of the traced-back source to the channels-last copy, excluding only the graph output. An ordinary sibling op (sigmoid -> tanh next to sigmoid -> q -> conv) was classified NCHW yet rewired to read the NHWC copy. Invert the selection: move only the dynamic quantize wrapper that the walk stepped over and the choose_qparams feeding it; every other consumer keeps the source. Adds a sibling-branch regression test that fails on the previous revision (tanh reads the _to_copy) and passes here.
2b7a2da to
a1e0579
Compare
|
I think may need to switch the approach to instead tracking the subgraph chain we are interested in. as is, this still does not solve the sibling problem fully, as there is an edge case with two siblings that can both be dynamically quantized, eg And this cannot really be disambiguated. So, instead track the specific q/dq chain being traversed, then redirect only its quantize node and associated qparam nodes |
Review follow-up: with two dynamically quantized siblings sharing one source (a linear and a conv), the type-based redirect moved both chains to the NHWC copy. Record the chain the walk steps over and move only its quantize and the choose_qparams feeding it. Adds the shared linear/conv regression test, which fails on the previous revision.
JakeStevens
left a comment
There was a problem hiding this comment.
thank you for this PR and the back and forth to clean it up!
|
Hi @JakeStevens. Just checking if there’s anything else needed for this PR to be merged. Thanks. |
Summary
The dynamic-quant branch of
ChannelsLastTaggedReshapePass.input_to_nhwctraces back over theq/dq wrapper so the NHWC copy is inserted ahead of the quantize, keeping the
x -> q -> dq -> convchain XNNPACK matches intact. The loop stops on "
args[0]is not a Node" rather than on "this isnot a q/dq node", so it does not stop at the quantized tensor and continues into ordinary compute:
The
input_node.replace_all_uses_with(input_node_nhwc)that follows is then applied from whereverthe walk landed, rewriting consumers the pass never reasoned about. That shows up two ways:
If the walk stops on an intermediate op, lowering succeeds but the serialized graph is
inconsistent, and only the first
execute()reports it:The failing partition contains an elementwise op whose input and output dims disagree, next to a
correct one in the sibling branch:
If the walk reaches a non-4D constant it fails earlier, during the pass:
b_mul_10_const_inputis a(256, 1, 1)per-channel constant. A placeholder has noargs, so thewalk stops there and tries to convert it.
can_be_converted_to_nhwcdoes check rank 4, but thispath never calls it.
Both were found on w8a8 dynamic-quantized vision models, a YOLOX detector for the first and a SAM
image encoder for the second.
Fix
Restrict the walk to q/dq nodes.
dq -> q -> sourceis two hops and the source is not a q/dq node,so the walk stops at the source, which is the node it was aiming for, and the
x -> _to_copy -> q -> dq -> convordering is unchanged.is_quantis already exported frombackends/xnnpack/utils/quant_utils.pyalongside theis_dynamic_qdqthis file imports.Instrumenting the loop on the YOLOX graph: 83 invocations, every one with exactly 2 q/dq hops, and
69 of them (83%) walking past that, up to 26 hops. Bounding the walk leaves the delegate count at
16 either way and removes 16
XNNStaticTransposenodes (414 to 398 total), so the overshoot wasnot buying larger fused partitions.
Test
test_dq_conv2d_eltwise_source_channels_last_tagged_reshape_passbuilds the smallest graph thattriggers it: a dynamically quantized conv whose input is a
sigmoidreading the placeholder, sothe conv sees
sigmoid -> q -> dq. It asserts the sigmoid keeps its placeholder input and that thechannels-last copy sits on the sigmoid's output.
Without the fix the pass produces
x -> _to_copy(channels_last) -> sigmoid -> q -> dq -> convandthe test fails with
AssertionError: 'call_function' != 'placeholder'; with it the order isx -> sigmoid -> _to_copy(channels_last) -> q -> dq -> conv. The assertion is structural becausechannels_lastdoes not change eager results, sorun_method_and_compare_outputsalone cannotcatch this.
The full file passes (22 tests). Separately, 10 w8a8 dynamic models that already lowered and
executed correctly before this change (googlenet, inception_v3, efficientnet_b4, wideresnet50,
sesr_m5, mobile_vit_s, swin_t, vit_b_16, quicksrnet_small, squeezenet1_0) were re-lowered with the
grouped partitioner and executed: 10/10 pass. The two models above now lower and execute, with
output shapes matching a per-op-partitioned reference build.
cc @GregoryComer @digantdesai @cbilgin @JakeStevens