Skip to content

Qualcomm AI Engine Direct - Adding QNN backend support for scatter.value core ATen op#20998

Open
qti-horodnic wants to merge 1 commit into
pytorch:mainfrom
CodeLinaro:scatter_value
Open

Qualcomm AI Engine Direct - Adding QNN backend support for scatter.value core ATen op#20998
qti-horodnic wants to merge 1 commit into
pytorch:mainfrom
CodeLinaro:scatter_value

Conversation

@qti-horodnic

@qti-horodnic qti-horodnic commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds support for aten.scatter.value on the QNN backend by extending the existing ScatterElements builder.

torch.scatter(data, dim, index, value) scatters a scalar value into data at positions specified by index. Unlike scatter.src (which takes a tensor), scatter.value takes a scalar which requires special handling in the builder to convert the scalar to a static tensor compatible with QNN's ScatterElements op.

Also updated the new_op_development Claude skill to include testing information for the new testing framework.

Test plan

pytest backends/qualcomm/tests/rework/htp/op/v68/test.py -k "test_scatter_value" -v


python backends/qualcomm/tests/test_qnn_delegate.py TestQNNQuantizedOperator.test_qnn_backend_scatter_value --soc_model SM8750 --host aisw-vm15-labsd --device 545ee4aa --build_folder build-android

python backends/qualcomm/tests/test_qnn_delegate.py TestQNNFloatingPointOperator.test_qnn_backend_scatter_value --soc_model SM8750 --host aisw-vm15-labsd --device 545ee4aa --build_folder build-android

@qti-horodnic

Copy link
Copy Markdown
Contributor Author

@pytorchbot label "release notes: qualcomm"

@github-actions

Copy link
Copy Markdown

This PR needs a release notes: label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

@pytorch-bot

pytorch-bot Bot commented Jul 16, 2026

Copy link
Copy Markdown

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/20998

Note: Links to docs will display an error until the docs builds have been completed.

❌ 2 Cancelled Jobs, 1 Unrelated Failure

As of commit a041f9c with merge base 96c83ed (image):

CANCELLED JOBS - The following jobs were cancelled. Please retry:

FLAKY - The following job failed but was likely due to flakiness present on trunk:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 16, 2026
@psiddh

psiddh commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

@claude review this PR

@claude

claude Bot commented Jul 18, 2026

Copy link
Copy Markdown

Claude finished @psiddh's task in 3m 4s —— View job


Code Review: QNN scatter.value support

  • Read the PR diff against origin/main
  • Review builder, passes, quantizer, and tests
  • Post review feedback

Nicely scoped PR — the pass registrations (i64_to_i32, layout_transform, lift_constant_scalar_operands), annotator entries, and test coverage all line up consistently with the existing scatter.src support. A few things worth a closer look:

🔴 Main concern: quantized updates tensor has no quantization encoding

In op_scatter_elements.py:92-100, the quantized scatter.value path builds the STATIC updates tensor by passing index_node as the tensor_source_node:

updates_tensor_wrapper = self.define_tensor(
    index_node,          # <-- source node for quant/name resolution
    node,
    updates_tensor,      # raw pre-quantized uint8/uint16 values
    ...QNN_TENSOR_TYPE_STATIC,
    ...
)

define_tensor derives the quantization encoding from tensor_source_node via get_quant_encoding_conf(index_node, node) (node_visitor.py:527). Since index_node is an integer index tensor, it has no QCOM_QUANT_ATTRS, so the call returns QNN_QUANTIZATION_ENCODING_UNDEFINED and an empty quant_configs. The result is a STATIC tensor carrying raw quantized integers but no scale/zero-point encoding, so QNN has no way to interpret those bytes as quantized values matching the data tensor.

This is very likely why the quantized case had to be marked pytest.raises(AssertionError) on the emulator (per the new gotcha note). Even where it "works" on-device, silently-missing encoding is fragile. Consider attaching the data tensor's per-tensor quant config to the updates wrapper (e.g. via make_qnn_per_tensor_config + define_custom_tensor_wrapper) so the STATIC updates tensor is properly encoded rather than raw ints. Fix this →

🟠 Manual quantization duplicates existing logic

op_scatter_elements.py:78-86 reimplements scalar quantization by hand:

quantized_val = int(round(scalar_val / scale) + zero_point)
info = torch.iinfo(quant_dtype)
quantized_val = max(info.min, min(info.max, quantized_val))

This duplicates get_quant_tensor_value (node_visitor.py:344-369), which already does tensor.div(scale).add(zero_point).round().to(dtype) and handles the uint16/4-bit/2-bit packing cases. The hand-rolled version does not handle uint16-stored-as-int32 or sub-byte packing, so it will diverge for 16-bit/4-bit activations. Reusing define_tensor's built-in quantization path (feeding it a float torch.full tensor + correct quant source) would avoid this divergence and simplify the branch. This ties directly into the encoding issue above.

🟡 Use the QCOM_DTYPE constant

op_scatter_elements.py:80 hardcodes the string key "dtype":

quant_dtype = quant_attrs.get("dtype", torch.uint8)

The codebase defines QCOM_DTYPE = "dtype" and uses it everywhere else (node_visitor.py). Import and use QCOM_DTYPE for consistency. Also note the .get(..., torch.uint8) default silently masks a missing dtype — elsewhere quant_attrs[QCOM_DTYPE] is accessed directly since it should always be present when QCOM_QUANT_ATTRS exists.

🟡 Rework test marks the fp case as expected-failure too

In tests/rework/htp/op/v68/test.py, all three activation dtypes (8a / 16a / fp) are marked pytest.raises(AssertionError):

@enumerate_activation_dtype(
    [pytest.raises(AssertionError), pytest.raises(AssertionError), pytest.raises(AssertionError)]
)

But the new gotcha note in new_op_development.md only says "scatter.value quantized" fails on the x86 emulator. If the fp path actually passes on the emulator, this test will itself fail (the raises context expects an exception that never comes). Can you confirm the fp case genuinely fails on the emulator? If only the quantized paths fail, the fp entry should be a real tolerance check (Tolerance(...) / CosineSimilarity(...)).

Minor / questions

  • op_scatter_elements.py:88-90 (float path): torch.full(index_tensor.shape, scalar_val, dtype=input_tensor.dtype) — good, updates must match the index shape for QNN ScatterElements. 👍
  • The node_name=f"{node.name}_updates_value" + wrapper_idx=2 override correctly avoids a cache collision with the index_node-sourced index wrapper (wrapper_idx=0), since define_tensor caches on the overridden node_name. Worth a one-line comment noting why index_node is reused as the source (shape source) to prevent confusion with the encoding issue above.
  • Test data uses torch.rand/random inputs without a seed. The test_qnn_delegate.py cases match models.py::ScatterValue, which is consistent with existing ops, so no change needed — just flagging that the reduction is NONE so overlapping index writes are order-dependent (fine here since indices are unique per column/row).

Overall the structure is sound; the encoding + duplicated-quantization points are the ones I'd want resolved before merge, since they affect correctness for anything beyond 8-bit and explain the emulator failures.
· branch scatter_value

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants