Fix GEMMT transforming the input array B in some complex cases #5143
Conversation
| @@ -688,5 +688,19 @@ void CNAME(enum CBLAS_ORDER order, enum CBLAS_UPLO Uplo, | |||
|
|
|||
| IDEBUG_END; | |||
|
|
|||
| /* transform B back if necessary */ | |||
There was a problem hiding this comment.
@martin-frbg Not sure if I'm interpreting the overall code correctly, but shouldn't B not be written to at all?
While GEMMT is not part of standard BLAS and only exists as an "extension", it does have a definition within MKL, where B is marked as "const". In other words, writing to B should never happen within GEMMT (ie. no in-place operations on B).
https://www.intel.com/content/www/us/en/docs/onemkl/developer-reference-c/2025-0/cblas-gemmt.html
There was a problem hiding this comment.
Technically the promise is only that the inputs will be unchanged on exit, IMHO what happens behind closed doors is implementation-defined. And the current implementation is inefficient and needs to be redone eventually. It was only a quick, GEMV-based hack to provide any support for GEMMT at all when the Reference implementation did not yet have it.
There was a problem hiding this comment.
Agree that it's better to have something rather than nothing. Looking purely at the implementation side, writing to B would still be ill-advised as it's promised as "const" by MKL. One possible case is that B is shared with another thread (say in a non-OpenBLAS function), which expects it unchanged. There may also be a possible performance degradation due to cache pollution stemming from unnecessary writes to B.
There was a problem hiding this comment.
As I say the entire implementation is an ugly hack and needs to be replaced - but that is another can of worms, namely #4921, and I can only do so much in a day (and sometimes not even that)
fixes #5111