[Snippets] Added type support to LoopPort #28310
Conversation
| @@ -56,42 +56,40 @@ void propagate_updated_subtensor_through_loop(const LinearIR& linear_ir, | |||
| // First step: set new dim value to the corresponding input_ports' dimensions | |||
| if (most_outer_loop) { | |||
| for (const auto& port : loop_info->get_input_ports()) { | |||
| const auto& reg_type = port.expr_port->get_descriptor_ptr()->get_reg().type; | |||
There was a problem hiding this comment.
Do I understand correctly that previously we used undefined reg_type as a marker that loop port is NotProcessed?
There was a problem hiding this comment.
I'd say that this is not so..
Because previously we tried to find Processed ports: Incremented or NotIncremented. But gpr could be incremented and not_incremented and not_processed.. Now we use port.get_type() != LoopPort::Type::NotProcessed to explicitly find needed ports for propagation.
src/common/snippets/src/lowered/pass/validate_unified_loops.cpp
Outdated
Show resolved
Hide resolved
| template<LoopPort::Type T = Type::Incremented, | ||
| typename std::enable_if<T == Type::Incremented || T == Type::NotIncremented, bool>::type = true> | ||
| static LoopPort create(const ExpressionPort& port, size_t dim_idx = 0) { | ||
| return LoopPort(port, dim_idx, T); | ||
| } | ||
|
|
||
| template<LoopPort::Type T, | ||
| typename std::enable_if<T == Type::NotProcessed, bool>::type = true> | ||
| static LoopPort create(const ExpressionPort& port) { | ||
| return LoopPort(port, UNDEFINED_DIM_IDX, Type::NotProcessed); | ||
| } |
There was a problem hiding this comment.
It's cool that we try to use template builders here 👍
However, in this particular case this does not look necessary since we have to explicitly specify the type anyway. It would probably be easier just to use good-old 3 args constructor here and add an assert that Incremented/NotIncremented provide non-default argument.
LoopPort(const ExpressionPort& port, dim_idx = 0, LoopPort::Type = Type::Incremented);
I get it that the present approach doesn't even allow to specify an invalid dim_idx, but it just seems to me that there is an easier way to validate dim_idx, and we already use runtime validation in set_dim_idx anyway.
There was a problem hiding this comment.
Thanks!
Firstly, I tried to use SFINAE-principle as we do in other code parts in Snippets (I learnt it 😄 ). It helps us to avoid invalid dim_idx in case of NotProcessed, as you noticed - this is right.
Secondly, this behavior resolves my big problem which I got during this work.
If we have only one constructor: LoopPort(const ExpressionPort& port, dim_idx = 0, LoopPort::Type = Type::Incremented) and I want to create NotProcessed port, I should always call LoopPort(expr_port, UNDEFINED_DIM_IDX, Type::NotProcessed): user should always pass UNDEFINED_DIM_IDX. And if he pass another value, he will get runtime error (not compilation as in SFINAE).
If we have only one constructor: LoopPort(const ExpressionPort& port, LoopPort::Type = Type::Incremented, dim_idx = 0): user can pass invalid dim_idx again for NotProcessed. Also if he want create loop port which processed dim_idx, he should call LoopPort(const ExpressionPort& port, Type::Incremented, 1) - it's so long (for me at least)! I just want to create default loop port for dim_idx and I have to pass default type of loop port.
I believe that these arguments will help you to notice that probably this is best option at the moment 🤔
Otherwise, we can discuss offline.
There was a problem hiding this comment.
Since I have already shared my opinion about how much I like the current solution, I have to stand up for it 😄
I believe the main benefit we have from it is that in case when we do something wrong, we get an error on compilation stage, not in runtime. This may save some time on debugging, especially for the contributors who are just starting to get acquainted with the Snippets
There was a problem hiding this comment.
I'm glad you both like this solution 😁
As I understood after reading the rest of the comments, we are going to introduce loop port classes soon.
In this case, these factory methods are indeed better because they can be easily replaced with dedicated class constructors.
src/common/snippets/src/lowered/expressions/buffer_expression.cpp
Outdated
Show resolved
Hide resolved
src/common/snippets/src/lowered/pass/clean_repeated_ptr_shifts.cpp
Outdated
Show resolved
Hide resolved
src/common/snippets/src/lowered/pass/validate_unified_loops.cpp
Outdated
Show resolved
Hide resolved
src/common/snippets/tests/src/lowered/pass/extracted_loop_invariants.cpp
Outdated
Show resolved
Hide resolved
a-sidorova
force-pushed
the
fix/159913
branch
from
2c9cfcb to
01aa05d
Compare
|
@IvanNovoselov thanks a lot for the review! Please take a look again 😊 |
- Added more models types to WWB tests - Updated real models to random models in LLM bench tests to save execution time After openvinotoolkit/openvino#28310 is merged, random tiny models are expected to work
### Details: - *Moved fields of the class `LoopPort` to private section and added getters/setters* - *Implemented `Type` to `LoopPort` to distinguish not incremented ports due to double ptr increment and not incremented ports of Brgemm.* - *These changes fix inefficient calculation of Buffer allocation size in dynamic MHA Subgraphs. More details are described in the ticket 159913* ### Tickets: - *157326* - *159913*
Details:
LoopPortto private section and added getters/settersTypetoLoopPortto distinguish not incremented ports due to double ptr increment and not incremented ports of Brgemm.Tickets: