[Snippets][CPU] Enable verbose perf count insertion pass conditionally #28846
Conversation
aobolensk
force-pushed
the
perf-count-verbose-insertion
branch
from
1d5fba7 to
302fa8e
Compare
aobolensk
changed the title
| @@ -187,6 +190,9 @@ class Subgraph : public ov::op::util::SubGraphOp { | |||
| // True if Subgraph contains ops that are not applicable to auto broadcast rule. | |||
| // (e.g. GroupNormalization, reshape) | |||
| bool m_has_broadcast_sensitive_ops = false; | |||
| #ifdef SNIPPETS_DEBUG_CAPS | |||
| DebugCapsConfig m_debug_config; | |||
There was a problem hiding this comment.
When we store DebugCapsConfig by value in two places (op::Subgraph & LIR), there always be a problem of consistency of these two states. For example, what if I call Subgraph::get_debug_config() after LIR is created? Can we rely on this value? What if it was already changed in the LIR config?
The easiest way to address this issue is to store DebugCapsConfig by pointer in both of the configs. This way we will always automatically maintain consistency. Do you think we can do it?
| @@ -40,7 +40,7 @@ class Config { | |||
| // False if LIR can be built from ov::Model only. Prevents adding I/O expressions | |||
| bool m_manual_build_support = false; | |||
| #ifdef SNIPPETS_DEBUG_CAPS | |||
| DebugCapsConfig debug_config; | |||
| std::shared_ptr<DebugCapsConfig> debug_config; | |||
There was a problem hiding this comment.
I understand why you didn't initialize this pointer - because it should be copied from Subgraph::config.
However, we often dereference the result of "linear_ir->get_config().debug_config" without any additional checks here or here for example. So if a LIR was constructed directly (e.g. in unit tests), we can easily get a segfault.
So we probably need to add an openvino assert to LinearIR::get_config to make sure that debug_config was initialized properly.
There was a problem hiding this comment.
Another, a bit dirtier solution is to default-initialize it here as well. I wouldn't recommend it though, because of the consistency problem we discussed.
There was a problem hiding this comment.
Added assert to LinearIR::get_config
| @@ -40,7 +40,7 @@ class Config { | |||
| // False if LIR can be built from ov::Model only. Prevents adding I/O expressions | |||
| bool m_manual_build_support = false; | |||
| #ifdef SNIPPETS_DEBUG_CAPS | |||
| DebugCapsConfig debug_config; | |||
| std::shared_ptr<DebugCapsConfig> debug_config; | |||
There was a problem hiding this comment.
Another, a bit dirtier solution is to default-initialize it here as well. I wouldn't recommend it though, because of the consistency problem we discussed.
aobolensk
force-pushed
the
perf-count-verbose-insertion
branch
from
8a31d8d to
7316acb
Compare
| @@ -101,6 +101,9 @@ class Subgraph : public ov::op::util::SubGraphOp { | |||
|
|
|||
| size_t get_virtual_port_count() const { return m_virtual_port_count; } | |||
| bool is_quantized() const { return config.m_is_quantized; } | |||
| #ifdef SNIPPETS_DEBUG_CAPS | |||
| DebugCapsConfig& get_debug_config() { return *config.m_debug_config; } | |||
There was a problem hiding this comment.
Minor:
Can we add assert to check that ptr is not null? I see that the m_debug_config is defined in public section as non-const and can be nullified/changed in the code.
Details:
Disable verbose performance count logging only when
OV_SNIPPETS_DUMP_BRGEMM_PARAMSenvironment variable is setTickets: