[qti-chuteng]

Summary

Normalize and validate static Gather and GatherBlockQuantized indices before QNN consumes them.

Changes

• Reuse common index normalization for static int32 and int64 indices.
• Convert validated int64 indices to int32 only after negative-index normalization and bounds checks.
• Apply the same validation to GatherBlockQuantized and add focused regression coverage.

Commits

• 1c5d8f5 — Add Gather index validation tests.
• ec3dfef — Validate static Gather indices.

Validation

• git diff --check passed.
• Build and test binaries were not run, as requested.

[CLAassistant]

All committers have signed the CLA.

[minfhong-qti]

I think there are too many fixes in one PR. Probably further separate into several PRs.

[huaychou]

LGTM

[qti-yuduo]

1. [Medium] The new Gather regression test likely passes on the pre-fix code
   onnxruntime/test/providers/qnn/gather_test.cc:356-366

The test uses an int64 index of 2147483648 with an axis dimension of 2147483649 and expects ExpectedEPNodeAssignment::None.

The pre-fix code (MakeStaticIndicesPositiveAndValidate<int64_t, int32_t>) would:

pass the int64 bounds check (2147483648 < 2147483649), then
do static_cast<int32_t>(2147483648), producing -2147483648.
QNN rejects negative static indices and the node falls back to CPU — this is documented directly in normalize_indices_utils.h ("QNN rejects both [negative and INT_64]... silently drops the node to CPU") and is the reason the ScatterElements/ScatterND tests expect QNN assignment only after normalization. With a negative truncated index, the pre-fix code also produces ExpectedEPNodeAssignment::None, so the test passes without the fix.

The unusual {2147483649, 0} data shape also means the test may pass because the QNN EP rejects the huge dimension for unrelated reasons, rather than because of the index check.

Suggested fix: add a direct unit test for utils::NormalizeIndicesBytes<int64_t> (false for INT32_MAX + 1, true for INT32_MAX and in-range negative values), or restructure the integration test so the pre-fix truncation would produce an observable wrong result rather than the same CPU fallback. If the integration test is kept, treat it as a "falls back on out-of-range" check, not as the regression guard for the truncation fix.

Caveat: If a particular QNN version accepts the negative index at validation and instead fails at graph finalize, the test would catch that version-specific behavior. That is not the behavior documented in this codebase, so the test should not rely on it.

2. [Low, behavior change] GatherBlockQuantized out-of-range indices are legal but now reject the whole node
   onnxruntime/core/providers/qnn/builder/opbuilder/gatherblockquantized_op_builder.cc:310

The com.microsoft.GatherBlockQuantized schema explicitly says: unlike ONNX Gather, an out-of-range index produces zeros for the corresponding output slice. The new RETURN_IF_NOT(indices_are_valid, "GatherBlockQuantized: indices are out of range") rejects the whole node, causing a CPU fallback.

The fallback produces the correct zeros, so this is not a correctness bug. If QNN currently rejects out-of-range static values anyway, the outcome is unchanged (fallback); if QNN would have accepted them and zero-filled, this is a coverage regression. Either way, the validation is stricter than the op contract, and the error message makes a valid model look invalid. Worth a comment explaining the intentional fallback, and a test that locks in the expected assignment for out-of-range static indices.

3. [Low, latent] The GatherBlockQuantized bound is taken from the packed ONNX weight shape, not the QNN weight shape
   onnxruntime/core/providers/qnn/builder/opbuilder/gatherblockquantized_op_builder.cc:178-180

gather_axis_dim is computed from weight_info.shape before the weight is reinterpreted for QNN. For packed UInt4x2 data:

ONNX shape is [vocab, hidden * bits / 8], so weight_shape[1] is the packed byte count (hidden / 2 for bits=4).
The QNN weight tensor created below is [vocab, scale_shape[1] * block_size], i.e. [vocab, hidden].
For gather_axis == 1, the new normalization/bounds checks use hidden / 2 while QNN's tensor axis has size hidden. A negative index would normalize to the wrong row, and valid positive indices in [hidden / 2, hidden) would be rejected.

This is not reachable for valid models today because the schema requires gather_axis == 0 for uint8 data. The builder, however, does not enforce that constraint. Computing the bound from the QNN shape ({weight_shape[0], scale_shape[1] * block_size}[gather_axis]) would make the code correct regardless of the packing form and avoid relying on schema validation. If the ONNX shape is kept, add an explicit rejection for gather_axis != 0 with packed uint8 data.

4. [Low] The new GatherBlockQuantized test only covers the int64 branch
   onnxruntime/test/providers/qnn/gatherblockquantized_test.cc:241-249

The PR adds normalization for both int32 and int64 static indices, but the new test uses TestInputDef<int64_t> only. The new NormalizeIndicesBytes<int32_t> branch in gatherblockquantized_op_builder.cc:305-309 is untested. Adding an int32 variant would be cheap and would directly cover the branch introduced by this PR.

5. [Low, maintainability] The GatherBlockQuantized deterministic rename lacks the Gather explanation
   onnxruntime/core/providers/qnn/builder/opbuilder/gatherblockquantized_op_builder.cc:312-314

The Gather builder has a comment explaining why the name must be deterministic rather than produced by UniqueNameGenerator (the generator is stateful/global and is re-invoked on the compile pass, which would duplicate the tensor). The BQ code copies the naming pattern without that comment. docs/qnn_op_builder_guidelines.md explicitly documents this as a footgun. Copy the comment or extract the rename into a shared helper so a future cleanup does not reintroduce the duplication bug.

6. [Trivial] Small cleanups
   gather_op_builder.cc:4: #include is now unused; the only assert was in the removed cast block.
   gather_op_builder.cc:17-20: the top-of-file comment still says static int64 indices are "reinterpreted as uint32 in ProcessAttributesAndOutputs". The new path converts to int32 in ProcessInputs via the shared helper. Pre-existing, but this PR touches the exact path.
   gatherblockquantized_op_builder.cc:298-310: indices_are_valid defaults to false, so an unexpected dtype (neither INT_32 nor INT_64) is reported as "indices are out of range". The ONNX Tind constraint makes this dead code, but if the defensive branch is kept, return a specific "indices must be INT_32 or INT_64" error as ScatterElements does.