[CPU] Add DisableBF16CompCumSumSinGen transformation - #37274
Conversation
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Adds a CPU transformation + runtime support to keep the StyleTTS2/iSTFTNet l_sin_gen CumSum→Sin chain in FP32 during BF16 inference to prevent phase drift.
Changes:
- Introduces
DisableBF16CompCumSumSinGenmatcher pass that tags matched nodes with “disable f32→bf16 conversion”. - Extends CPU Graph BF16 enforcement to preserve a continuous FP32 region around tagged nodes.
- Adds gtest coverage for positive/negative pattern matching.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/plugins/intel_cpu/tests/unit/transformations/disable_bf16_comp_cumsum_sin_gen_test.cpp | Adds unit tests validating the matcher marks expected nodes (and doesn’t match a near-miss). |
| src/plugins/intel_cpu/src/transformations/transformation_pipeline.cpp | Registers the new matcher pass in the CPU transformation pipeline. |
| src/plugins/intel_cpu/src/transformations/cpu_opset/common/pass/disable_bf16_comp_cumsum_sin_gen.hpp | Declares the new matcher pass and documents intended topology/scope. |
| src/plugins/intel_cpu/src/transformations/cpu_opset/common/pass/disable_bf16_comp_cumsum_sin_gen.cpp | Implements the pattern match and tagging via disable_conversion(f32,bf16). |
| src/plugins/intel_cpu/src/node.h / src/plugins/intel_cpu/src/node.cpp / src/plugins/intel_cpu/src/graph.cpp | Plumbs the disable-flag into Node and expands FP32 “skip” region during BF16 precision enforcement. |
|
@zhangYiIntel , could you please review? |
| @@ -721,6 +724,7 @@ class Node { | |||
| MemoryFormatFilter memoryFormatFilter; | |||
| bool enforceBF16evenForGraphTail = false; | |||
| bool keepOriginalPrecision = false; | |||
| bool disableBF16Conversion = false; | |||
There was a problem hiding this comment.
quite strange flag that looks like WA, not the full solution. What if I want to disable conversion from other type
There was a problem hiding this comment.
I would not consider this a workaround, but rather an extension of the existing precision-control mechanism in the CPU plugin. We already have a dedicated guard for fp16 (keepOriginalPrecision), so introducing a BF16 specific guard is consistent with current plugin design.
rkazants
left a comment
There was a problem hiding this comment.
I would recommend to ask review from @maxnick, @CuriousPanCake, @mryzhov
|
You change common transformation pipeline with which you need to be careful since it can impact many other models. Did you run the full validation on all models? |
| @@ -2092,6 +2087,16 @@ void Graph::EnforceInferencePrecision() const { | |||
|
|
|||
| // Pattern-based node skipping for BF16 precision enforcement | |||
| if (inferPrec == ov::element::bf16) { | |||
| // Preserve a continuous f32 region around model-declared precision-sensitive operations. | |||
There was a problem hiding this comment.
For Rope markup in openvino/src/common/transformations/src/transformations/common_optimizations/mark_rope_input_to_keep_in_mixed_precision.cpp, we just use disable_conversion in transformation level why do we need to add a search logic here ?
There was a problem hiding this comment.
The disable_conversion is only a marker, while precision enforcement happens later on CPU plugin graph nodes. Without backward/forward closure from marked seeds, the FP32 region may become discontinuous after graph rewrites and optimizers, causing unintended BF16 enforcement inside the intended sensitive chain.
Hi @rkazants This change is gated by a fixed subgraph match in transformation and also a friendly-name check containing "l_sin_gen" which is mainly TTS/vocoder pipelines, such as the StyleTTS2 / iSTFTNet F0 oscillator path. Unrelated models should not be affected. |
| auto transpose_pre_m = wrap_type<ov::op::v1::Transpose>({any_input(), any_input()}); | ||
|
|
||
| auto interp_pre_3_m = | ||
| wrap_type<ov::op::v4::Interpolate, ov::op::v11::Interpolate>({transpose_pre_m, any_input(), any_input()}); |
There was a problem hiding this comment.
What about v0::Interpolate?
There was a problem hiding this comment.
Add v0::Interpolate support.
| wrap_type<ov::op::v4::Interpolate, ov::op::v11::Interpolate>({transpose_pre_m, any_input(), any_input()}); | ||
| auto interp_pre_4_m = wrap_type<ov::op::v4::Interpolate, ov::op::v11::Interpolate>( | ||
| {transpose_pre_m, any_input(), any_input(), any_input()}); | ||
| auto interp_pre_m = std::make_shared<ov::pass::pattern::op::Or>(OutputVector{interp_pre_3_m, interp_pre_4_m}); |
There was a problem hiding this comment.
Use the shorter '|` syntax for or.
There was a problem hiding this comment.
I kept the explicit Or form because switching to | causes compilation failures in this pass.
There was a problem hiding this comment.
Are you sure you're not missing some import? Check how other transformations do it or ask the LLM to do it. I'm pretty sure it should compile.
There was a problem hiding this comment.
Switched to shorter '|` syntax for or and compiled pass. Thanks!
| wrap_type<ov::op::v4::Interpolate, ov::op::v11::Interpolate>({mul2_m, any_input(), any_input()}); | ||
| auto interp_down_4_m = | ||
| wrap_type<ov::op::v4::Interpolate, ov::op::v11::Interpolate>({mul2_m, any_input(), any_input(), any_input()}); | ||
| auto interp_down_m = std::make_shared<ov::pass::pattern::op::Or>(OutputVector{interp_down_3_m, interp_down_4_m}); |
There was a problem hiding this comment.
Use the shorter '|` syntax for or.
| auto interp_down_3_m = | ||
| wrap_type<ov::op::v4::Interpolate, ov::op::v11::Interpolate>({mul2_m, any_input(), any_input()}); | ||
| auto interp_down_4_m = | ||
| wrap_type<ov::op::v4::Interpolate, ov::op::v11::Interpolate>({mul2_m, any_input(), any_input(), any_input()}); |
There was a problem hiding this comment.
Same question regarding v0::Interpolate
| if (!node) { | ||
| return false; | ||
| } | ||
| return node->get_friendly_name().find("l_sin_gen") != std::string::npos; |
There was a problem hiding this comment.
I'm not sure this is the best way to identify a node. Where is it set?
There was a problem hiding this comment.
Btw, you can check it on the pattern level using a predicate without entering the callback.
There was a problem hiding this comment.
I originally used it only as an additional safety gate to reduce the matching scope. However, I think the current subgraph pattern is already specific enough. We can rely on the structural match alone and remove the dependency on the node name.
There was a problem hiding this comment.
To verify that, additional accuracy validation needs to be run on the changes.
There was a problem hiding this comment.
Generally, I agree, the pattern looks complicated enough to avoid false-positives.
| for (const auto& key : {interp_pre_3_m, interp_pre_4_m, interp_down_3_m, interp_down_4_m}) { | ||
| auto it = pattern_map.find(key); | ||
| if (it != pattern_map.end()) { | ||
| to_mark.push_back(it->second.get_node_shared_ptr()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Are you recognizing what nodes have been matched by Or? You can extract them by using the 'Or' handle: pattern_map.at(interp_down_m). Btw, if you want to mark all the matched nodes by the pattern, consider just using Matcher::get_matched_nodes() and marking them in a loop. That would help you to get rid off the mark_fp32_chain() function.
| return false; | ||
| } | ||
|
|
||
| if (!is_l_sin_gen_node(sin_node)) { |
There was a problem hiding this comment.
Same concern here, friendly name could change in different torch/transformer version, if we are using the pattern matching, could you find some other more deterministic criteria ?
There was a problem hiding this comment.
Same as above, I think the current subgraph pattern is already specific enough. We can rely on the structural match alone. I'll remove the dependency on the node name.
c859eaa to
10f432f
Compare
| auto transpose_pre_m = wrap_type<ov::op::v1::Transpose>({any_input(), any_input()}); | ||
|
|
||
| auto interp_pre_v0_m = wrap_type<ov::op::v0::Interpolate>({transpose_pre_m, any_input()}); | ||
| auto interp_pre_v4_m = wrap_type<ov::op::v4::Interpolate>({transpose_pre_m, any_input(), any_input()}); | ||
| auto interp_pre_v4_with_axes_m = | ||
| wrap_type<ov::op::v4::Interpolate>({transpose_pre_m, any_input(), any_input(), any_input()}); | ||
| auto interp_pre_v11_m = wrap_type<ov::op::v11::Interpolate>({transpose_pre_m, any_input()}); | ||
| auto interp_pre_v11_with_axes_m = | ||
| wrap_type<ov::op::v11::Interpolate>({transpose_pre_m, any_input(), any_input()}); |
There was a problem hiding this comment.
I believe, it would be useful here to build a helper like 'interploate_variations(input)` that would produce all the alternatives for an Interpolate node. It looks like below you're doing the same thing so there's a double reason for that :)
8458650 to
e57c0bf
Compare
e57c0bf to
a18c78a
Compare
|
Hi @CuriousPanCake @zhangYiIntel @rkazants All review comments have been addressed. Could you please take another look? Thanks! |
| const std::string l_sin_gen_prefix = "__module.decoder.generator.m_source.l_sin_gen/"; | ||
|
|
||
| // Names used to look up matched nodes after the pass has run. | ||
| const std::string name_transpose_pre = "transpose_pre"; | ||
| const std::string name_interp_pre = "interp_pre"; | ||
| const std::string name_transpose_pre_cumsum = "transpose_pre_cumsum"; | ||
| const std::string name_cumsum = "cumsum"; | ||
| const std::string name_mul_after_cumsum = "mul_after_cumsum"; | ||
| const std::string name_transpose_after_mul = "transpose_after_mul"; | ||
| const std::string name_scale_mul = "scale_mul"; | ||
| const std::string name_interp_after = "interp_after"; | ||
| const std::string name_transpose_after_interp = "transpose_after_interp"; | ||
| const std::string name_sin = l_sin_gen_prefix + "aten::sin/Sin"; |
There was a problem hiding this comment.
Do we still need these? I think, we ditched the idea of name-based matching?
There was a problem hiding this comment.
These names are only used in test assertions to identify nodes and verify their precision in test case. They are not used for name-based pattern matching in transformation pass. I also simplified the Sin node name to "sin" and removed the unused "l_sin_gen" prefix.
Details:
Tickets:
AI Assistance: