Skip to content

Add Index-64 API as extended API with _64 suffix for CBLAS #846

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

Merged
merged 1 commit into from
Jun 6, 2023

Conversation

mkrainiuk
Copy link
Contributor

@mkrainiuk mkrainiuk commented Jun 1, 2023

Description

This PR extends CBLAS API with _64 suffix to support 64-bit integer, that will allow mixing 32-bit integer and 64-bit integer API in the same environment without symbol conflicts. The API extension proposal: #666

Please note, this PR doesn't include LAPACKE related changes (work in progress), only CBLAS.

Checklist

  • The documentation has been updated. TBD in another PR
  • Partially implements CBLAS/LAPACKE extension for 64bit integer #666.
  • Two new examples and local tests passed with new API. LAPACK-xeigtstz_zec_in failed in the PR verification, but it looks like not related to the PR, I see the same problem in other opened PRs as well.

langou
langou previously approved these changes Jun 3, 2023
@angsch
Copy link
Collaborator

angsch commented Jun 4, 2023

cblas.h supports defining cblas_xerbla as a weak symbol for testing.

void
#ifdef HAS_ATTRIBUTE_WEAK_SUPPORT
__attribute__((weak))
#endif
cblas_xerbla(CBLAS_INT p, const char *rout, const char *form, ...);

Should cblas_64.h do the same?

@langou
Copy link
Contributor

langou commented Jun 4, 2023

cblas.h supports defining cblas_xerbla as a weak symbol for testing. Should cblas_64.h do the same?

Yes, cblas_64.h should support defining cblas_xerbla as a weak symbol for testing.

Good point @angsch. Thanks!

@mkrainiuk
Copy link
Contributor Author

cblas.h supports defining cblas_xerbla as a weak symbol for testing. Should cblas_64.h do the same?

Yes, cblas_64.h should support defining cblas_xerbla as a weak symbol for testing.

Good point @angsch. Thanks!

Thank you for catching it! Added.

Quick local test:

$ gcc -c custom_xerbla.c && gcc cblas_dgemm_64_negative_ldc.c  -Iinclude custom_xerbla.o -Llib -lcblas && ./a.out
This is custom cblas_xerbla_64: 14 cblas_dgemm

$ gcc -c custom_xerbla.c && gcc cblas_dgemm_negative_ldc.c  -Iinclude custom_xerbla.o -Llib -lcblas && ./a.out
This is custom cblas_xerbla: 14 cblas_dgemm

@mkrainiuk
Copy link
Contributor Author

Could you please advise what documentation I can update as part of the checklist? I didn't find CBLAS specific documentation in the repo...

@langou
Copy link
Contributor

langou commented Jun 5, 2023

Hi Maria, you are correct and raising a good point. I do not think there is any CBLAS documentation. Which is an issue. I do not know what to do about this. You are welcome to start a documentation. At least it would be good to document (with a README.md?) the new feature, and start some documentation, and hopefully the documentation will populate itself overtime. I do not think a documentation is necessary for the pull request to be merged. (My opinion.) Let us know what you want to do, what you think. Etc. Julien.

@mkrainiuk
Copy link
Contributor Author

Hi Maria, you are correct and raising a good point. I do not think there is any CBLAS documentation. Which is an issue. I do not know what to do about this. You are welcome to start a documentation. At least it would be good to document (with a README.md?) the new feature, and start some documentation, and hopefully the documentation will populate itself overtime. I do not think a documentation is necessary for the pull request to be merged. (My opinion.) Let us know what you want to do, what you think. Etc. Julien.

Hi Julien, thanks for clarifying it. Let me add an initial CBLAS documentation as a separate PR that won't require re-running build and testing.

@weslleyspereira
Copy link
Collaborator

weslleyspereira commented Jun 6, 2023

I believe the CI errors are unrelated to your PR. I ran the same job (coverage) in my local machine and it succeeded.
Those issues may be solved after a merge (or rebase) with the current master branch in LAPACK.

@mkrainiuk, could you please do the merge (or rebase)? I am happy to do it for you as well. Just let me know.

@mkrainiuk
Copy link
Contributor Author

I believe the CI errors are unrelated to your PR. I ran the same job (coverage) in my local machine and it succeeded. Those issues may be solved after a merge (or rebase) with the current master branch in LAPACK.

@mkrainiuk, could you please do the merge (or rebase)? I am happy to do it for you as well. Just let me know.

I've rebased the branch, but looks like I don't have an access to merge the PR, could you please help to merge it?

@langou langou merged commit a83d8d2 into Reference-LAPACK:master Jun 6, 2023
@mkrainiuk mkrainiuk mentioned this pull request Jun 15, 2023
@langou
Copy link
Contributor

langou commented Jun 21, 2023

I think there were some issues in the CI so I reverted the merge.

This was also mentioned by @angsch in
#852 (comment)

@mkrainiuk, can you have a look?

@mkrainiuk
Copy link
Contributor Author

mkrainiuk commented Jun 21, 2023

I think there were some issues in the CI so I reverted the merge.

This was also mentioned by @angsch in #852 (comment)

@mkrainiuk, can you have a look?

Hi @langou could you please let me know how can I get the build logs from the failed steps? I see that the build failed in https://github.com/Reference-LAPACK/lapack/actions/runs/5338940285/jobs/9677000893?pr=857#logs

Build project
   Each symbol represents 10[24](https://github.com/Reference-LAPACK/lapack/actions/runs/5338940285/jobs/9677001044#step:7:25) bytes of output.
   '!' represents an error and '*' a warning.
    ...............********.******.*****..*****.......  Size: 49K
    ........******************.*************..*.*.....  Size: 99K
    .*..*****..*.*****.......*****..*****.....*!!!!!!!  Size: 149K
    !!!!* Size of output: 154K
Error(s) when building project
   149 Compiler errors

But it doesn't show what was the exact problem (in the attached archive it also shows the same 149 Compiler errors summary without actual error messages) and reports missed tests in the next Test step instead. When I'm building it locally on Ubuntu22.04 + GNU 11.3 it works fine for the came config:

cmake -B build -G Ninja -D CMAKE_BUILD_TYPE=Release -D CMAKE_INSTALL_PREFIX=/home/runner/work/lapack/lapack/lapack_install -D CBLAS:BOOL=ON -D LAPACKE:BOOL=ON -D BUILD_TESTING:BOOL=ON -D LAPACKE_WITH_TMG:BOOL=ON -D BUILD_SHARED_LIBS:BOOL=ON

I also added detailed config/build/testing steps to #857

@langou
Copy link
Contributor

langou commented Jun 22, 2023

Hi @mkrainiuk,

could you please let me know how can I get the build logs from the failed steps?

I do not know more than the log that you sent.

I am not sure whether this is helpful but when I look at the "log" that you just sent which is at
https://github.com/Reference-LAPACK/lapack/actions/runs/5338940285/jobs/9677000893?pr=857#logs

I see things like:
Start 28: example2_64_CBLAS Could not find executable /home/runner/work/lapack/lapack/build/bin/xexample2_64_CBLAS Looked in the following places: /home/runner/work/lapack/lapack/build/bin/xexample2_64_CBLAS /home/runner/work/lapack/lapack/build/bin/xexample2_64_CBLAS /home/runner/work/lapack/lapack/build/bin/Release/xexample2_64_CBLAS /home/runner/work/lapack/lapack/build/bin/Release/xexample2_64_CBLAS /home/runner/work/lapack/lapack/build/bin/Debug/xexample2_64_CBLAS /home/runner/work/lapack/lapack/build/bin/Debug/xexample2_64_CBLAS /home/runner/work/lapack/lapack/build/bin/MinSizeRel/xexample2_64_CBLAS /home/runner/work/lapack/lapack/build/bin/MinSizeRel/xexample2_64_CBLAS /home/runner/work/lapack/lapack/build/bin/RelWithDebInfo/xexample2_64_CBLAS /home/runner/work/lapack/lapack/build/bin/RelWithDebInfo/xexample2_64_CBLAS /home/runner/work/lapack/lapack/build/bin/Deployment/xexample2_64_CBLAS /home/runner/work/lapack/lapack/build/bin/Deployment/xexample2_64_CBLAS /home/runner/work/lapack/lapack/build/bin/Development/xexample2_64_CBLAS /home/runner/work/lapack/lapack/build/bin/Development/xexample2_64_CBLAS home/runner/work/lapack/lapack/build/bin/xexample2_64_CBLAS home/runner/work/lapack/lapack/build/bin/xexample2_64_CBLAS home/runner/work/lapack/lapack/build/bin/Release/xexample2_64_CBLAS home/runner/work/lapack/lapack/build/bin/Release/xexample2_64_CBLAS home/runner/work/lapack/lapack/build/bin/Debug/xexample2_64_CBLAS home/runner/work/lapack/lapack/build/bin/Debug/xexample2_64_CBLAS home/runner/work/lapack/lapack/build/bin/MinSizeRel/xexample2_64_CBLAS home/runner/work/lapack/lapack/build/bin/MinSizeRel/xexample2_64_CBLAS home/runner/work/lapack/lapack/build/bin/RelWithDebInfo/xexample2_64_CBLAS home/runner/work/lapack/lapack/build/bin/RelWithDebInfo/xexample2_64_CBLAS home/runner/work/lapack/lapack/build/bin/Deployment/xexample2_64_CBLAS home/runner/work/lapack/lapack/build/bin/Deployment/xexample2_64_CBLAS home/runner/work/lapack/lapack/build/bin/Development/xexample2_64_CBLAS home/runner/work/lapack/lapack/build/bin/Development/xexample2_64_CBLAS 121/123 Test #28: example2_64_CBLAS ................***Not Run 0.00 sec

I think that's the problem.
Julien.

@langou
Copy link
Contributor

langou commented Jun 22, 2023

Another error from the log:

` 1/123 Test #96: LAPACK-xlintstz_ztest_in .........***Failed 0.00 sec
Running: /home/runner/work/lapack/lapack/build/bin/xlintstz
ARGS= OUTPUT_FILE;/home/runner/work/lapack/lapack/build/TESTING/ztest.out;ERROR_FILE;/home/runner/work/lapack/lapack/build/TESTING/ztest.out.err;INPUT_FILE;/home/runner/work/lapack/lapack/TESTING/ztest.in
Test OUTPUT:

Test ERROR:

CMake Error at /home/runner/work/lapack/lapack/TESTING/runtest.cmake:36 (message):
Test /home/runner/work/lapack/lapack/build/bin/xlintstz returned No such
file or directory`

@mkrainiuk
Copy link
Contributor Author

The problem was fixed in #857

FYI: verbose option in CI config helped me to get actual error from the build step.

diff --git a/.github/workflows/cmake.yml b/.github/workflows/cmake.yml
index 4c4b87f26..5eb21e535 100644
--- a/.github/workflows/cmake.yml
+++ b/.github/workflows/cmake.yml
@@ -109,7 +109,7 @@ jobs:
       run: |
         ctest -D ExperimentalStart
         ctest -D ExperimentalConfigure
-        ctest -D ExperimentalBuild
+        ctest -D ExperimentalBuild --verbose

Does it make sense to document it somewhere or maybe enable by default?

@langou
Copy link
Contributor

langou commented Jun 23, 2023

The problem was fixed in #857

Thanks. Looks good to me.

Does it make sense to document it somewhere or maybe enable by default?

Thanks. Let us try to have it as the default then. I assume the output will be way to verbose for most needs, but I guess that's better than to forget how to get the verbose on. I did not know about this flag. I think having it as a default is better than document it. (My opinion.)

@mkrainiuk
Copy link
Contributor Author

Thanks. Let us try to have it as the default then. I assume the output will be way to verbose for most needs, but I guess that's better than to forget how to get the verbose on. I did not know about this flag. I think having it as a default is better than document it. (My opinion.)

I've opened small PR for this #858

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 this pull request may close these issues.

4 participants