Repository navigation
Qualcomm AI Engine Direct - Keep the dtype of a recomposed RMSNorm - #24428
shoumikhin wants to merge 3 commits into
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/24428
Note: Links to docs will display an error until the docs builds have been completed. ✅ You can merge normally! (1 Unrelated Failure)As of commit b8fd5ec with merge base a057bb4 ( BROKEN TRUNK - The following job failed but were present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
@claude /pr-review |
|
Claude finished @shoumikhin's task in 25m 39s —— View job PR Review: #24428No blockers, but in rare mixed-dtype graphs the pass can still build an Detailed reviewSummaryThe PR makes Nothing blocks the PR. The problems are:
All findings come from reading the code. Python can't run in this environment, so none of them were executed. Code Quality
Testing
RecommendationNeeds Discussion Nothing blocks. Item 3 needs an answer on whether QNN's RmsNorm requires one dtype throughout, because that decides how items 2 and 3 should be fixed. Items 1 and 2 are small enough to close in this PR.
|
424b1fc to
ca4b6e5
Compare
|
Thanks for the review. The latest push changes this:
Tests: the dtype cases are now in their own test, |
|
@claude /pr-review |
|
Claude finished @shoumikhin's task in 36m 11s —— View job PR Review: #24428During quantization capture, the new check between the normalization and the weight stops a norm that casts back with Detailed reviewSummaryThe PR makes
I covered both changed files. Python can't run in this environment, so nothing was executed; every finding comes from reading the code. Code Quality
RecommendationRequest Changes Item 1 makes the quantization flow worse for norms written with
|
ca4b6e5 to
22dc132
Compare
|
Thanks. The latest push changes this:
|
|
@claude /pr-review |
|
Claude finished @shoumikhin's task in 42m 16s —— View job PR Review: #24428Nothing blocks this PR. Quantization capture can still rebuild a norm whose weight has another dtype, no test runs the lifted Detailed reviewSummaryThe PR changes two passes:
I found no blockers: three minor code problems and one test gap. All three changed files were covered. Python can't run in this environment, so every finding comes from reading the code. Code Quality
Testing
RecommendationApprove Nothing blocks. Item 1 and the test gap are small and worth folding into this PR. Items 2 and 3 are one-line cleanups.
|
… Samsung passes (pytorch#24316) ## What is wrong today torch.export does not record some of the node names it assigns: the names of inputs, and the names of ops it renamed to make room for an input. With an input called `mul`, the model's own `input * 2` becomes `mul_1`, and nothing marks `mul_1` as taken. A later pass that adds a node can pick one of those names again, and the graph breaks: ``` RuntimeError: Node redefined name t_1! ``` QNN models with autocast regions or attention hit this in the QNN export passes. The Samsung passes hit it too: with an input named `cat`, `torch.cat` followed by `torch.roll` fails in `DecomposeRoll`. ## What this change does `reserve_node_names` in `backends/transforms/utils.py` records the name of every node in the top-level graph, so nodes added later get names that are not taken. The QNN and Samsung pass managers call it at the start of `_transform`. torch.fx has no public way to reserve a name, so the helper adds the names to the graph's name set directly. Copying the graph would register the names through public API, but it also renames inputs named after Python builtins (`input` becomes `input_1`) while the graph signature keeps the old names. The real fix belongs in torch.export (pytorch/pytorch#202927), and a TODO says to drop the helper once that lands. ## Tests `test_export_pipeline_keeps_node_names_unique` runs the QNN export passes on an autocast model and on an attention model whose inputs are named `input` and `mul`. It checks that the graph lints, keeps the input names the graph signature uses, and matches eager, and that the collision really happens without the helper. If a torch update removes the collision, the test skips with a pointer to the torch issue instead of failing. On Linux x86_64 with torch 2.14 and a 2.16 nightly: the test fails with `Node redefined name t_1!` and `Node redefined name mul_1!` when the call is removed, and it fails on the `input` case when the helper copies the graph instead. The Samsung case above fails without the helper and passes with it (torch only). `test_passes.py` passes, except two tests that need torchtune or the quantized CPU kernels. Earlier versions of this PR also carried an HTP change that decomposes constant-only ops left on the CPU, and an RMSNorm dtype fix. Both now have their own PRs: RMSNorm dtype fix: pytorch#24428 HTP constant-only decomposition: pytorch#24429 cc @cbilgin @psiddh
22dc132 to
2465f21
Compare
|
About a weight in another dtype during quantization capture: I kept the rebuild on purpose. The quantizer quantizes the weight with the same spec as the input, so QNN gets the input, the weight and the output of RmsNorm as one quantized type (8-bit with 8a8w, 16-bit with 16a16w). I ran a float32 input through PTQ and lowered it for SM8650, with a bfloat16 weight (8a8w and 16a16w) and a float16 weight (8a8w). With the rebuild, the whole norm becomes one delegated I added a capture case with a bfloat16 weight and a float32 input that expects the rebuild, so this choice is now tested. |
|
@claude /pr-review |
|
Claude finished @shoumikhin's task in 49m 38s —— View job PR Review: #24428Nothing blocks this PR: three minor gaps leave norms decomposed that could be rebuilt, mostly during quantization capture, and one edge-flow case is untested. Detailed reviewSummaryThe PR makes There are no blockers:
All three changed files were covered. Python needs approval in this environment, so nothing was executed. Every finding comes from reading the code. Testing
Performance
RecommendationApprove Nothing blocks. Performance item 1 needs an answer: should a weight wider than the input get the same capture-mode rebuild that reply 3 keeps for a narrower one? The other items are small.
|
RecomposeRmsNorm finds the norm's input by skipping casts, but it only skipped aten.to.dtype. Edge graphs carry the cast as _to_copy or _to_dim_order_copy, so for a Qwen-style norm (upcast to float32, normalize, cast back) the rebuilt rms_norm read the float32 cast and the cast back was lost. Its metadata was copied from the original output, so the graph still claimed the old dtype until a later pass recomputed it. A float16 Qwen norm then failed to lower with "rms_norm was not decomposed or delegated". A bfloat16 Qwen norm followed by masked_scatter failed with "expected self and source to have same dtypes", and when the rebuilt norm stayed on the CPU, to_executorch failed with "Missing out variants: aten::rms_norm". The pass assumes QNN's RmsNorm takes one dtype throughout, so it now rebuilds the norm only when its input and its output share a dtype, and no cast in between rounds below that dtype: - It skips casts that change only the dtype: to.dtype or type_as in captured graphs, _to_copy or _to_dim_order_copy in edge graphs, whose output keeps the input's layout, device and dim order. - It walks back to the nearest tensor in the output dtype, but not past a cast to a narrower dtype or one that changes the dim order. - Only such casts may sit between the normalization and the weight. The "*" in the patterns can match any op, so before, a skipped norm let a later multiply be rebuilt as the norm with the wrong weight. - Outside quantization capture, the dtype also needs a QNN type. - The tensor it scales has to be the tensor it squares, past such casts (T5's norm squares a float32 copy), and the square has to be pow(2) or h * h, past such casts. Before, any multiply by a root mean square was rebuilt as a norm of the squared tensor, and the scaled tensor was dropped. So a float16 Qwen norm is rebuilt on its float16 input. A norm that rounds to float16 before a float32 weight, and a bfloat16 norm, stay decomposed, unless the bfloat16 norm is about to be quantized. A new test covers these cases, with and without dim order, with a channels-last input and a cast that changes its dim order, a Llama-style norm, a T5 norm, float16 rounding before and inside a float32 norm, a multiply after the norm, a square of two casts, and quantization capture, including type_as and a weight narrower than the input there, and a wider one, which stays decomposed since rms_norm returns its input's dtype. Edge graphs first go through the QNN export pipeline, as in to_edge_transform_and_lower_to_qnn, so the exponent of pow is a lifted constant. The test checks which tensor the norm reads and compares the output with eager, with random weights. Another test checks that a different scaled tensor, a cube and a product of two tensors stay decomposed, with and without the lifted exponent.
Without dim order, a layout change in an edge graph is a _to_copy with a memory_format argument, and RemoveRedundancy drops every such _to_copy, because QNN does not need layout changes. A cast like x.to(torch.float16, memory_format=torch.contiguous_format) is also a _to_copy with memory_format, so its dtype change was dropped too, and later ops read the old dtype while their metadata said the new one. RemoveRedundancy now drops a _to_copy with memory_format only when it keeps the dtype, as it already does for _to_dim_order_copy. The new test exports such casts without dim order. A layout-only cast is still removed, and a cast to float16 stays, with a float16 output.
RecomposeRmsNorm rebuilds a norm into one rms_norm only when its input, its weight and its output share a dtype. A Llama-style norm with a bfloat16 input or weight upcasts to float32, normalizes, casts back and multiplies by the weight, so it stayed decomposed. In float16 mode QNN then ran its float32 pow, mean and rsqrt in float16. x * x passes the largest float16 value (65504) once |x| > 255.9, so rows with large values, like the massive activations of a language model, came out wrong (relative error up to 4.5 on SM8650 for values of 1000 to 2000). When the whole norm cannot be rebuilt and it is not about to be quantized, the pass now rebuilds only its float32 normalization, as an rms_norm without a weight, and leaves the casts and the weight multiply after it as they are. rms_norm computes in float32, so this matches what the model computes. A float32 norm whose weight is not one value per channel, such as a 0-d scale, gets the same. A normalization in float16 stays decomposed, since rms_norm would compute it in float32 instead. Quantization capture never takes this path, and quantized models lower as before. rms_norm has no CPU kernel, so a rebuilt norm that QNN refuses fails the lowering. The pass now rebuilds only what QNN's RmsNorm takes: the last of 1 to 4 dims, with static sizes and a constant eps, both for a whole norm and for its normalization, and also during quantization capture. Anything else stays decomposed. This also lets a rank-5 or a dynamic-shape float32 norm lower, which failed before. On the QNN SDK's x86 simulator for SM8650 (float16), a bfloat16 norm and a float32 norm with a bfloat16 weight now lower to RmsNorm, with a worst relative error of 0.005 and 0.0007 on rows with values up to 2000, like a float32 norm (0.0008). The test now also checks whether each rebuilt norm keeps its weight. It expects the float32 normalization alone to be rebuilt for a bfloat16 norm, a float32 input with a bfloat16 weight, a float16 cast before a float32 weight, a cast that changes the dim order, and a multiply after the norm, and no normalization-only rebuild in float16 or during quantization capture. A new test checks that a rank-5 input, a dynamic dim and an eps buffer stay decomposed, and that a norm with a 0-d scale is rebuilt without it.
2465f21 to
b8fd5ec
Compare
|
Thanks for the review. The new push adds a third commit and answers the 4 points:
|
|
@claude /pr-review |
PR Review: #24428
|
|
@claude /pr-review |
PR Review: #24428
|

What is wrong today
RecomposeRmsNormrebuilds a decomposed RMSNorm into onerms_normnode. It walked back to the norm's input overaten.to.dtypecasts only, but edge graphs use_to_copyor_to_dim_order_copy. So for a Qwen- or Llama-style norm (upcast to float32, normalize, cast back), the rebuiltrms_normread the float32 cast and lost the cast back.A float16 norm did not lower, and bfloat16 models failed later:
What this change does
The first commit rebuilds the whole norm only if its input and output share a dtype and no cast in between rounds below it:
pow(2)orh * h.So a float16 Qwen- or Llama-style norm is rebuilt on its float16 input.
A bfloat16 norm, or a float32 norm with a bfloat16 weight, still stays decomposed, and QNN's float16 mode then overflows when it squares values above 255.9. The third commit rebuilds its float32 normalization alone, as an
rms_normwithout a weight. On the QNN SDK's x86 simulator for SM8650 (float16), the worst relative error on rows with values from 300 to 2000 drops from 4.5 to 0.005.Since
rms_normhas no CPU kernel, every rebuild now needs what QNN's RmsNorm takes: the last of 1 to 4 dims, static sizes, a constant eps. A rank-5 or dynamic-shape float32 norm now lowers instead of failing.The second commit keeps a dtype-changing
_to_copywithmemory_format, whichRemoveRedundancydropped without dim order.Tests
test_recompose_rms_norm_keeps_the_norm_dtypecovers the Qwen, Llama and T5 forms across all three dtypes, with and without dim order. Two more tests check that a different scaled tensor, a cube, a product of two tensors, a rank-5 input, a dynamic dim and an eps buffer stay decomposed.The new tests pass on Linux x86_64 with torch 2.14 and 2.16 nightly. With the QNN SDK (SM8650), a float16 Qwen-style norm is delegated as one
rms_norm, and a bfloat16 norm beforemasked_scatterlowers.cc @cbilgin @psiddh