[NPUW] Enable vocabulary sharing for the asymmetric vocabulary case - #37054
[NPUW] Enable vocabulary sharing for the asymmetric vocabulary case#37054AsyaPronina wants to merge 9 commits into
Conversation
baf31da to
4112c1e
Compare
… WeightlessAttribute
There was a problem hiding this comment.
Pull request overview
Adds NPUW support for sharing asymmetric quantized vocabularies as inputs, including deferred u8-to-i8 shifting and LM-head rewrites.
Changes:
- Adds i8 asymmetric unpacking and
Sub128lazy transformation. - Adds configurable vocabulary and MatMul-first graph rewrites.
- Extends partitioning to preserve and process i8 vocabulary constants.
Key risks include an SIMD buffer overrun, ignored configuration, stale tests, and missing regression coverage.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/plugins/intel_npu/src/plugin/npuw/util.cpp |
Routes i8 asymmetric unpacking. |
src/plugins/intel_npu/src/plugin/npuw/util_xarch.hpp |
Declares the i8 unpack kernel. |
src/plugins/intel_npu/src/plugin/npuw/util_xarch.cpp |
Implements AVX2 i8 dequantization. |
src/plugins/intel_npu/src/plugin/npuw/partitioning/patterns/opt.cpp |
Extends patterns to i8 weights. |
src/plugins/intel_npu/src/plugin/npuw/partitioning/partitioning.cpp |
Applies deferred shifts to closures. |
src/plugins/intel_npu/src/plugin/npuw/llm_compiled_model.cpp |
Adds vocabulary graph rewrites and configuration. |
src/plugins/intel_npu/src/plugin/npuw/lazy_tensor.hpp |
Defines the Sub128 lazy operation. |
src/plugins/intel_npu/src/plugin/npuw/lazy_tensor.cpp |
Implements and serializes Sub128. |
src/plugins/intel_npu/src/plugin/CMakeLists.txt |
Registers the new cross-compiled kernel. |
src/plugins/intel_npu/src/al/include/intel_npu/config/npuw_option_defs.inc |
Defines new NPUW LLM options. |
| NPUW_ASSERT(from->get_size() == to->get_size()); | ||
| NPUW_ASSERT(from->get_size() % 8 == 0); |
| auto vocab_as_input = get_option<bool>(other_props, std::string("NPUW_ASYM_VOCAB_AS_INPUT")); | ||
| if (m_cfg.get<::intel_npu::NPUW_LLM_ASYM_I8_VOCAB_AS_INPUT>() || | ||
| m_cfg.get<::intel_npu::NPUW_LLM_ASYM_VOCAB_AS_INPUT>()) { |
There was a problem hiding this comment.
That's valid comment
| if ((ov::element::u8 == matched_qweight->get_element_type() || | ||
| ov::element::i8 == matched_qweight->get_element_type()) && | ||
| (standard_layout || pretransposed_layout)) { |
There was a problem hiding this comment.
please take a look
| case TransformType::SUB128: | ||
| m_transform.emplace<op::Sub128>(ov::npuw::orc::load_versioned_payload<op::Sub128>(section)); |
| class ConvertVocabAsymU8ToI8 : public ov::pass::MatcherPass { | ||
| public: | ||
| OPENVINO_MATCHER_PASS_RTTI("ov::npuw::ConvertVocabU8ToI8"); | ||
| explicit ConvertVocabAsymU8ToI8() { |
There was a problem hiding this comment.
Do we need tests here?
dmatveev
left a comment
There was a problem hiding this comment.
Do we need a new HOST_GATHER pattern to properly handle the 0th subgraph?
| INTEL_NPU_NPUW_SIMPLE_OPT(NPUW_LLM_ASYM_VOCAB_AS_INPUT, bool, false, ov::intel_npu::npuw::llm, asym_vocab_as_input, "NPUW_LLM_ASYM_VOCAB_AS_INPUT", LLM, EXPOSED, CACHED, ALL) | ||
| INTEL_NPU_NPUW_SIMPLE_OPT(NPUW_LLM_ASYM_I8_VOCAB_AS_INPUT, bool, false, ov::intel_npu::npuw::llm, asym_i8_vocab_as_input, "NPUW_LLM_ASYM_I8_VOCAB_AS_INPUT", LLM, EXPOSED, CACHED, ALL) | ||
| INTEL_NPU_NPUW_SIMPLE_OPT(NPUW_LLM_MATMUL_FIRST_VOCAB, bool, false, ov::intel_npu::npuw::llm, matmul_first_vocab, "NPUW_LLM_MATMUL_FIRST_VOCAB", LLM, EXPOSED, CACHED, ALL) |
There was a problem hiding this comment.
Do we need all these options? I don't think so.
If there was found a way to share asym (u4) vocabs, let just do it by default
| } else if (ov::op::util::is_constant(input_node) && | ||
| input_node->get_rt_info().count(ov::npuw::weights::op::Sub128::rt_key) > 0) { | ||
| // A Sub128-marked Constant landed in consts_to_keep and stays inline | ||
| // in the function body - the shift would be silently skipped there, | ||
| // producing wrong numerics. Fail loudly instead. | ||
| OPENVINO_THROW("NPUW: Sub128-marked Constant ", | ||
| input_node->get_friendly_name(), | ||
| " is kept in the function body and won't be transformed"); |
There was a problem hiding this comment.
It should be kept anyway, isn't it?
| << "] (via prototype " << proto_layer_name << ")"); | ||
| funcall._lazy_closure[param_idx - function._param_offset] = | ||
| LazyTensor(std::static_pointer_cast<ov::op::v0::Constant>(input_node)); // (t)/1/c | ||
| funcall._lazy_closure[param_idx - function._param_offset] = put_to_closure(input_node); // (t)/1/c |
There was a problem hiding this comment.
Not sure why this special handling is required here.
| ov::Tensor Const::eval() const { | ||
| ov::Tensor Const::eval_view() const { | ||
| if (m_node) { | ||
| return ov::npuw::util::copy_tensor_from_const(m_node); | ||
| return ov::npuw::util::tensor_from_const(m_node); | ||
| } |
| ov::Tensor Sub128::eval() const { | ||
| const auto trs = tensor.get_transformations(); | ||
|
|
||
| ov::Tensor src; | ||
| if (trs.size() == 1 && std::holds_alternative<op::Const>(trs.front())) { | ||
| // Fused path: read straight through a zero-copy view of the source, | ||
| // skipping the intermediate copy Const::eval() would make. The view is | ||
| // only ever READ here, so this is correct for all Const flavors, | ||
| // including the deserialized ones (read-only mmap / cached bin tensor) | ||
| src = std::get<op::Const>(trs.front()).eval_view(); | ||
| } else { | ||
| src = tensor.eval(); | ||
| } | ||
|
|
||
| const auto src_type = src.get_element_type(); | ||
| NPUW_ASSERT(src_type == ov::element::u8 || src_type == ov::element::i8); | ||
|
|
||
| ov::Tensor dst(ov::element::i8, src.get_shape()); | ||
| const auto* s = static_cast<const uint8_t*>(src.data()); | ||
| auto* d = dst.data<int8_t>(); | ||
| for (std::size_t i = 0, n = src.get_size(); i < n; ++i) { | ||
| d[i] = static_cast<int8_t>(static_cast<int8_t>(s[i]) - 128); | ||
| } | ||
| return dst; | ||
| } |
There was a problem hiding this comment.
Honestly this could've been a generic subtract, I see no point in leaving it 128 only
| class ConvertVocabAsymU8ToI8 : public ov::pass::MatcherPass { | ||
| public: |
There was a problem hiding this comment.
I believe this pass must be moved to the passes.
| auto vocab_as_input = get_option<bool>(other_props, std::string("NPUW_ASYM_VOCAB_AS_INPUT")); | ||
| if (m_cfg.get<::intel_npu::NPUW_LLM_ASYM_I8_VOCAB_AS_INPUT>() || | ||
| m_cfg.get<::intel_npu::NPUW_LLM_ASYM_VOCAB_AS_INPUT>()) { | ||
| lm_head_config["NPUW_HOST_GATHER"] = "NO"; |
There was a problem hiding this comment.
Why? I believe HOST_GATHER was never a case for the lmhead submodel?
| ov::npuw::weights::LazyTensor put_to_closure(const std::shared_ptr<ov::Node>& input_node) { | ||
| auto const_node = std::static_pointer_cast<ov::op::v0::Constant>(input_node); | ||
| ov::npuw::weights::LazyTensor lt(const_node); | ||
| if (const_node->get_rt_info().count(ov::npuw::weights::op::Sub128::rt_key) > 0) { | ||
| LOG_DEBUG("Sub128 marker found on " << const_node->get_friendly_name() << " - applying to the LazyTensor"); | ||
| lt = lt.sub128(); | ||
| } | ||
| return lt; | ||
| } |
There was a problem hiding this comment.
This sound very strange to me. Why we should check it here? Why cant it be part of the normal evaluation?
| auto vocab_as_input = get_option<bool>(other_props, std::string("NPUW_ASYM_VOCAB_AS_INPUT")); | ||
| if (m_cfg.get<::intel_npu::NPUW_LLM_ASYM_I8_VOCAB_AS_INPUT>() || | ||
| m_cfg.get<::intel_npu::NPUW_LLM_ASYM_VOCAB_AS_INPUT>()) { |
There was a problem hiding this comment.
We don't need these options I believe
Details:
Tickets:
AI Assistance: