Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect ?gemmt signature, transposition not supported #4005

Closed
tttapa opened this issue Apr 15, 2023 · 3 comments · Fixed by #4006
Closed

Incorrect ?gemmt signature, transposition not supported #4005

tttapa opened this issue Apr 15, 2023 · 3 comments · Fixed by #4006

Comments

@tttapa
Copy link
Contributor

tttapa commented Apr 15, 2023

The gemmt functions seem to include an additional M argument that is not present in other BLAS libraries. The result of gemmt is always square (M == N), so the additional dimension is not required.

https://github.com/xianyi/OpenBLAS/blob/4eac244c9a1088cdb892a47908d5e4d46abace20/interface/gemmt.c#L70-L74

Intel MKL:
dgemmt (uplo, transa, transb, n, k, alpha, a, lda, b, ldb, beta, c, ldc)

IBM ESSL:
dgemmt (uplo, transa, transb, n, k, alpha, a, lda, b, ldb, beta, c, ldc)

Compare this to gemm, which does include the M argument to handle non-square matrices:

Intel MKL:
dgemm(transa, transb, m, n, k, alpha, a, lda, b, ldb, beta, c, ldc)

Unsurprisingly, OpenBLAS crashes with a segfault when other libraries like MUMPS try to use dgemmt.


I was looking to submit a patch myself, but I couldn't find any tests for dgemmt, so I wrote my own. The tests pass if transA = transB = 'N', but if any of the matrices are transposed, dgemmt returns the wrong result, and additionally corrupts the heap by going out of bounds on its arguments.

The following is a simple test program that works with both the Intel MKL (passes) and OpenBLAS (fails and crashes because of heap corruption).

https://github.com/tttapa/OpenBLAS/blob/39c9cb3564346d6d9c7d68c7a20224746407216c/test/dgemmt.cpp

You can run the tests in a clean ubuntu:jammy Docker container using:

apt update
DEBIAN_FRONTEND=noninteractive apt install -y --no-install-recommends libmkl-dev g++ cmake make git ca-certificates
git clone https://github.com/tttapa/OpenBLAS.git --single-branch --depth=1 --branch=dgemmt-test
cd OpenBLAS
g++ test/dgemmt.cpp -DMKL_ILP64 -I/usr/include/mkl -Wl,--no-as-needed -lmkl_intel_ilp64 -lmkl_sequential -lmkl_core -lpthread -lm -ldl -o dgemmt-test-mkl
./dgemmt-test-mkl
cmake -S. -Bbuild -DBUILD_WITHOUT_CBLAS=On -DBUILD_WITHOUT_LAPACK=On -DBUILD_TESTING=Off
cmake --build build -j
g++ test/dgemmt.cpp -DOPENBLAS -lopenblas -Lbuild/lib -I. -Ibuild -o dgemmt-test-openblas
./dgemmt-test-openblas

Until OpenBLAS' ?gemmt has the same API and supports all operations as other BLAS libraries, it might be best to remove it, or at least export it under a different name.
Build systems will often just try to see if the BLAS library contains a function with a given name, and decide on a certain implementation based on the available BLAS functions and extensions. If a BLAS-like function is available with the right name but the wrong API, the build system cannot know that, and the resulting binary will just crash at run time.

For example, this is what MUMPS does:
https://github.com/coin-or-tools/ThirdParty-Mumps/blob/5fcdbba365286dddbe7df0dfec38c63610058f40/configure.ac#L116-L117

tttapa added a commit to tttapa/cross-python that referenced this issue Apr 15, 2023
@martin-frbg
Copy link
Collaborator

Uhh... not sure what this abomination even is, must have committed a broken first draft by mistake :(
Should be fixed by #4006

@tttapa
Copy link
Contributor Author

tttapa commented Apr 15, 2023

Perfect, thanks for the quick response!
My tests seem to pass for #4006.

@martin-frbg
Copy link
Collaborator

Thank you for the confirmation - and for alerting me to this problem in the first place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants