cpu: x64: matmul: fixes correctness issue about tags #2362
Conversation
xuxinzen
added
bug
xuxinzen
changed the title
|
make test |
| @@ -1798,9 +1800,13 @@ void init_aux_values(brgemm_matmul_conf_t &bgmmc, | |||
| const dim_t src_stride = src_d.matches_tag(acbd) | |||
| ? bgmmc.A_strides[1] | |||
| : bgmmc.A_strides[0]; | |||
| const dim_t copy_A_src_stride = src_d.matches_tag(dabc) | |||
There was a problem hiding this comment.
If I understand it right, when src can be treated as plain (i.e. treat_transposed_A_as_plain is true) then we don't copy A so this stride won't be used, if so why do we need to adjust it?
There was a problem hiding this comment.
In this case, dabc can be treated as adbc which should be transpose format and treat_transposed_A_as_plain is false. We need to use copy_A.
There was a problem hiding this comment.
Can't we use bgmmc.transposed_A (as it already accounts for treating transposed as plain) instead of matching tag?
It seems less error prone for all code paths to rely on uniquely set variable than to retest specific tags.
xuxinzen
force-pushed
the
xzeng/fixup_matmul_tag
branch
from
1279bfd to
ffda84d
Compare
|
make test |
xuxinzen
force-pushed
the
xzeng/fixup_matmul_tag
branch
from
ffda84d to
f09eb76
Compare
|
make test |
There was a problem hiding this comment.
My 2 cents: it seems that when unit dimension are present, trying to match tags is bug prone as:
- we have to match the whole set of tags that are equivalent when a unit dimension is present. And if we miss one, that would go to a different code path.
- the order in which tags are matched would lead to weird behaviors.
I guess a more robust path would be to normalize the descriptor first and then match to "canonical" tags. What do you think?
| @@ -1798,9 +1800,13 @@ void init_aux_values(brgemm_matmul_conf_t &bgmmc, | |||
| const dim_t src_stride = src_d.matches_tag(acbd) | |||
| ? bgmmc.A_strides[1] | |||
| : bgmmc.A_strides[0]; | |||
| const dim_t copy_A_src_stride = src_d.matches_tag(dabc) | |||
There was a problem hiding this comment.
Can't we use bgmmc.transposed_A (as it already accounts for treating transposed as plain) instead of matching tag?
It seems less error prone for all code paths to rely on uniquely set variable than to retest specific tags.
Fixes # (MFDNN-12707, MFDNN-12625)
When one tag can be treated as plain or adbc, some offsets should be updated.
WIP: checking if additional test cases are needed