Skip to content

[IE] UnrollGroupQuantize: make unrolled slice locations unique per consumer - #302

Open
blairducrayoppat wants to merge 1 commit into
openvinotoolkit:developfrom
blairducrayoppat:fix/unroll-group-quantize-duplicate-slice-locations
Open

[IE] UnrollGroupQuantize: make unrolled slice locations unique per consumer#302
blairducrayoppat wants to merge 1 commit into
openvinotoolkit:developfrom
blairducrayoppat:fix/unroll-group-quantize-duplicate-slice-locations

Conversation

@blairducrayoppat

@blairducrayoppat blairducrayoppat commented Jun 19, 2026

Copy link
Copy Markdown

Details:

Problem. UnrollGroupQuantize (via the shared GenericUnrollBase::splitValue) names each
unrolled slice after the value being split — currently
appendLoc(val.getLoc(), "slice_d{axis}_{idx}"). When a single value feeds more than one
unrolled consumer — e.g. a grouped-INT4 per-group scale that is the dequantization scale of
both the weight DynamicDequantize and the activation/matmul DynamicDequantize — the
slices of that shared value get byte-identical locations across the two consumers, which
violates the location-uniqueness invariant StopLocationVerifierPass enforces.

This still reproduces on current develop. Running the pass on the reduction added by this PR,
the two consumers' slices of the same shared scale resolve to the same location:

%2  = IE.Slice %arg2 [0, 0, 0] [1, 1536, 1] ... loc(#loc28)   // consumer "weight_dq"
%12 = IE.Slice %arg2 [0, 0, 0] [1, 1536, 1] ... loc(#loc28)   // consumer "matmul_dq"
#loc28 = loc(fused[#loc7, #loc10])
#loc10 = loc("slice_d0_0")

The d{axis} component added in UD2026.28 does not separate this case: both consumers unroll the
shared value along the same axis, so the axis tag is identical on both sides.

Fix. Derive each slice's location from the consuming op plus an operand tag, keeping the
existing axis and chunk index — takeOpLoc(consumerOp, "{operandTag}_slice_d{axis}_{idx}").
Slices of a shared value then stay unique because they are rooted at the (distinct) consumers,
while the axis tag continues to do its own job of separating one value split along several axes.
All splitValue call sites are updated to pass the consumer + operand tag. The per-chunk reduced
ops (FakeQuantize / DynamicDequantize) keep their consumer-rooted slice_{idx} locations, which
are already unique by construction.

With the fix, the same two slices become:

#loc30 = loc(fused<{name = "weight_dq",  ...}>[#loc10, #loc13])
#loc37 = loc(fused<{name = "matmul_dq", ...}>[#loc21, #loc13])

Test. New LIT test
tests/lit/NPU/dialect/IE/passes/unroll_group_quantize_shared_param_locations.mlir: a shared
scale feeding two DynamicDequantize consumers; asserts each consumer's slices resolve to a
fused location rooted at that consumer + operand tag. It fails on unpatched develop and passes
with this change.

Validation:

Manual validation — the automatic checks on this PR have never run (fork PRs appear to require a
maintainer to approve workflows). Per CONTRIBUTING's allowance for manual results:

  • LIT, using the same invocation as the test's RUN line:
    vpux-opt --init-compiler="platform=NPU4000" --unroll-group-quantize --mlir-print-debuginfo | FileCheck
    • unpatched develop: FAIL (FileCheck exit 1)
    • with this change: PASS
  • Built from source on Windows: OpenVINO at 4089686065a245d648cdd2b99c31884f53cb7a5e (the
    commit pinned by validation/openvino_config.json on develop), npu_compiler at this PR's
    head, RelWithDebInfo, MSVC 19.44, Ninja. Hardware: Intel Core Ultra 7 258V (Lunar Lake),
    NPU 4000.
  • End-to-end, for completeness and as a correction — see below.

Correction to this PR's original claim:

This PR originally reported that it unblocked an end-to-end compile of Qwen3-0.6B grouped-INT4
via the NPUW-LLM path, which had been aborting with
StopLocationVerifierPass Pass failed : Found 40 duplicated names after full verification.

That is no longer true on current develop, and I am withdrawing it. I re-ran the original
reproducer (same model, same harness, same NPU_USE_NPUW/NPUW_LLM configuration) against
develop with and without this change. The two runs are equivalent: neither reports duplicated
names, both proceed past StopLocationVerifierPass, and both fail later at the same place —
UnrollDistributedOps Pass failed : Can't convert 20 Bit to Byte
(vpux/utils/core/mem_size.hpp:128), which is the sub-byte lowering limitation on this
configuration, not something this PR addresses.

So something between UD2026.20 and UD2026.28 changed the pipeline such that this particular model
no longer reaches the colliding configuration. I did not trace which change.

What this PR is now: a fix for a location-uniqueness violation that demonstrably still exists
in develop, plus a regression test that fails without it — not a fix for a currently-observable
compile blocker. If you would rather address the invariant differently, it can be reshaped or
closed.

Tickets:

AI Assistance:

  • AI assistance used: yes.
  • Tooling: Claude Code.
  • What was AI-assisted: the location-uniqueness analysis, the splitValue refactor, the LIT
    regression test, the rebase conflict resolution, and the before/after experiment design.
  • Human validation: I reviewed and approved the change before submitting it. All results above were
    produced by building both configurations from source and running them on real hardware
    (Intel Core Ultra 7 258V / NPU 4000). The before/after comparison was a single-variable control
    — only unroll_group_quantize.cpp was reverted, in the same build tree with identical flags —
    and was checked to have actually rebuilt (differing binaries) before the results were trusted.

@blairducrayoppat

Copy link
Copy Markdown
Author

Follow-up: I filed #303 documenting the next VPUX compiler blocker the same model (Qwen3-0.6B, asymmetric per-group INT4) hits once this PR is applied — a sub-byte (u4) per-group zero-point SEGMENTED slice that ConvertViewOpsToDeclarations cannot lower to a byte offset (Can't convert 4 Bit to Byte). This PR is a hard precondition for reaching it. Cross-linking for context; I'd be glad to attempt that fix as a follow-up once this merges.

@blairducrayoppat

Copy link
Copy Markdown
Author

Note: I've closed #303 — it surfaces on per-group asymmetric INT4, which the dev team flagged as unsupported on NPU (openvinotoolkit/openvino#34450). This PR is narrower: it fixes a StopLocationVerifierPass invariant violation — UnrollGroupQuantize emitting non-unique unrolled slice locations. Whether that's worth addressing on its own — independent of the unsupported configuration that surfaces it — is of course the team's call; happy to align the PR with however you'd prefer to handle it.

…nsumer

GenericUnrollBase::splitValue named each unrolled slice after the value being
split (appendLoc(val.getLoc(), "slice_{idx}")). When one value feeds more than
one unrolled consumer -- e.g. a grouped-INT4 per-group scale that is the
dequantization scale of both the weight and the activation DynamicDequantize --
the low-index slices of that shared value get byte-identical locations across
consumers, and StopLocationVerifierPass aborts with "Found N duplicated names
after full verification".

Derive each slice's location from the consuming op plus an operand tag and chunk
index (takeOpLoc(consumerOp, "{operandTag}_slice_{idx}")) so slices of a shared
value stay unique. The per-chunk reduced FakeQuantize/DynamicDequantize ops keep
their consumer-rooted "slice_{idx}" locations (already unique by construction).

Adds a LIT regression test (unroll_group_quantize_shared_param_locations.mlir)
covering a shared scale feeding two DynamicDequantize consumers.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@blairducrayoppat
blairducrayoppat force-pushed the fix/unroll-group-quantize-duplicate-slice-locations branch from 4983451 to 1c00f41 Compare August 27, 2026 19:34
@blairducrayoppat

blairducrayoppat commented Aug 27, 2026

Copy link
Copy Markdown
Author

Rebased onto current develop (6761af885), and re-validated from a source build. I've also
updated the PR description: one of my original claims no longer holds.

The rebase. One conflict, and it happened to be on the exact line this PR rewrites: UD2026.28
changed splitValue's slice naming to appendLoc(val.getLoc(), "slice_d{axis}_{idx}"), adding the
split axis. I've kept that and applied this PR's change on top —
takeOpLoc(consumerOp, "{operandTag}_slice_d{axis}_{idx}") — rather than reverting it, since the
two tags address different collisions: the axis separates one value split along several axes, and
the consumer root separates one value split by several consumers.

The correction. This PR originally cited an end-to-end compile failure — Qwen3-0.6B
grouped-INT4 aborting with Found 40 duplicated names after full verification. I re-ran that same
reproducer against current develop, with and without this change, and the two runs are
indistinguishable: no duplicated names in either, both proceed past StopLocationVerifierPass, and
both fail later at UnrollDistributedOps ... Can't convert 20 Bit to Byte. Something between
UD2026.20 and UD2026.28 changed the pipeline so this model no longer reaches the colliding
configuration. I haven't traced which change.

So that justification is withdrawn.

What does still hold, and is why I've kept the PR open rather than closing it: the underlying
violation is still present on develop. On the unpatched pass, two different consumers of one
shared scale still receive byte-identical locations:

%2  = IE.Slice %arg2 [0, 0, 0] [1, 1536, 1] ... loc(#loc28)   // "weight_dq"
%12 = IE.Slice %arg2 [0, 0, 0] [1, 1536, 1] ... loc(#loc28)   // "matmul_dq"
#loc28 = loc(fused[#loc7, #loc10])   #loc10 = loc("slice_d0_0")

The new LIT test fails on unpatched develop and passes with this change, so it's a genuine
regression test rather than a restatement of the new naming.

That reframes this from "fixes a compile blocker" to "fixes a latent invariant violation, with a
test". That's a smaller claim. If you'd prefer to handle the invariant differently, I can reshape
it, split it, or close it.

Two practical notes:

  1. The automated checks on this PR have never run — they've been sitting at action_required
    since it was opened, which I believe needs a maintainer to approve workflows for a fork PR. The
    validation above is therefore manual, per CONTRIBUTING's allowance. Happy to re-run anything
    specific you'd like to see.
  2. I can't apply the READY_FOR_REVIEW label myself from a fork. Please let me know if you'd like
    me to do anything else to move it into the review flow.

Separately, and not part of this PR: building develop on Windows with the documented
developer-build-relwithdebinfo preset currently fails for me with error C2855 — the shared
npu_compiler_pch_base PCH is compiled without /permissive- while its consumers get it from
enable_warnings_as_errors(... WIN_STRICT). I worked around it locally with
ENABLE_FASTER_BUILD=OFF. I'll raise that as its own issue rather than mixing it in here.

AI assistance disclosure: this work was AI-assisted (Claude Code) for the analysis, the
rebase conflict resolution, the regression test, and the experiment design. I directed and
reviewed it, and every result above comes from building both configurations from source and
running them on an Intel Core Ultra 7 258V / NPU 4000. The before/after was a single-variable
control — only unroll_group_quantize.cpp reverted, same tree, same flags — and I verified the
binaries actually differed before trusting the comparison.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant